From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62511 invoked by alias); 21 Jul 2017 21:57:40 -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 62315 invoked by uid 89); 21 Jul 2017 21:57:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=thrown, 36287, intermediary, 2684 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Jul 2017 21:57:26 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v6LLvJkF025917 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 21 Jul 2017 17:57:24 -0400 Received: by simark.ca (Postfix, from userid 112) id 319E11E5B1; Fri, 21 Jul 2017 17:57:19 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 365A31E5A6; Fri, 21 Jul 2017 17:57:17 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 21 Jul 2017 21:57:00 -0000 From: Simon Marchi To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Catch exceptions thrown from gdbarch_skip_prologue In-Reply-To: <1500463680-1483-1-git-send-email-yao.qi@linaro.org> References: <1500463680-1483-1-git-send-email-yao.qi@linaro.org> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 21 Jul 2017 21:57:19 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00325.txt.bz2 Hi Yao, Thanks for the patch. It look good to me, I just noted a few nits: On 2017-07-19 13:27, Yao Qi wrote: > PR 21555 is caused by the exception during the prologue analysis when > re-set > a breakpoint. > > (gdb) bt > #0 memory_error_message (err=TARGET_XFER_E_IO, gdbarch=0x153db50, > memaddr=93824992233232) at ../../binutils-gdb/gdb/corefile.c:192 > #1 0x00000000005718ed in memory_error (err=TARGET_XFER_E_IO, > memaddr=memaddr@entry=93824992233232) at > ../../binutils-gdb/gdb/corefile.c:220 > #2 0x00000000005719d6 in read_memory_object > (object=object@entry=TARGET_OBJECT_CODE_MEMORY, > memaddr=93824992233232, memaddr@entry=1, > myaddr=myaddr@entry=0x7fffffffd0a0 "P\333S\001", len=len@entry=1) at > ../../binutils-gdb/gdb/corefile.c:259 > #3 0x0000000000571c6e in read_code (len=1, myaddr=0x7fffffffd0a0 > "P\333S\001", memaddr=) at > ../../binutils-gdb/gdb/corefile.c:287 > #4 read_code_unsigned_integer (memaddr=memaddr@entry=93824992233232, > len=len@entry=1, byte_order=byte_order@entry=BFD_ENDIAN_LITTLE) > at ../../binutils-gdb/gdb/corefile.c:362 > #5 0x000000000041d4a0 in amd64_analyze_prologue > (gdbarch=gdbarch@entry=0x153db50, pc=pc@entry=93824992233232, > current_pc=current_pc@entry=18446744073709551615, > cache=cache@entry=0x7fffffffd1e0) at > ../../binutils-gdb/gdb/amd64-tdep.c:2310 > #6 0x000000000041e404 in amd64_skip_prologue (gdbarch=0x153db50, > start_pc=93824992233232) at ../../binutils-gdb/gdb/amd64-tdep.c:2459 > #7 0x000000000067bfb0 in skip_prologue_sal > (sal=sal@entry=0x7fffffffd4e0) at ../../binutils-gdb/gdb/symtab.c:3628 > #8 0x000000000067c4d8 in find_function_start_sal > (sym=sym@entry=0x1549960, funfirstline=1) at > ../../binutils-gdb/gdb/symtab.c:3501 > #9 0x000000000060999d in symbol_to_sal > (result=result@entry=0x7fffffffd5f0, funfirstline=, > sym=sym@entry=0x1549960) at ../../binutils-gdb/gdb/linespec.c:3860 > .... > #16 0x000000000054b733 in location_to_sals (b=b@entry=0x15792d0, > location=0x157c230, search_pspace=search_pspace@entry=0x1148120, > found=found@entry=0x7fffffffdc64) at > ../../binutils-gdb/gdb/breakpoint.c:14211 > #17 0x000000000054c1f5 in breakpoint_re_set_default (b=0x15792d0) at > ../../binutils-gdb/gdb/breakpoint.c:14301 > #18 0x00000000005412a9 in breakpoint_re_set_one > (bint=bint@entry=0x15792d0) at > ../../binutils-gdb/gdb/breakpoint.c:14412 > > This problem can be fixed by > > - either each prologue analyzer doesn't throw exception, > - or catch the exception thrown from gdbarch_skip_prologue, > > I choose the latter because the former needs to fix *every* prologue > analyzer to not throw exception. > > This error can be reproduced by changing reread.exp. The test > reread.exp > has already test that breakpoint can be reset correctly after the > executable is re-read. This patch extends this test by compiling test > c > file with and without -fPIE. > > (gdb) run ^M > The program being debugged has been started already.^M > Start it from the beginning? (y or n) y^M > x86_64/gdb/testsuite/outputs/gdb.base/reread/reread' has changed; > re-reading symbols. > Error in re-setting breakpoint 1: Cannot access memory at address > 0x555555554790^M > Error in re-setting breakpoint 2: Cannot access memory at address > 0x555555554790^M > Starting program: > /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/reread/reread > ^M > This is foo^M > [Inferior 1 (process 27720) exited normally]^M > (gdb) FAIL: gdb.base/reread.exp: opts= "-fPIE" "ldflags=-pie" : run to > foo() second time (the program exited) > > This patch doesn't re-indent the code, to keep the patch simple. > Regression tested on x86_64-linux (ubuntu xenial), and verified that > it fixes some fails on Debian/stretch. I wouldn't mind if you indented the code in this patch. Many diff viewers are good at highlighting what changed in a line, so it's obvious when it's only whitespaces. Otherwise, here's always git diff/show -w for those who like looking at raw diffs. > > gdb: > > 2017-07-18 Yao Qi > > PR gdb/21555 > * arch-utils.c (gdbarch_skip_prologue_noexcept): New function. > * arch-utils.h (gdbarch_skip_prologue_noexcept): Declare. > * infrun.c: Include arch-utils.h > (handle_step_into_function): Call gdbarch_skip_prologue_noexcept. > (handle_step_into_function_backward): Likewise. > * symtab.c (skip_prologue_sal): Likewise. > > gdb/testsuite: > > 2017-07-18 Yao Qi > > PR gdb/21555 > * gdb.base/reread.exp: Wrap the whole test with two kinds of > compilation flags, with -fPIE and without -fPIE. > --- > gdb/arch-utils.c | 19 +++++++++++++++++++ > gdb/arch-utils.h | 3 +++ > gdb/infrun.c | 9 +++++---- > gdb/symtab.c | 3 ++- > gdb/testsuite/gdb.base/reread.exp | 12 +++++++++--- > 5 files changed, 38 insertions(+), 8 deletions(-) > > diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c > index 2ae3413..2f63b80 100644 > --- a/gdb/arch-utils.c > +++ b/gdb/arch-utils.c > @@ -985,6 +985,25 @@ default_print_insn (bfd_vma memaddr, > disassemble_info *info) > return (*disassemble_fn) (memaddr, info); > } > > +/* Wrapper to gdbarch_skip_prologue, but doesn't throw exception. > Catch > + exception thrown from gdbarch_skip_prologue, and return PC. */ Could you explain in the comment what is returned if an exception is caught by the wrapper? The comment should probably go in the .h, with a "See arch-utils.h" comment here. > +CORE_ADDR > +gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) > noexcept > +{ > + CORE_ADDR new_pc = pc; > + > + TRY > + { > + new_pc = gdbarch_skip_prologue (gdbarch, new_pc); I know it does the same thing, but I think it would be clearer to pass "pc" as a parameter to the function. > + } > + CATCH (ex, RETURN_MASK_ALL) > + {} > + END_CATCH > + > + return new_pc; > +} > + > /* -Wmissing-prototypes */ > extern initialize_file_ftype _initialize_gdbarch_utils; > > diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h > index 2aa9159..11c4afc 100644 > --- a/gdb/arch-utils.h > +++ b/gdb/arch-utils.h > @@ -268,4 +268,7 @@ extern void default_guess_tracepoint_registers > (struct gdbarch *gdbarch, > > extern int default_print_insn (bfd_vma memaddr, disassemble_info > *info); > > +extern CORE_ADDR gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, > + CORE_ADDR pc) noexcept; > + > #endif > diff --git a/gdb/infrun.c b/gdb/infrun.c > index a68da6a..37ff015 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -66,6 +66,7 @@ > #include "common/enum-flags.h" > #include "progspace-and-thread.h" > #include "common/gdb_optional.h" > +#include "arch-utils.h" > > /* Prototypes for local functions */ > > @@ -7317,8 +7318,8 @@ handle_step_into_function (struct gdbarch > *gdbarch, > > cust = find_pc_compunit_symtab (stop_pc); > if (cust != NULL && compunit_language (cust) != language_asm) > - ecs->stop_func_start = gdbarch_skip_prologue (gdbarch, > - ecs->stop_func_start); > + ecs->stop_func_start > + = gdbarch_skip_prologue_noexcept (gdbarch, > ecs->stop_func_start); > > stop_func_sal = find_pc_line (ecs->stop_func_start, 0); > /* Use the step_resume_break to step until the end of the prologue, > @@ -7396,8 +7397,8 @@ handle_step_into_function_backward (struct > gdbarch *gdbarch, > > cust = find_pc_compunit_symtab (stop_pc); > if (cust != NULL && compunit_language (cust) != language_asm) > - ecs->stop_func_start = gdbarch_skip_prologue (gdbarch, > - ecs->stop_func_start); > + ecs->stop_func_start > + = gdbarch_skip_prologue_noexcept (gdbarch, > ecs->stop_func_start); > > stop_func_sal = find_pc_line (stop_pc, 0); > > diff --git a/gdb/symtab.c b/gdb/symtab.c > index d4e107a..ccf31cc 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -64,6 +64,7 @@ > #include "progspace-and-thread.h" > #include "common/gdb_optional.h" > #include "filename-seen-cache.h" > +#include "arch-utils.h" > > /* Forward declarations for local functions. */ > > @@ -3627,7 +3628,7 @@ skip_prologue_sal (struct symtab_and_line *sal) > if (gdbarch_skip_entrypoint_p (gdbarch)) > pc = gdbarch_skip_entrypoint (gdbarch, pc); > if (skip) > - pc = gdbarch_skip_prologue (gdbarch, pc); > + pc = gdbarch_skip_prologue_noexcept (gdbarch, pc); > > /* For overlays, map pc back into its mapped VMA range. */ > pc = overlay_mapped_address (pc, section); > diff --git a/gdb/testsuite/gdb.base/reread.exp > b/gdb/testsuite/gdb.base/reread.exp > index cc0f577..74bd20e 100644 > --- a/gdb/testsuite/gdb.base/reread.exp > +++ b/gdb/testsuite/gdb.base/reread.exp > @@ -15,6 +15,11 @@ > > set prototypes 1 > > +# Build programs in PIE mode, to reproduce PR 21555. > +foreach_with_prefix opts { > + { "" "" } > + { "-fPIE" "ldflags=-pie" } } { > + > # build the first test case > > set testfile1 "reread1" > @@ -22,7 +27,7 @@ set srcfile1 ${testfile1}.c > # Cygwin needs $EXEEXT. > set binfile1 [standard_output_file ${testfile1}$EXEEXT] > > -if { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}" > executable {debug nowarnings}] != "" } { > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}" > executable [list debug nowarnings additional_flags=[lindex $opts 0] > [lindex $opts 1]]] != "" } { > untested "failed to compile first testcase" > return -1 > } > @@ -33,8 +38,8 @@ set testfile2 "reread2" > set srcfile2 ${testfile2}.c > set binfile2 [standard_output_file ${testfile2}$EXEEXT] > > -if { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" > executable {debug nowarnings}] != "" > - && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" > executable {debug nowarnings additional_flags=-DNO_SECTIONS}] != ""} { > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" > executable [list debug nowarnings additional_flags=[lindex $opts 0] > [lindex $opts 1]]] != "" > + && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" > executable [list debug nowarnings "additional_flags=-DNO_SECTIONS > [lindex $opts 0]" [lindex $opts 1]]] != ""} { Could you split those really long lines? It would perhaps help to define an intermediary variable for compilation opts. > untested "failed to compile second testcase" > return -1 > } > @@ -120,6 +125,7 @@ if [is_remote target] { > gdb_test "" "Breakpoint.* foo .* at .*:9.*" "second pass: run to > foo() second time" > } > > +} > # End of tests. > > return 0 Thanks, Simon