diff mbox

Fix breakpoints on file reloads for PIE binaries

Message ID 20190603125344.40595-1-alan.hayward@arm.com
State New, archived
Headers

Commit Message

Alan Hayward June 3, 2019, 12:53 p.m. UTC 
  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:

2019-06-03  Alan Hayward  <alan.hayward@arm.com>

	* symfile.c (symbol_file_command): Call solib_create_inferior_hook.

gdb/testsuite/ChangeLog:

2019-06-03  Alan Hayward  <alan.hayward@arm.com>

	* gdb.base/break-idempotent.exp: Test both PIE and non PIE.
---
 gdb/symfile.c                               | 12 ++++
 gdb/testsuite/gdb.base/break-idempotent.exp | 72 ++++++++++++---------
 2 files changed, 52 insertions(+), 32 deletions(-)

-- 
2.20.1 (Apple Git-117)

Comments

Alan Hayward June 12, 2019, 10:04 a.m. UTC | #1 
Quick ping on this.


Thanks,
Alan.

> On 3 Jun 2019, at 13:53, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 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:
> 
> 2019-06-03  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* symfile.c (symbol_file_command): Call solib_create_inferior_hook.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-06-03  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* gdb.base/break-idempotent.exp: Test both PIE and non PIE.
> ---
> gdb/symfile.c                               | 12 ++++
> gdb/testsuite/gdb.base/break-idempotent.exp | 72 ++++++++++++---------
> 2 files changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index af99da18f7..0baed96e06 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 (0);
> +
> +      /* 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" } {
> +    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
> +	}
> +
> +	clean_restart $testfile
> +
> +	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
> +	    }
> +	}
> 
> -    if {![skip_hw_watchpoint_access_tests]
> -	&& ![skip_hw_watchpoint_multi_tests]} {
> -	test_break $always_inserted "rwatch"
> -	test_break $always_inserted "awatch"
> +	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"
> +	}
>     }
> }
> -- 
> 2.20.1 (Apple Git-117)
>
Alan Hayward July 4, 2019, 11:23 a.m. UTC | #2 
> On 12 Jun 2019, at 11:04, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> Quick ping on this.
> 
> 
> Thanks,
> Alan.
> 
>> On 3 Jun 2019, at 13:53, Alan Hayward <Alan.Hayward@arm.com> wrote:
>> 
>> 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:
>> 
>> 2019-06-03  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* symfile.c (symbol_file_command): Call solib_create_inferior_hook.
>> 
>> gdb/testsuite/ChangeLog:
>> 
>> 2019-06-03  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* gdb.base/break-idempotent.exp: Test both PIE and non PIE.
>> ---
>> gdb/symfile.c                               | 12 ++++
>> gdb/testsuite/gdb.base/break-idempotent.exp | 72 ++++++++++++---------
>> 2 files changed, 52 insertions(+), 32 deletions(-)
>> 
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index af99da18f7..0baed96e06 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 (0);
>> +
>> +      /* 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" } {
>> +    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
>> +	}
>> +
>> +	clean_restart $testfile
>> +
>> +	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
>> +	    }
>> +	}
>> 
>> -    if {![skip_hw_watchpoint_access_tests]
>> -	&& ![skip_hw_watchpoint_multi_tests]} {
>> -	test_break $always_inserted "rwatch"
>> -	test_break $always_inserted "awatch"
>> +	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"
>> +	}
>>    }
>> }
>> -- 
>> 2.20.1 (Apple Git-117)
>> 
>
diff mbox

Patch

diff --git a/gdb/symfile.c b/gdb/symfile.c
index af99da18f7..0baed96e06 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 (0);
+
+      /* 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" } {
+    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
+	}
+
+	clean_restart $testfile
+
+	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
+	    }
+	}
 
-    if {![skip_hw_watchpoint_access_tests]
-	&& ![skip_hw_watchpoint_multi_tests]} {
-	test_break $always_inserted "rwatch"
-	test_break $always_inserted "awatch"
+	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"
+	}
     }
 }