Re: [8.3 backport] Fix breakpoints on file reloads for PIE binaries

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 4 Aug 2019, at 09:21, Tom de Vries <tdevries@suse.de> wrote:
> 
> On 08-07-19 11:52, Alan Hayward wrote:
>> Thanks for the review and additional patch. I pushed them.
> 
> Hi,
> 
> This patch applies cleanly to 8.3 branch, but in order to get the
> test-case to work I needed to:
> - drop the part of the patch that adds a type argument to send_gdb in
>  the test-case
> - backport commit 968aa7ae38 "Testsuite: Ensure pie is disabled on some
>  tests", in order to make the "pie" option for gdb_compile work.  This
>  patch applies cleanly.
> 
> OK to backport both patches to 8.3 branch? [ Attached only the modified
> patch: "Fix breakpoints on file reloads for PIE binaries". ]
> 
> Build and reg-tested on x86_64-linux, no regressions.

I don’t have the power to approve this, but the diff looks good to me and
I don’t see any reasons why back porting this could cause issues.


Alan.



> 
> Thanks,
> - Tom
> 
> Fix breakpoints on file reloads for PIE binaries
> 
> [ Backport of master commit ea142fbfc9. ]
> 
> When a binary is built using PIE, reloading the file will cause GDB to error
> on restart.  For example:
> gdb ./a.out
> (gdb) break main
> (gdb) run
> (gdb) file ./a.out
> (gdb) continue
> 
> Will cause GDB to error with:
> Continuing.
> Warning:
> Cannot insert breakpoint 1.
> Cannot access memory at address 0x9e0
> Command aborted.
> 
> This is due to the symbol offsets not being relocated after reloading the file.
> 
> Fix is to ensure solib_create_inferior_hook is called, in the same manner as
> infrun.c:follow_exec().
> 
> Expand the idempotent test to cover PIE scenarios.
> 
> gdb/ChangeLog:
> 
> 	* symfile.c (symbol_file_command): Call solib_create_inferior_hook.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/break-idempotent.exp: Test both PIE and non PIE.
> 
> ---
> gdb/ChangeLog                               |  4 ++
> gdb/symfile.c                               | 12 ++++++
> gdb/testsuite/ChangeLog                     |  4 ++
> gdb/testsuite/gdb.base/break-idempotent.exp | 62 ++++++++++++++++-------------
> 4 files changed, 54 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index f15cc5fcf0..f101a27be5 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-08-04  Alan Hayward  <alan.hayward@arm.com>
> +
> +	* symfile.c (symbol_file_command): Call solib_create_inferior_hook.
> +
> 2019-05-29  Tom Tromey  <tromey@adacore.com>
> 
> 	PR c++/20020:
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index bd79315687..a03ac29541 100644
> --- 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 (from_tty);
> +
> +      /* Now it's safe to re-add the breakpoints.  */
> +      breakpoint_re_set ();
>     }
> }
> 
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 03824fa2ae..db67e6ec0f 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-07-08  Alan Hayward  <alan.hayward@arm.com>
> +
> +	* gdb.base/break-idempotent.exp: Test both PIE and non PIE.
> +
> 2019-03-22  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* README: Add pie options.
> diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp
> index 902a5f818b..a8b4d92733 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.
> 
> @@ -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,43 @@ proc test_break { always_inserted break_command } {
>     }
> }
> 
> -foreach always_inserted { "off" "on" } {
> -    test_break $always_inserted "break"
> +# 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" } {
> +
> +    set opts {debug}
> +    lappend opts $pie
> 
> -    if {![skip_hw_breakpoint_tests]} {
> -	test_break $always_inserted "hbreak"
> +    set binfile [standard_output_file $testfile-$pie]
> +
> +    if {[prepare_for_testing "failed to prepare" $binfile $srcfile $opts]} {
> +	continue
>     }
> 
> -    if {![skip_hw_watchpoint_tests]} {
> -	test_break $always_inserted "watch"
> +    if [is_remote host] {
> +	set arg [remote_download host $binfile]
> +	if { $arg == "" } {
> +	    untested "download failed"
> +	    continue
> +	}
>     }
> 
> -    if {![skip_hw_watchpoint_access_tests]
> -	&& ![skip_hw_watchpoint_multi_tests]} {
> -	test_break $always_inserted "rwatch"
> -	test_break $always_inserted "awatch"
> +    foreach_with_prefix always_inserted { "off" "on" } {
> +	test_break $always_inserted "break"
> +
> +	if {![skip_hw_breakpoint_tests]} {
> +	    test_break $always_inserted "hbreak"
> +	}
> +
> +	if {![skip_hw_watchpoint_tests]} {
> +	    test_break $always_inserted "watch"
> +	}
> +
> +	if {![skip_hw_watchpoint_access_tests]
> +	    && ![skip_hw_watchpoint_multi_tests]} {
> +	    test_break $always_inserted "rwatch"
> +	    test_break $always_inserted "awatch"
> +	}
>     }
> }