[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