Re: [PATCH] Fix breakpoints on file reloads for PIE binaries

[Pedro Alves <palves at redhat dot com>]

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