From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84436 invoked by alias); 9 Oct 2018 16:15:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 84255 invoked by uid 89); 9 Oct 2018 16:15:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 09 Oct 2018 16:15:09 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 43B5F356E4; Tue, 9 Oct 2018 16:15:08 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3C7AC7E10A; Tue, 9 Oct 2018 16:15:07 +0000 (UTC) Subject: Re: [PATCH v2 2/2] Aarch64: Fix segfault when casting dummy calls To: Alan Hayward , gdb-patches@sourceware.org References: <20181001155255.14859-1-alan.hayward@arm.com> <20181001155255.14859-3-alan.hayward@arm.com> Cc: nd@arm.com From: Pedro Alves Message-ID: <6f827ce2-c97c-dd56-8fe0-e9a9fabd2ddc@redhat.com> Date: Tue, 09 Oct 2018 16:15:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181001155255.14859-3-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-10/txt/msg00216.txt.bz2 On 10/01/2018 04:52 PM, Alan Hayward wrote: > Prevent the following int cast causing a segfault on aarch64: > (gdb) b foo if (int)strcmp(name,"abc") == 0 > (gdb) run > > Instead of incorrectly re-calculating lang_struct_return, reuse > the version passed in. > > Add common test case using a shared library to ensure FUNC resolving. > Isn't an important point here that the called function should have no debug info, so that GDB does not know its return type? The testcase compiles the library with debug info, which confuses me. > gdb/ChangeLog: > > 2018-10-01 Alan Hayward > > PR gdb/22736: > * aarch64-tdep.c (aarch64_push_dummy_call): Reuse > lang_struct_return. > > gdb/testsuite/ChangeLog: > > 2018-10-01 Alan Hayward > > PR gdb/22736: > * gdb.base/condbreak-solib-lib.cc: New test. > * gdb.base/condbreak-solib-main.cc: New test. > * gdb.base/condbreak-solib.exp: New file. > --- > gdb/aarch64-tdep.c | 27 +----- > gdb/testsuite/gdb.base/condbreak-solib-lib.cc | 21 +++++ > .../gdb.base/condbreak-solib-main.cc | 33 +++++++ > gdb/testsuite/gdb.base/condbreak-solib.exp | 93 +++++++++++++++++++ > 4 files changed, 149 insertions(+), 25 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/condbreak-solib-lib.cc > create mode 100644 gdb/testsuite/gdb.base/condbreak-solib-main.cc > create mode 100644 gdb/testsuite/gdb.base/condbreak-solib.exp > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 504b040c2e..f6eabc7029 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -1513,14 +1513,11 @@ static CORE_ADDR > aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > struct regcache *regcache, CORE_ADDR bp_addr, > int nargs, struct value **args, CORE_ADDR sp, > - int struct_return, int lang_struct_return_unused, > + int struct_return, int lang_struct_return, > CORE_ADDR struct_addr) > { > int argnum; > struct aarch64_call_info info; > - struct type *func_type; > - struct type *return_type; > - int lang_struct_return; > > memset (&info, 0, sizeof (info)); > > @@ -1542,27 +1539,7 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > If the language code decides to pass in memory we want to move > the pointer inserted as the initial argument from the argument > list and into X8, the conventional AArch64 struct return pointer > - register. > - > - This is slightly awkward, ideally the flag "lang_struct_return" > - would be passed to the targets implementation of push_dummy_call. > - Rather that change the target interface we call the language code > - directly ourselves. */ > - > - func_type = check_typedef (value_type (function)); > - > - /* Dereference function pointer types. */ > - if (TYPE_CODE (func_type) == TYPE_CODE_PTR) > - func_type = TYPE_TARGET_TYPE (func_type); > - > - gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC > - || TYPE_CODE (func_type) == TYPE_CODE_METHOD); > - > - /* If language_pass_by_reference () returned true we will have been > - given an additional initial argument, a hidden pointer to the > - return slot in memory. */ > - return_type = TYPE_TARGET_TYPE (func_type); > - lang_struct_return = language_pass_by_reference (return_type); > + register. */ > > /* Set the return address. For the AArch64, the return breakpoint > is always at BP_ADDR. */ > diff --git a/gdb/testsuite/gdb.base/condbreak-solib-lib.cc b/gdb/testsuite/gdb.base/condbreak-solib-lib.cc > new file mode 100644 > index 0000000000..02bf450c43 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/condbreak-solib-lib.cc > @@ -0,0 +1,21 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int cmp3 (char *name) Line break before cmp3. > +{ > + return name[0] == '3'; > +} > diff --git a/gdb/testsuite/gdb.base/condbreak-solib-main.cc b/gdb/testsuite/gdb.base/condbreak-solib-main.cc > new file mode 100644 > index 0000000000..43d39d1da4 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/condbreak-solib-main.cc > @@ -0,0 +1,33 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +extern int cmp3 (char *name); > + > +const char *word = "stuff"; > + > +int bar () Write: int bar (void) { > +{ > + return 1; > +} > + > +int main (void) Ditto. > +{ > + cmp3 ((char*)"a"); Write: cmp3 ((char*) "a"); I.e., space after case. Or, make cmp3 take a "const char *" and get rid of the casts? > + bar (); > + cmp3 ((char*)"a"); Ditto. > + bar (); > +} > diff --git a/gdb/testsuite/gdb.base/condbreak-solib.exp b/gdb/testsuite/gdb.base/condbreak-solib.exp > new file mode 100644 > index 0000000000..27639bd109 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/condbreak-solib.exp > @@ -0,0 +1,93 @@ > +# This testcase is part of GDB, the GNU debugger. > +# Copyright 2018 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# Test conditional breakpoints on functions in shared libraries. Add reference to gdb/22736 here. > + > +if { [skip_cplus_tests] } { continue } > + > +if {![is_elf_target]} { > + return 0 > +} Why do you need this? > + > +if [skip_shlib_tests] { > + return 0 > +} > + > +set target_size TARGET_UNKNOWN > +if {[is_lp64_target]} { > + set target_size TARGET_LP64 > +} elseif {[is_ilp32_target]} { > + set target_size TARGET_ILP32 Something odd with indentation here. > +} else { > + return 0 > +} > + > +set main_basename condbreak-solib-main > +set lib_basename condbreak-solib-lib > + > +standard_testfile $main_basename.cc $lib_basename.cc > + > +if [get_compiler_info "c++"] { > + return -1 > +} > + > +set libsrc "${srcdir}/${subdir}/${srcfile2}" > +set lib_so [standard_output_file ${lib_basename}.so] > +set lib_syms [shlib_symbol_file ${lib_so}] > +set lib_dlopen [shlib_target_file ${lib_basename}.so] > + > + > +if [get_compiler_info] { > + return -1 > +} > + > +# Compile a shared library containing cmp3 > + Missing period. > +if {[gdb_compile_shlib $libsrc $lib_so {debug c++}] != ""} { > + untested "failed to compile shared library" > + return > +} > + > +# Compile the main test linked against the shared library Missing period. > + > +set exec_opts [list debug c++ "additional_flags= -I$srcdir/../../include/ -D$target_size\ > + -DSHLIB_NAME\\=\"$lib_dlopen\""] What's the -I..../include for? And SHLIB_NAME/$lib_dlopen? I didn't see any dlopen in the testcase. > + > +if {[prepare_for_testing "failed to prepare" $binfile "$srcfile $srcfile2" $exec_opts]} { > + return > +} > + > +gdb_load_shlib ${lib_so} > + > +if ![runto_main] then { > + fail "can't run to main" > + return > +} > + > +# Create conditional breakpoint on cmp3(), which will fail the condition. > + > +gdb_test "b cmp3 if (int)strcmp(word,\"abc\") == 0" "Breakpoint .*" > + > +gdb_test "b bar" "Breakpoint .*" > + > +gdb_test "c" "Breakpoint .* bar .*" > + > +# Create conditional breakpoint on cmp3(), which will pass the condition. > + > +gdb_test "b cmp3 if (int)strcmp(word,\"stuff\") == 0" "Breakpoint .*" > + > +gdb_test "c" "Breakpoint .* cmp3 .*" > + Duplicate test names here. See: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique Thanks, Pedro Alves