[PATCH] Fix breakpoints on file reloads for PIE binaries

Alan Hayward Alan.Hayward@arm.com
Mon Jul 8 09:52:00 GMT 2019



> On 5 Jul 2019, at 19:36, Pedro Alves <palves@redhat.com> wrote:
> 
> 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.


I really should have spotted most of those test case issues.

Thanks for the review and additional patch. I pushed them.



Alan.


> 
> This is OK otherwise.  Thanks.
> 
> From c0b167cee9903e6e68fbe899e6960bd94f285132 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> 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]} {
> -- 
> 2.14.5
> 



More information about the Gdb-patches mailing list