From patchwork Fri Jul 5 18:36:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 33610 Received: (qmail 18143 invoked by alias); 5 Jul 2019 18:36:09 -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 18134 invoked by uid 89); 5 Jul 2019 18:36:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=HX-Received:600c, rebuild, H*RU:209.85.128.65, 14631 X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 05 Jul 2019 18:36:08 +0000 Received: by mail-wm1-f65.google.com with SMTP id h19so8142277wme.0 for ; Fri, 05 Jul 2019 11:36:07 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4c97:6d52:2cea:997b? ([2001:8a0:f913:f700:4c97:6d52:2cea:997b]) by smtp.gmail.com with ESMTPSA id j16sm6105005wrt.88.2019.07.05.11.36.04 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Fri, 05 Jul 2019 11:36:05 -0700 (PDT) Subject: Re: [PATCH] Fix breakpoints on file reloads for PIE binaries To: Alan Hayward , "gdb-patches@sourceware.org" References: <20190603125344.40595-1-alan.hayward@arm.com> Cc: nd From: Pedro Alves Message-ID: <1a87019f-b53e-5556-08bc-0ec373c26062@redhat.com> Date: Fri, 5 Jul 2019 19:36:04 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190603125344.40595-1-alan.hayward@arm.com> On 6/3/19 1:53 PM, Alan Hayward wrote: > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -1672,7 +1672,19 @@ symbol_file_command (const char *args, int from_tty) > > validate_readnow_readnever (flags); > > + /* Set SYMFILE_DEFER_BP_RESET because the proper displacement for a PIE > + (Position Independent Executable) main symbol file will only be > + computed by the solib_create_inferior_hook below. Without it, > + breakpoint_re_set would fail to insert the breakpoints with the zero > + displacement. */ > + add_flags |= SYMFILE_DEFER_BP_RESET; > + > symbol_file_add_main_1 (name, add_flags, flags, offset); > + > + solib_create_inferior_hook (0); solib_create_inferior_hook's parameter is "from_tty", so you should pass it down instead of hardcoding false. > + > + /* Now it's safe to re-add the breakpoints. */ > + breakpoint_re_set (); > } > } > > diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp > index 902a5f818b..38e7cf4710 100644 > --- a/gdb/testsuite/gdb.base/break-idempotent.exp > +++ b/gdb/testsuite/gdb.base/break-idempotent.exp > @@ -36,23 +36,6 @@ > > standard_testfile > > -if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { > - return -1 > -} > - > -if ![runto_main] then { > - fail "can't run to main" > - return 0 > -} > - > -if [is_remote host] { > - set arg [remote_download host $binfile] > - if { $arg == "" } { > - perror "download failed" > - return -1 > - } > -} > - > # Force a breakpoint re-set in GDB. Currently this is done by > # reloading symbols with the "file" command. > > @@ -62,11 +45,11 @@ proc force_breakpoint_re_set {} { > set test "file \$binfile" > gdb_test_multiple "file $binfile" $test { > -re "Are you sure you want to change the file. .*y or n. $" { > - send_gdb "y\n" > + send_gdb "y\n" optional > exp_continue > } > -re "Load new symbol table from \".*\".*y or n. $" { > - send_gdb "y\n" > + send_gdb "y\n" optional > exp_continue > } > -re "Reading symbols from.*$gdb_prompt $" { > @@ -123,7 +106,7 @@ proc set_breakpoint { break_command } { > proc test_break { always_inserted break_command } { > set cmd [lindex [split "$break_command"] 0] > > - with_test_prefix "always-inserted $always_inserted: $cmd" { > + with_test_prefix "$cmd" { > delete_breakpoints > > if ![runto_main] then { > @@ -163,20 +146,45 @@ proc test_break { always_inserted break_command } { > } > } > > -foreach always_inserted { "off" "on" } { > - test_break $always_inserted "break" > +foreach_with_prefix pie { "nopie" "pie" } { It's not obvious from reading the testcase alone why we do this nopie/pie. I think this warrants a comment. > + foreach_with_prefix always_inserted { "off" "on" } { > > - if {![skip_hw_breakpoint_tests]} { > - test_break $always_inserted "hbreak" > - } > + set opts {debug} > + lappend opts $pie > > - if {![skip_hw_watchpoint_tests]} { > - test_break $always_inserted "watch" > - } > + if {[prepare_for_testing "failed to prepare" $testfile $srcfile $opts]} { > + return -1 > + } There's no need to rebuild for the always_inserted off/on variants. I.e., you can compile twice instead of four times. Also, to reproduce problems, it's better when each build generates its own separate binary, instead of the last iteration overwriting the previous iteration's file. > + > + clean_restart $testfile prepare_for_testing already does this. > + > + if ![runto_main] then { > + fail "can't run to main" > + return 0 > + } This isn't really necessary since test_break calls runto_main too. > + > + if [is_remote host] { > + set arg [remote_download host $binfile] > + if { $arg == "" } { > + perror "download failed" > + return -1 > + } > + } Pedantically, these errors shouldn't cause the whole testcase to return, but instead should only cause the current iteration to be skipped. So e.g., on a system that fails to build nopie, we'd still test pie. In principle. Here's a patch you can merge with yours that addresses the issues above. This is OK otherwise. Thanks. From c0b167cee9903e6e68fbe899e6960bd94f285132 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 5 Jul 2019 19:16:06 +0100 Subject: [PATCH] fixes --- gdb/symfile.c | 2 +- gdb/testsuite/gdb.base/break-idempotent.exp | 34 ++++++++++++++--------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/gdb/symfile.c b/gdb/symfile.c index 0d353b47e57..dc6bdf3fd4a 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1681,7 +1681,7 @@ symbol_file_command (const char *args, int from_tty) symbol_file_add_main_1 (name, add_flags, flags, offset); - solib_create_inferior_hook (0); + solib_create_inferior_hook (from_tty); /* Now it's safe to re-add the breakpoints. */ breakpoint_re_set (); diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp index 38e7cf4710e..96f91c50f90 100644 --- a/gdb/testsuite/gdb.base/break-idempotent.exp +++ b/gdb/testsuite/gdb.base/break-idempotent.exp @@ -146,31 +146,29 @@ proc test_break { always_inserted break_command } { } } +# The testcase uses the "file" command to force breakpoint re-set in +# GDB. Test both with and without PIE, as GDB used to mishandle +# breakpoint re-set when reloading PIEs. foreach_with_prefix pie { "nopie" "pie" } { - foreach_with_prefix always_inserted { "off" "on" } { - - set opts {debug} - lappend opts $pie - if {[prepare_for_testing "failed to prepare" $testfile $srcfile $opts]} { - return -1 - } + set opts {debug} + lappend opts $pie - clean_restart $testfile + set binfile [standard_output_file $testfile-$pie] - if ![runto_main] then { - fail "can't run to main" - return 0 - } + if {[prepare_for_testing "failed to prepare" $binfile $srcfile $opts]} { + continue + } - if [is_remote host] { - set arg [remote_download host $binfile] - if { $arg == "" } { - perror "download failed" - return -1 - } + if [is_remote host] { + set arg [remote_download host $binfile] + if { $arg == "" } { + untested "download failed" + continue } + } + foreach_with_prefix always_inserted { "off" "on" } { test_break $always_inserted "break" if {![skip_hw_breakpoint_tests]} {