Re: [PATCH] breakpoint: Make sure location types match before swapping

[Pedro Alves <palves@redhat.com>]

On 4/17/20 1:28 PM, Andrew Burgess wrote:
> @@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  			      gdb_assert (is_hardware_watchpoint (loc2->owner));
>  			      loc2->watchpoint_type = old_loc->watchpoint_type;
>  			    }
> +			  else if (is_breakpoint (old_loc->owner))
> +			    {
> +			      /* If we swap the inserted status of a
> +				 hardware and software breakpoint then GDB
> +				 will insert the breakpoint as one type,
> +				 and later try to remove the breakpoint as
> +				 the other type.  This is not good.  */
> +			      gdb_assert (is_breakpoint (loc2->owner));
> +			      if (old_loc->loc_type != loc2->loc_type)
> +				continue;
> +			    }

Unfortunately this doesn't actually work correctly.

For example:

 $ gdb ~/gdb/tests/threads -ex "set breakpoint always-inserted on" -ex "tar rem :9999" -ex "set debug remote 1"
 ...
 (gdb) hbreak main
 Sending packet: $m400700,40#28...Packet received:  89e58b...
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

Note no Z0 was sent after "b main".  That's because the sw and
hw breakpoints are still considered duplicates.

But now, let's delete the hw breakpoint:

(gdb) del 1
Sending packet: $z1,400736,1#68...Packet received: OK

Note z1 (remove hw break) was sent, but Z0 (insert sw break) was not.
At this point, the target side has no breakpoint installed!
That is very wrong, because we have "set breakpoint always-inserted on".
If the target was running at this point (e.g., non-stop mode),
even with "set breakpoint always-inserted off", GDB should keep
breakpoints installed, otherwise threads would miss the breakpoint.
"set breakpoint always-inserted on" is just a handy way to emulate
that.

If we do any other operation that forces installing breakpoints,
then finally the Z0 is sent:

 (gdb) b main
 Note: breakpoint 2 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 3 at 0x400736: file threads.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK
 (gdb) 

I spent a good while yesterday trying to make this all work
with GDB still considering hw breakpoints and sw breakpoints
as duplicates.

We also need to handle my concern about insert_bp_location
changing the type of breakpoint locations from sw breakpoint
to hw breakpoint, _after_ the update_global_location_list
sorted out which breakpoints are duplicates of which.

The result is that GDB behaves like this:

 (gdb) hbreak main
 Sending packet: $m400700,40#28...Packet received: 89e58b...
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) b main
 Note: breakpoint 1 also set at pc 0x400736.
 Sending packet: $m400736,1#fe...Packet received: 48
 Breakpoint 2 at 0x400736: file threads.c, line 57.
 Sending packet: $Z1,400736,1#48...Packet received: OK

 (gdb) del 1
 Sending packet: $Z0,400736,1#47...Packet received: OK
 Sending packet: $z1,400736,1#68...Packet received: OK
 Sending packet: $Z0,400736,1#47...Packet received: OK

Note how after deleting breakpoint 1 (the hw break),
GDB first installs the sw breakpoint, and then deletes
the hw breakpoint.  This order is important to ensures that
at no moment could a thread miss breakpoint 2.  The final
Z0, is an artifact of the target-side condition handling.
We could get rid of that but I didn't try it.

It was only after all the effort that I realized that it's
pointless to try to make hw and sw breakpoint locations
match _if we always need to install the new sw break before
deleting the hw break_.  I mean, we go through all that
effort, but then there's always going to be a small window
where the remote target needs to be able to handle
a sw breakpoint and a hw breakpoint at the same address
anyway (e.g., handle target-side breakpoint conditions
correctly, handle target-side commands correctly, etc.)

So the patch below is the one I wrote yesterday.  It
wasn't finished, but was well advanced.  I'm posting
it for the archives and for discussion.  At this point,
I don't believe we should follow this patch's approach.
I'll send another reply with today's new patch.

From e2fe5095cc2e136dadbec373ebaed2938e282145 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 18 Apr 2020 23:28:46 +0100
Subject: [PATCH] Handle hw and sw breakpoints at the same address

---
 gdb/breakpoint.c | 314 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 236 insertions(+), 78 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..25ce3b9d217 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2405,6 +2405,8 @@ breakpoint_kind (struct bp_location *bl, CORE_ADDR *addr)
     return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
 }
 
+static void handle_automatic_hardware_breakpoints (bp_location *bl);
+
 /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
    location.  Any error messages are printed to TMP_ERROR_STREAM; and
    DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -2454,69 +2456,6 @@ insert_bp_location (struct bp_location *bl,
   if (bl->loc_type == bp_loc_software_breakpoint
       || bl->loc_type == bp_loc_hardware_breakpoint)
     {
-      if (bl->owner->type != bp_hardware_breakpoint)
-	{
-	  /* If the explicitly specified breakpoint type
-	     is not hardware breakpoint, check the memory map to see
-	     if the breakpoint address is in read only memory or not.
-
-	     Two important cases are:
-	     - location type is not hardware breakpoint, memory
-	     is readonly.  We change the type of the location to
-	     hardware breakpoint.
-	     - location type is hardware breakpoint, memory is
-	     read-write.  This means we've previously made the
-	     location hardware one, but then the memory map changed,
-	     so we undo.
-	     
-	     When breakpoints are removed, remove_breakpoints will use
-	     location types we've just set here, the only possible
-	     problem is that memory map has changed during running
-	     program, but it's not going to work anyway with current
-	     gdb.  */
-	  struct mem_region *mr 
-	    = lookup_mem_region (bl->target_info.reqstd_address);
-	  
-	  if (mr)
-	    {
-	      if (automatic_hardware_breakpoints)
-		{
-		  enum bp_loc_type new_type;
-		  
-		  if (mr->attrib.mode != MEM_RW)
-		    new_type = bp_loc_hardware_breakpoint;
-		  else 
-		    new_type = bp_loc_software_breakpoint;
-		  
-		  if (new_type != bl->loc_type)
-		    {
-		      static int said = 0;
-
-		      bl->loc_type = new_type;
-		      if (!said)
-			{
-			  fprintf_filtered (gdb_stdout,
-					    _("Note: automatically using "
-					      "hardware breakpoints for "
-					      "read-only addresses.\n"));
-			  said = 1;
-			}
-		    }
-		}
-	      else if (bl->loc_type == bp_loc_software_breakpoint
-		       && mr->attrib.mode != MEM_RW)
-		{
-		  fprintf_unfiltered (tmp_error_stream,
-				      _("Cannot insert breakpoint %d.\n"
-					"Cannot set software breakpoint "
-					"at read-only address %s\n"),
-				      bl->owner->number,
-				      paddress (bl->gdbarch, bl->address));
-		  return 1;
-		}
-	    }
-	}
-        
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
 	  || bl->section == NULL
@@ -2830,7 +2769,11 @@ insert_breakpoints (void)
 
   /* Updating watchpoints creates new locations, so update the global
      location list.  Explicitly tell ugll to insert locations and
-     ignore breakpoints_always_inserted_mode.  */
+     ignore breakpoints_always_inserted_mode.  Also,
+     update_global_location_list tries to "upgrade" software
+     breakpoints to hardware breakpoints to handle "set breakpoint
+     auto-hw", so we need to call it even if we don't have new
+     locations.  */
   update_global_location_list (UGLL_INSERT);
 }
 
@@ -2905,6 +2848,22 @@ update_inserted_breakpoint_locations (void)
     }
 }
 
+static void
+breakpoint_error_stream (string_file *tmp_error_stream,
+			 int *hw_breakpoint_error,
+			 int *hw_bp_error_explained_already)
+{
+  /* If a hardware breakpoint or watchpoint was inserted, add a
+     message about possibly exhausted resources.  */
+  if (*hw_breakpoint_error && !*hw_bp_error_explained_already)
+    {
+      tmp_error_stream->printf ("Could not insert hardware breakpoints:\n\
+You may have requested too many hardware breakpoints/watchpoints.\n");
+    }
+  target_terminal::ours_for_output ();
+  error_stream (*tmp_error_stream);
+}
+
 /* Used when starting or continuing the program.  */
 
 static void
@@ -2991,17 +2950,9 @@ insert_breakpoint_locations (void)
     }
 
   if (error_flag)
-    {
-      /* If a hardware breakpoint or watchpoint was inserted, add a
-         message about possibly exhausted resources.  */
-      if (hw_breakpoint_error && !hw_bp_error_explained_already)
-	{
-	  tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\
-You may have requested too many hardware breakpoints/watchpoints.\n");
-	}
-      target_terminal::ours_for_output ();
-      error_stream (tmp_error_stream);
-    }
+    breakpoint_error_stream (&tmp_error_stream,
+			     &hw_breakpoint_error,
+			     &hw_bp_error_explained_already);
 }
 
 /* Used when the program stops.
@@ -8515,8 +8466,93 @@ mention (struct breakpoint *b)
 }
 \f
 
+static int
+bp_location_number (bp_location *loc)
+{
+  int n = 1;
+  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
+    {
+      if (l == loc)
+	return n;
+      ++n;
+    }
+
+  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
+}
+
 static bool bp_loc_is_permanent (struct bp_location *loc);
 
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
+    {
+      if (bl->owner->type != bp_hardware_breakpoint)
+	{
+	  /* If the explicitly specified breakpoint type is not
+	     hardware breakpoint, check the memory map to see if the
+	     breakpoint address is in read only memory or not.
+
+	     Two important cases are:
+	     - location type is not hardware breakpoint, memory is
+	     readonly.  We change the type of the location to hardware
+	     breakpoint.
+	     - location type is hardware breakpoint, memory is
+	     read-write.  This means we've previously made the
+	     location hardware one, but then the memory map changed,
+	     so we undo.
+
+	     When breakpoints are removed, remove_breakpoints will use
+	     location types we've just set here, the only possible
+	     problem is that memory map has changed during running
+	     program, but it's not going to work anyway with current
+	     gdb.  */
+	  struct mem_region *mr
+	    = lookup_mem_region (bl->target_info.reqstd_address);
+
+	  if (mr)
+	    {
+	      if (automatic_hardware_breakpoints)
+		{
+		  enum bp_loc_type new_type;
+
+		  if (mr->attrib.mode != MEM_RW)
+		    new_type = bp_loc_hardware_breakpoint;
+		  else
+		    new_type = bp_loc_software_breakpoint;
+
+		  if (new_type != bl->loc_type)
+		    {
+		      static int said = 0;
+
+		      bl->loc_type = new_type;
+		      if (!said)
+			{
+			  fprintf_filtered (gdb_stdout,
+					    _("Note: automatically using "
+					      "hardware breakpoints for "
+					      "read-only addresses.\n"));
+			  said = 1;
+			}
+		    }
+		}
+	      else if (bl->loc_type == bp_loc_software_breakpoint
+		       && mr->attrib.mode != MEM_RW)
+		{
+		  bl->enabled = false;
+
+		  warning (_("Breakpoint %d.%d disabled.\n"
+			    "Cannot set software breakpoint "
+			    "at read-only address %s\n"),
+			   bl->owner->number, bp_location_number (bl),
+			   paddress (bl->gdbarch, bl->address));
+		}
+	    }
+	}
+    }
+}
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -11451,6 +11487,29 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
   if (a->permanent != b->permanent)
     return a->permanent > b->permanent;
 
+  /* Sort hardware breakpoint locations before software breakpoint
+     locations, in order to prefer inserting hardware breakpoint
+     locations.  */
+  auto type_order = [](bp_loc_type loc_type)
+    {
+      switch (loc_type)
+	{
+	case bp_loc_hardware_breakpoint:
+	return 0;
+	case bp_loc_software_breakpoint:
+	return 1;
+	case bp_loc_hardware_watchpoint:
+	return 2;
+	case bp_loc_other:
+	return 3;
+	}
+
+      gdb_assert_not_reached ("bad switch");
+    };
+
+  if (type_order (a->loc_type) < type_order (b->loc_type))
+    return true;
+
   /* Make the internal GDB representation stable across GDB runs
      where A and B memory inside GDB can differ.  Breakpoint locations of
      the same type at the same address can be sorted in arbitrary order.  */
@@ -11625,6 +11684,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
       loc->cond_bytecode.reset ();
     }
 }
+
 /* Called whether new breakpoints are created, or existing breakpoints
    deleted, to update the global location list and recompute which
    locations are duplicate of which.
@@ -11654,6 +11714,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   struct bp_location *awp_loc_first; /* access watchpoint */
   struct bp_location *rwp_loc_first; /* read watchpoint */
 
+  /* Used for the case we need to insert a sw breakpoint to replace a
+     hw breakpoint, and vice versa.  */
+  bool error_flag = false;
+  int disabled_breaks = 0;
+  int hw_breakpoint_error = 0;
+  int hw_bp_error_explained_already = 0;
+  string_file tmp_error_stream;
+  /* Explicitly mark the warning -- this will only
+     be printed if there was an error.  */
+  tmp_error_stream.puts ("Warning:\n");
+
   /* Saved former bp_locations array which we compare against the newly
      built bp_locations from the current state of ALL_BREAKPOINTS.  */
   struct bp_location **old_locp;
@@ -11673,6 +11744,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
   ALL_BREAKPOINTS (b)
     for (loc = b->loc; loc; loc = loc->next)
       *locp++ = loc;
+
+  /* See if we need to "upgrade" a software breakpoint to a hardware
+     breakpoint.  Do this before deciding whether locations are
+     duplicates.  Also do this before sorting because sorting order
+     depends on location type.  */
+  for (locp = bp_locations;
+       locp < bp_locations + bp_locations_count;
+       locp++)
+    {
+      loc = *locp;
+      if (!loc->inserted)
+	handle_automatic_hardware_breakpoints (loc);
+    }
+
   std::sort (bp_locations, bp_locations + bp_locations_count,
 	     bp_location_is_less_than);
 
@@ -11769,6 +11854,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	      /* OLD_LOC comes from existing struct breakpoint.  */
 	      if (bl_address_is_meaningful (old_loc))
 		{
+		  bp_location *replacement_loc = nullptr;
+
 		  for (loc2p = locp;
 		       (loc2p < bp_locations + bp_locations_count
 			&& (*loc2p)->address == old_loc->address);
@@ -11776,6 +11863,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
+		      if (loc2 == old_loc)
+			continue;
+
 		      if (breakpoint_locations_match (loc2, old_loc))
 			{
 			  /* Read watchpoint locations are switched to
@@ -11786,12 +11876,25 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			      gdb_assert (is_hardware_watchpoint (loc2->owner));
 			      loc2->watchpoint_type = old_loc->watchpoint_type;
 			    }
+			  else if ((old_loc->loc_type == bp_loc_hardware_breakpoint
+				    || old_loc->loc_type == bp_loc_software_breakpoint)
+				   && loc2->loc_type != old_loc->loc_type)
+			    {
+			      /* A hardware breakpoint is being
+				 removed.  Don't consider software
+				 breakpoint locations as duplicates,
+				 to avoid consuming hardware resources
+				 unnecessarily without user
+				 request.  */
+			      if (unduplicated_should_be_inserted (loc2))
+				replacement_loc = loc2;
+			      continue;
+			    }
 
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be
 			     unduplicated.  */
-			  if (loc2 != old_loc
-			      && unduplicated_should_be_inserted (loc2))
+			  if (unduplicated_should_be_inserted (loc2))
 			    {
 			      swap_insertion (old_loc, loc2);
 			      keep_in_target = 1;
@@ -11799,6 +11902,29 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			    }
 			}
 		    }
+
+		  /* If we're deleting a harware breakpoint, and we
+		     have a replacement software breakpoint to install
+		     (or vice versa), we must install the new one
+		     before deleting the old one, so that the inferior
+		     doesn't miss a breakpoint in case it is running
+		     right now.  */
+		  if (!keep_in_target && replacement_loc != nullptr)
+		    {
+		      scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
+		      switch_to_program_space_and_thread (replacement_loc->pspace);
+
+		      replacement_loc->duplicate = false;
+
+		      int val
+			= insert_bp_location (replacement_loc, &tmp_error_stream,
+					      &disabled_breaks,
+					      &hw_breakpoint_error,
+					      &hw_bp_error_explained_already);
+		      if (val)
+			error_flag = val;
+		    }
 		}
 	    }
 
@@ -11959,13 +12085,45 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	 is not duplicated, and is the inserted one.
 	 All following are marked as duplicated, and are not inserted.  */
       if (loc->inserted)
-	swap_insertion (loc, *loc_first_p);
+	{
+	  if (loc->loc_type == bp_loc_software_breakpoint
+	      && (*loc_first_p)->loc_type == bp_loc_hardware_breakpoint)
+	    {
+	      /* Insert the replacement breakpoint first, so that
+		 there's never a time the inferior could miss a
+		 breakpoint, in case the inferior is running right
+		 now.  */
+	      scoped_restore_current_pspace_and_thread restore_pspace_thread;
+
+	      switch_to_program_space_and_thread ((*loc_first_p)->pspace);
+
+	      int val
+		= insert_bp_location ((*loc_first_p), &tmp_error_stream,
+				      &disabled_breaks,
+				      &hw_breakpoint_error,
+				      &hw_bp_error_explained_already);
+	      if (val)
+		error_flag = val;
+
+	      remove_breakpoint (loc);
+	    }
+	  else
+	    swap_insertion (loc, *loc_first_p);
+	}
       loc->duplicate = 1;
 
       /* Clear the condition modification flag.  */
       loc->condition_changed = condition_unchanged;
     }
 
+  /* Should this be propagated down to insert_breakpoint_locations if
+     it's going to be called, so that we have a single place that
+     throws?  */
+  if (error_flag)
+    breakpoint_error_stream (&tmp_error_stream,
+			     &hw_breakpoint_error,
+			     &hw_bp_error_explained_already);
+
   if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)

base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
-- 
2.14.5