[binutils-gdb] Fix PR19548: Breakpoint re-set inserts breakpoints when it shouldn't

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2a7f3dffced7a20c992e1488d9f05fed8b8001fd

commit 2a7f3dffced7a20c992e1488d9f05fed8b8001fd
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Feb 9 12:12:17 2016 +0000

    Fix PR19548: Breakpoint re-set inserts breakpoints when it shouldn't
    
    PR19548 shows that we still have problems related to 13fd3ff34329:
    
     [PR17431: following execs with "breakpoint always-inserted on"]
     https://sourceware.org/ml/gdb-patches/2014-09/msg00733.html
    
    The problem this time is that we currently update the global location
    list and try to insert breakpoint locations after re-setting _each_
    breakpoint in turn.
    
    Say:
    
     - We have _more_ than one breakpoint set.  Let's assume 2.
    
     - There's a breakpoint with a pre-exec address that ends up being an
       unmapped address after the exec.
    
     - That breakpoint is NOT the first in the breakpoint list.
    
    Then when handling an exec, and we re-set the first breakpoint in the
    breakpoint list, we mistakently try to install the old pre-exec /
    un-re-set locations of the other breakpoint, which fails:
    
     (gdb) continue
     Continuing.
     process 28295 is executing new program: (...)/execl-update-breakpoints2
     Error in re-setting breakpoint 1: Warning:
     Cannot insert breakpoint 2.
     Cannot access memory at address 0x1000764
    
     Breakpoint 1, main (argc=1, argv=0x7fffffffd368) at /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/execl-update-breakpoints.c:34
     34        len = strlen (argv[0]);
     (gdb)
    
    Fix this by deferring the global location list update till after all
    breakpoints are re-set.
    
    Tested on x86_64 Fedora 20, native and gdbserver.
    
    gdb/ChangeLog:
    2016-02-09  Pedro Alves  <palves@redhat.com>
    
    	PR breakpoints/19548
    	* breakpoint.c (create_overlay_event_breakpoint): Don't update
    	global location list here.
    	(create_longjmp_master_breakpoint)
    	(create_std_terminate_master_breakpoint)
    	(create_exception_master_breakpoint, create_jit_event_breakpoint)
    	(update_breakpoint_locations):
    	(breakpoint_re_set): Update global location list after all
    	breakpoints are re-set.
    
    gdb/testsuite/ChangeLog:
    2016-02-09  Pedro Alves  <palves@redhat.com>
    
    	PR breakpoints/19548
    	* gdb.base/execl-update-breakpoints.c (some_function): New
    	function.
    	(main): Call it.
    	* gdb.base/execl-update-breakpoints.exp: Add a second breakpoint.
    	Tighten expected GDB output.

Diff:
---
 gdb/ChangeLog                                      | 12 +++++++++
 gdb/breakpoint.c                                   | 25 +++++++----------
 gdb/testsuite/ChangeLog                            |  9 +++++++
 gdb/testsuite/gdb.base/execl-update-breakpoints.c  |  6 +++++
 .../gdb.base/execl-update-breakpoints.exp          | 31 ++++++++++++++++++++--
 5 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6150f6c..8e6e4ec 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2016-02-09  Pedro Alves  <palves@redhat.com>
+
+	PR breakpoints/19548
+	* breakpoint.c (create_overlay_event_breakpoint): Don't update
+	global location list here.
+	(create_longjmp_master_breakpoint)
+	(create_std_terminate_master_breakpoint)
+	(create_exception_master_breakpoint, create_jit_event_breakpoint)
+	(update_breakpoint_locations):
+	(breakpoint_re_set): Update global location list after all
+	breakpoints are re-set.
+
 2016-02-08  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* remote.c (remote_register_number_and_offset): Remove unused
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index afd9065..c059861 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3488,7 +3488,6 @@ create_overlay_event_breakpoint (void)
          overlay_events_enabled = 0;
        }
     }
-  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 static void
@@ -3602,7 +3601,6 @@ create_longjmp_master_breakpoint (void)
 	}
     }
   }
-  update_global_location_list (UGLL_MAY_INSERT);
 
   do_cleanups (old_chain);
 }
@@ -3661,8 +3659,6 @@ create_std_terminate_master_breakpoint (void)
     }
   }
 
-  update_global_location_list (UGLL_MAY_INSERT);
-
   do_cleanups (old_chain);
 }
 
@@ -3766,8 +3762,6 @@ create_exception_master_breakpoint (void)
       b->location = new_explicit_location (&explicit_loc);
       b->enable_state = bp_disabled;
     }
-
-  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 void
@@ -7807,12 +7801,8 @@ struct lang_and_radix
 struct breakpoint *
 create_jit_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 {
-  struct breakpoint *b;
-
-  b = create_internal_breakpoint (gdbarch, address, bp_jit_event,
-				  &internal_breakpoint_ops);
-  update_global_location_list_nothrow (UGLL_MAY_INSERT);
-  return b;
+  return create_internal_breakpoint (gdbarch, address, bp_jit_event,
+				     &internal_breakpoint_ops);
 }
 
 /* Remove JIT code registration and unregistration breakpoint(s).  */
@@ -14278,7 +14268,6 @@ update_breakpoint_locations (struct breakpoint *b,
       /* Ranged breakpoints have only one start location and one end
 	 location.  */
       b->enable_state = bp_disabled;
-      update_global_location_list (UGLL_MAY_INSERT);
       printf_unfiltered (_("Could not reset ranged breakpoint %d: "
 			   "multiple locations found\n"),
 			 b->number);
@@ -14376,8 +14365,6 @@ update_breakpoint_locations (struct breakpoint *b,
 
   if (!locations_are_equal (existing_locations, b->loc))
     observer_notify_breakpoint_modified (b);
-
-  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 /* Find the SaL locations corresponding to the given LOCATION.
@@ -14617,6 +14604,11 @@ breakpoint_re_set (void)
   save_input_radix = input_radix;
   old_chain = save_current_space_and_thread ();
 
+  /* Note: we must not try to insert locations until after all
+     breakpoints have been re-set.  Otherwise, e.g., when re-setting
+     breakpoint 1, we'd insert the locations of breakpoint 2, which
+     hadn't been re-set yet, and thus may have stale locations.  */
+
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     /* Format possible error msg.  */
@@ -14637,6 +14629,9 @@ breakpoint_re_set (void)
   create_longjmp_master_breakpoint ();
   create_std_terminate_master_breakpoint ();
   create_exception_master_breakpoint ();
+
+  /* Now we can insert.  */
+  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 /* Reset the thread number of this breakpoint:
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 2a71930..f8e0800 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+2016-02-09  Pedro Alves  <palves@redhat.com>
+
+	PR breakpoints/19548
+	* gdb.base/execl-update-breakpoints.c (some_function): New
+	function.
+	(main): Call it.
+	* gdb.base/execl-update-breakpoints.exp: Add a second breakpoint.
+	Tighten expected GDB output.
+
 2016-02-08  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* Makefile.in (ALL_SUBDIRS): Remove.
diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.c b/gdb/testsuite/gdb.base/execl-update-breakpoints.c
index 3978a70..38f1cea 100644
--- a/gdb/testsuite/gdb.base/execl-update-breakpoints.c
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.c
@@ -20,6 +20,11 @@
 #include <stdlib.h>
 #include <string.h>
 
+void
+some_function (void)
+{
+}
+
 int
 main (int argc, char **argv)
 {
@@ -34,5 +39,6 @@ main (int argc, char **argv)
 
   execl (bin, bin, (char *) NULL);
   perror ("execl failed");
+  some_function ();
   exit (1);
 }
diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
index 4f35999..47a848e 100644
--- a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
@@ -96,6 +96,7 @@ if {!$cannot_access} {
 
 proc test { always_inserted } {
     global exec1
+    global gdb_prompt
 
     clean_restart ${exec1}
 
@@ -106,13 +107,39 @@ proc test { always_inserted } {
 	return -1
     }
 
-    # On a buggy GDB, with always-inserted on, we'd see:
+    # Set a second breakpoint (whose original address also ends up
+    # unmmapped after the exec), for PR 19548.
+    gdb_test "break some_function" "Breakpoint .*"
+
+    # PR17431: with always-inserted on, we'd see:
     #  (gdb) continue
     #  Continuing.
     #  Warning:
     #  Cannot insert breakpoint 1.
     #  Cannot access memory at address 0x10000ff
-    gdb_test "continue" "Breakpoint 1, main.*" "continue across exec"
+
+    # PR 19548: with more than one breakpoint, we'd see:
+    #  (gdb) continue
+    #  Continuing.
+    #  process (...) is executing new program: (...)/execl-update-breakpoints2
+    #  Error in re-setting breakpoint 1: Warning:
+    #  Cannot insert breakpoint 2.
+    #  Cannot access memory at address 0x1000764
+    set not_nl "\[^\r\n\]*"
+    set regex ""
+    append regex \
+	"^continue\r\n" \
+	"Continuing\\.\r\n" \
+	"${not_nl} is executing new program: ${not_nl}\r\n" \
+	"(Reading ${not_nl} from remote target\\.\\.\\.\r\n)*" \
+	"\r\n" \
+	"Breakpoint 1, main.*$gdb_prompt $"
+    set message "continue across exec"
+    gdb_test_multiple "continue" $message {
+	-re $regex {
+	    pass $message
+	}
+    }
 }
 
 foreach always_inserted { "off" "on" } {