Fix breakpoints on file reloads for PIE binaries

Message ID 20190603125344.40595-1-alan.hayward@arm.com
State New
Headers show
Series
  • Fix breakpoints on file reloads for PIE binaries
Related show

Commit Message

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

>> 

>
Pedro Alves July 5, 2019, 6:36 p.m. | #3
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.

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
Alan Hayward July 8, 2019, 9:52 a.m. | #4
> 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

>

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"
+	}
     }
 }