[8.3,backport] Fix breakpoints on file reloads for PIE binaries
Commit Message
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.
Thanks,
- Tom
Comments
> 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"
> + }
> }
> }
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> OK to backport both patches to 8.3 branch? [ Attached only the modified
Tom> patch: "Fix breakpoints on file reloads for PIE binaries". ]
Tom> Build and reg-tested on x86_64-linux, no regressions.
It seems reasonable to me, so go ahead.
Thank you.
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(-)
@@ -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:
@@ -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 ();
}
}
@@ -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.
@@ -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"
+ }
}
}