From patchwork Tue Jul 25 10:38:21 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 21754 Received: (qmail 100532 invoked by alias); 25 Jul 2017 10:38:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 99872 invoked by uid 89); 25 Jul 2017 10:38:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=re-run, Wrap, 9856 X-HELO: mail-it0-f47.google.com Received: from mail-it0-f47.google.com (HELO mail-it0-f47.google.com) (209.85.214.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Jul 2017 10:38:26 +0000 Received: by mail-it0-f47.google.com with SMTP id v127so10183229itd.0 for ; Tue, 25 Jul 2017 03:38:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=W8reshY20wp2Vv7FJsJMuVKYUnoC50zvN2Nf+Ob7bWs=; b=XOvPC6wQ2NL00ObxIDDp8SghOYYGGZtlaCq4BhjRttL3gOy1W6PFYGTjhK8AIHWLQW GC1rMUimTWLWMNnh0vfVYb2ZFUtxLh5f4WqYHAsHyTyU/l2BpQrzsGYKQFs175LnbyFI sis6f2IhsoCPq3G1fpAHwlq5jzvuaE0tQgqpIGgWx0knPO/G81JIQa5SfwAnOxnFVDrs P/ea7PKTiCCvq6+hZmEmEGPHk/fnR0LnKRSNEoHsLIRxL5VwZKNqHS4bsb9vcuaoaP+w I5mb4MIiokuwvCcVJse6faDbJEeINTxBSFD4M/dng3Cx0ElGKrYdfvSJGiFIhPWHhaIY v3hw== X-Gm-Message-State: AIVw113dJssVTgNHbjQPOxus+pDj5xt0p0yikRJlbHdfpygbQ5+NHbKR jHF8TAMzDZ7qq72S X-Received: by 10.36.118.194 with SMTP id z185mr9888403itb.66.1500979104844; Tue, 25 Jul 2017 03:38:24 -0700 (PDT) Received: from E107787-LIN (static.42.136.251.148.clients.your-server.de. [148.251.136.42]) by smtp.gmail.com with ESMTPSA id b83sm4770821iod.35.2017.07.25.03.38.23 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Tue, 25 Jul 2017 03:38:24 -0700 (PDT) From: Yao Qi To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Catch exceptions thrown from gdbarch_skip_prologue References: <1500463680-1483-1-git-send-email-yao.qi@linaro.org> Date: Tue, 25 Jul 2017 11:38:21 +0100 In-Reply-To: (Simon Marchi's message of "Fri, 21 Jul 2017 23:57:17 +0200") Message-ID: <86wp6wpzs2.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Simon Marchi writes: >> 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. > It was more about reading patch in email client. Anyway, I put these two into one patch. >> >> 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? I think I did :), "Catch exception throw from gdbarch_skip_prologue, and return PC.". > > The comment should probably go in the .h, with a "See arch-utils.h" > comment here. > OK, done. >> +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. > OK, fixed. >> >> -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. Fixed. Patch below is pushed in. diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index 2ae3413..0402eba 100644 --- a/gdb/arch-utils.c +++ b/gdb/arch-utils.c @@ -985,6 +985,24 @@ default_print_insn (bfd_vma memaddr, disassemble_info *info) return (*disassemble_fn) (memaddr, info); } +/* See arch-utils.h. */ + +CORE_ADDR +gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) noexcept +{ + CORE_ADDR new_pc = pc; + + TRY + { + new_pc = gdbarch_skip_prologue (gdbarch, pc); + } + 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..fa29086 100644 --- a/gdb/arch-utils.h +++ b/gdb/arch-utils.h @@ -268,4 +268,10 @@ extern void default_guess_tracepoint_registers (struct gdbarch *gdbarch, extern int default_print_insn (bfd_vma memaddr, disassemble_info *info); +/* Wrapper to gdbarch_skip_prologue, but doesn't throw exception. Catch + exception thrown from gdbarch_skip_prologue, and return PC. */ + +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..4e611ce 100644 --- a/gdb/testsuite/gdb.base/reread.exp +++ b/gdb/testsuite/gdb.base/reread.exp @@ -15,111 +15,131 @@ set prototypes 1 -# build the first test case +# Build programs in PIE mode, to reproduce PR 21555. +foreach_with_prefix opts { + { "" "" } + { "-fPIE" "ldflags=-pie" } } { -set testfile1 "reread1" -set srcfile1 ${testfile1}.c -# Cygwin needs $EXEEXT. -set binfile1 [standard_output_file ${testfile1}$EXEEXT] - -if { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}" executable {debug nowarnings}] != "" } { - untested "failed to compile first testcase" - return -1 -} - -# build the second test case - -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}] != ""} { - untested "failed to compile second testcase" - return -1 -} - -# Start with a fresh gdb. - -set testfile "reread" -set binfile [standard_output_file ${testfile}$EXEEXT] - -gdb_start -gdb_reinitialize_dir $srcdir/$subdir - -# Load the first executable. - -gdb_rename_execfile ${binfile1} ${binfile} -gdb_load ${binfile} - -# Set a breakpoint at foo - -gdb_test "break foo" \ - "Breakpoint.*at.* file .*$srcfile1, line 14.*" \ - "breakpoint foo in first file" - - -# Run, should see "Breakpoint 1, foo () at hello1.c:14" - -gdb_run_cmd -gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" "run to foo()" - -# Restore first executable to its original name, and move -# second executable into its place. Ensure that the new -# executable is at least a second newer than the old. - -gdb_rename_execfile ${binfile} ${binfile1} -gdb_rename_execfile ${binfile2} ${binfile} -gdb_test "shell sleep 1" ".*" "" -gdb_touch_execfile ${binfile} - -# Run a second time; GDB should detect that the executable has changed -# and reset the breakpoints correctly. -# Should see "Breakpoint 1, foo () at reread2.c:9" - -set test "run to foo() second time" -if [is_remote target] { - unsupported $test -} else { - gdb_run_cmd - gdb_test "" "Breakpoint.* foo .* at .*:9.*" $test -} - - -### Second pass: verify that GDB checks the executable file's -### timestamp when the program is *restarted*, not just when it exits. - -if [is_remote target] { - unsupported "second pass: GDB should check for changes before running" -} else { - - # Put the older executable back in place. - gdb_rename_execfile ${binfile} ${binfile2} - gdb_rename_execfile ${binfile1} ${binfile} - - # Restart GDB entirely. - clean_restart ${binfile} - - # Set a breakpoint on foo and run to it. - gdb_test "break foo" \ - "Breakpoint.*at.* file .*$srcfile1, line 14.*" \ - "second pass: breakpoint foo in first file" - gdb_run_cmd - gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" "second pass: run to foo()" - - # This time, let the program run to completion. If GDB checks the - # executable file's timestamp now, it won't notice any change. - gdb_continue_to_end "second pass" - - # Now move the newer executable into place, and re-run. GDB - # should still notice that the executable file has changed, - # and still re-set the breakpoint appropriately. - gdb_rename_execfile ${binfile} ${binfile1} - gdb_rename_execfile ${binfile2} ${binfile} - gdb_run_cmd - gdb_test "" "Breakpoint.* foo .* at .*:9.*" "second pass: run to foo() second time" -} + # build the first test case + set testfile1 "reread1" + set srcfile1 ${testfile1}.c + # Cygwin needs $EXEEXT. + set binfile1 [standard_output_file ${testfile1}$EXEEXT] + + set testfile1_opt [list debug nowarnings \ + additional_flags=[lindex $opts 0] \ + [lindex $opts 1] ] + if { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}" \ + executable ${testfile1_opt}] != "" } { + untested "failed to compile first testcase" + return -1 + } + + # build the second test case + + set testfile2 "reread2" + set srcfile2 ${testfile2}.c + set binfile2 [standard_output_file ${testfile2}$EXEEXT] + + set testfile2_opt1 [list debug nowarnings \ + additional_flags=[lindex $opts 0] \ + [lindex $opts 1]] + set testfile2_op2 [list debug nowarnings \ + "additional_flags=-DNO_SECTIONS [lindex $opts 0]" \ + [lindex $opts 1]] + if { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" \ + executable ${testfile2_opt1}] != "" + && [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" \ + executable ${testfile2_opt2}] != ""} { + untested "failed to compile second testcase" + return -1 + } + + # Start with a fresh gdb. + + set testfile "reread" + set binfile [standard_output_file ${testfile}$EXEEXT] + + gdb_start + gdb_reinitialize_dir $srcdir/$subdir + + # Load the first executable. + + gdb_rename_execfile ${binfile1} ${binfile} + gdb_load ${binfile} + + # Set a breakpoint at foo + + gdb_test "break foo" \ + "Breakpoint.*at.* file .*$srcfile1, line 14.*" \ + "breakpoint foo in first file" + + + # Run, should see "Breakpoint 1, foo () at hello1.c:14" + + gdb_run_cmd + gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" "run to foo()" + + # Restore first executable to its original name, and move + # second executable into its place. Ensure that the new + # executable is at least a second newer than the old. + + gdb_rename_execfile ${binfile} ${binfile1} + gdb_rename_execfile ${binfile2} ${binfile} + gdb_test "shell sleep 1" ".*" "" + gdb_touch_execfile ${binfile} + + # Run a second time; GDB should detect that the executable has changed + # and reset the breakpoints correctly. + # Should see "Breakpoint 1, foo () at reread2.c:9" + + set test "run to foo() second time" + if [is_remote target] { + unsupported $test + } else { + gdb_run_cmd + gdb_test "" "Breakpoint.* foo .* at .*:9.*" $test + } + + + ### Second pass: verify that GDB checks the executable file's + ### timestamp when the program is *restarted*, not just when it exits. + + if [is_remote target] { + unsupported "second pass: GDB should check for changes before running" + } else { + + # Put the older executable back in place. + gdb_rename_execfile ${binfile} ${binfile2} + gdb_rename_execfile ${binfile1} ${binfile} + + # Restart GDB entirely. + clean_restart ${binfile} + + # Set a breakpoint on foo and run to it. + gdb_test "break foo" \ + "Breakpoint.*at.* file .*$srcfile1, line 14.*" \ + "second pass: breakpoint foo in first file" + gdb_run_cmd + gdb_test "" "Breakpoint.* foo .* at .*$srcfile1:14.*" \ + "second pass: run to foo()" + + # This time, let the program run to completion. If GDB checks the + # executable file's timestamp now, it won't notice any change. + gdb_continue_to_end "second pass" + + # Now move the newer executable into place, and re-run. GDB + # should still notice that the executable file has changed, + # and still re-set the breakpoint appropriately. + gdb_rename_execfile ${binfile} ${binfile1} + gdb_rename_execfile ${binfile2} ${binfile} + gdb_run_cmd + gdb_test "" "Breakpoint.* foo .* at .*:9.*" \ + "second pass: run to foo() second time" + } + + } # End of tests. return 0