Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2020-04-19 19:49:12 +0100]:

> On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote:
> 
> > 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.
> 
> So here's my proposed patch.
> 
> This makes breakpoint_locations_match consider the location's
> type too, like Keno's original patch.  But then does a bunch
> more stuff to actually make that work correctly.

LGTM.

Thanks,
Andrew


> 
> - We need to handle the duplicates detection better.  Take a look
>   at the loop at the tail end of update_global_location_list.
>   Currently, because breakpoint locations aren't sorted by type,
>   we can end up with, at the same address, a sw break, then a hw break,
>   then a sw break, etc.  The result is that that second sw break won't
>   be considered a duplicate of the first sw break.  Seems like
>   we already handle that incorrectly for range breakpoints.
> 
> - The "set breakpoint auto-hw" handling is moved out of insert_bp_location
>   to update_global_location_list, before the duplicates determination.
> 
>   This change comes with one visible behavior change with
>   "set breakpoint auto-hw off", however.  Here:
> 
>   Before:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    (gdb) c
>    Continuing.
>    Warning:
>    Cannot insert breakpoint 2.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>    Command aborted.
>    (gdb)
> 
>   After:
> 
>    (gdb) break *0x400487
>    Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
>    warning: Breakpoint 2.1 disabled.
>    Cannot set software breakpoint at read-only address 0x400487
> 
>   I.e., we now warn earlier, instead of waiting until resume time to error out.
> 
> - In update_breakpoint_locations, the logic of matching old locations
>   with new locations, in the have_ambiguous_names case, is updated
>   to still consider sw vs hw locations the same.
> 
> - I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any
>   needed updating.  Note that that macro walks all locations at
>   a given address, and doesn't call breakpoint_locations_match.
> 
> The result against GDBserver is this:
> 
>  (gdb) hbreak main
>  Sending packet: $m400700,40#28...Packet received: 89e58b0....
>  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: $Z0,400736,1#47...Packet received: OK
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
>  (gdb) del 1
>  Sending packet: $z1,400736,1#68...Packet received: OK
>  Sending packet: $Z0,400736,1#47...Packet received: OK
>  (gdb) 
> 
> Or, with "set breakpoint condition-evaluation host", 
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) b main
>  Note: breakpoint 3 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 4 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z0,400736,1#47...Packet received: OK
>  (gdb) del 3
>  Sending packet: $z1,400736,1#68...Packet received: OK
> 
> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 19 Apr 2020 18:25:55 +0100
> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
> 
> ---
>  gdb/breakpoint.c                                   | 269 ++++++++++++++-------
>  gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
>  2 files changed, 183 insertions(+), 88 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461ba..27799e89807 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
>  static int watchpoint_locations_match (struct bp_location *loc1,
>  				       struct bp_location *loc2);
>  
> +static int breakpoint_locations_match (struct bp_location *loc1,
> +				       struct bp_location *loc2,
> +				       bool sw_hw_bps_match = false);
> +
>  static int breakpoint_location_address_match (struct bp_location *bl,
>  					      const struct address_space *aspace,
>  					      CORE_ADDR addr);
> @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
>      return;
>  
>    /* Do a first pass to check for locations with no assigned
> -     conditions or conditions that fail to parse to a valid agent expression
> -     bytecode.  If any of these happen, then it's no use to send conditions
> -     to the target since this location will always trigger and generate a
> -     response back to GDB.  */
> +     conditions or conditions that fail to parse to a valid agent
> +     expression bytecode.  If any of these happen, then it's no use to
> +     send conditions to the target since this location will always
> +     trigger and generate a response back to GDB.  Note we consider
> +     all locations at the same address irrespective of type, i.e.,
> +     even if the locations aren't considered duplicates (e.g.,
> +     software breakpoint and hardware breakpoint at the same
> +     address).  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
>  	}
>      }
>  
> -  /* No NULL conditions or failed bytecode generation.  Build a condition list
> -     for this location's address.  */
> +  /* No NULL conditions or failed bytecode generation.  Build a
> +     condition list for this location's address.  If we have software
> +     and hardware locations at the same address, they aren't
> +     considered duplicates, but we still marge all the conditions
> +     anyway, as it's simpler, and doesn't really make a practical
> +     difference.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
>    if (dprintf_style != dprintf_style_agent)
>      return;
>  
> -  /* For now, if we have any duplicate location that isn't a dprintf,
> -     don't install the target-side commands, as that would make the
> -     breakpoint not be reported to the core, and we'd lose
> +  /* For now, if we have any location at the same address that isn't a
> +     dprintf, don't install the target-side commands, as that would
> +     make the breakpoint not be reported to the core, and we'd lose
>       control.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
> @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
>  	}
>      }
>  
> -  /* No NULL commands or failed bytecode generation.  Build a command list
> -     for this location's address.  */
> +  /* No NULL commands or failed bytecode generation.  Build a command
> +     list for all duplicate locations at this location's address.
> +     Note that here we must care for whether the breakpoint location
> +     types are considered duplicates, otherwise, say, if we have a
> +     software and hardware location at the same address, the target
> +     could end up running the commands twice.  For the moment, we only
> +     support targets-side commands with dprintf, but it doesn't hurt
> +     to be pedantically correct in case that changes.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> -      if (loc->owner->extra_string
> +      if (breakpoint_locations_match (bl, loc)
> +	  && loc->owner->extra_string
>  	  && is_breakpoint (loc->owner)
>  	  && loc->pspace->num == bl->pspace->num
>  	  && loc->owner->enable_state == bp_enabled
> @@ -2454,69 +2473,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 +2786,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);
>  }
>  
> @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1,
>  
>  /* Assuming LOC1 and LOC2's types' have meaningful target addresses
>     (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
> -   the same location.  */
> +   the same location.  If SW_HW_BPS_MATCH is true, then software
> +   breakpoint locations and hardware breakpoint locations match,
> +   otherwise they don't.  */
>  
>  static int
> -breakpoint_locations_match (struct bp_location *loc1, 
> -			    struct bp_location *loc2)
> +breakpoint_locations_match (struct bp_location *loc1,
> +			    struct bp_location *loc2,
> +			    bool sw_hw_bps_match)
>  {
>    int hw_point1, hw_point2;
>  
> @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1,
>    else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
>      return tracepoint_locations_match (loc1, loc2);
>    else
> -    /* We compare bp_location.length in order to cover ranged breakpoints.  */
> +    /* We compare bp_location.length in order to cover ranged
> +       breakpoints.  Keep this in sync with
> +       bp_location_is_less_than.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>  				     loc2->pspace->aspace, loc2->address)
> +	    && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
>  	    && loc1->length == loc2->length);
>  }
>  
> @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b)
>  }
>  \f
>  
> +/* Return the location number of LOC.  */
> +
> +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);
>  
> +/* Handle "set breakpoint auto-hw".
> +
> +   If the explicitly specified breakpoint type is not hardware
> +   breakpoint, check the memory map to see whether the breakpoint
> +   address is in read-only memory.
> +
> +   And then, if "set breakpoint auto-hw" is enabled:
> +
> +   - location type is not hardware breakpoint, memory is read-only.
> +   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.
> +
> +   However, if "set breakpoint auto-hw" is disabled, and memory is
> +   read-only, we warn and disable the breakpoint location.  */
> +
> +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)
> +	{
> +	  /* 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.  */
> +	  mem_region *mr = lookup_mem_region (bl->address);
> +
> +	  if (mr != nullptr)
> +	    {
> +	      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 bool said = false;
> +
> +		      bl->loc_type = new_type;
> +		      if (!said)
> +			{
> +			  fprintf_filtered (gdb_stdout,
> +					    _("Note: automatically using "
> +					      "hardware breakpoints for "
> +					      "read-only addresses.\n"));
> +			  said = true;
> +			}
> +		    }
> +		}
> +	      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 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
>    if (a->permanent != b->permanent)
>      return a->permanent > b->permanent;
>  
> +  /* Sort by type in order to make duplicate determination easier.
> +     See update_global_location_list.  This is kept in sync with
> +     breakpoint_locations_match.  */
> +  if (a->loc_type < b->loc_type)
> +    return true;
> +
> +  /* Likewise, for range-breakpoints, sort by length.  */
> +  if (a->loc_type == bp_loc_hardware_breakpoint
> +      && b->loc_type == bp_loc_hardware_breakpoint
> +      && a->length < b->length)
> +    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 +11694,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.
> @@ -11673,6 +11743,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);
>  
> @@ -11776,6 +11860,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
> @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  			  /* 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;
> @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b,
>  	    if (have_ambiguous_names)
>  	      {
>  		for (; l; l = l->next)
> -		  if (breakpoint_locations_match (e, l))
> -		    {
> -		      l->enabled = 0;
> -		      break;
> -		    }
> +		  {
> +		    /* Ignore software vs hardware location type at
> +		       this point, because with "set breakpoint
> +		       auto-hw", after a re-set, locations that were
> +		       hardware can end up as software, or vice versa.
> +		       As mentioned above, this is an heuristic and in
> +		       practice should give the correct answer often
> +		       enough.  */
> +		    if (breakpoint_locations_match (e, l, true))
> +		      {
> +			l->enabled = 0;
> +			break;
> +		      }
> +		  }
>  	      }
>  	    else
>  	      {
> diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> index 65f7baae9f5..b950e433ed8 100644
> --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
>  # Ensure that inserting a software breakpoint in a known-read-only
>  # region fails.
>  gdb_test "break *$main_lo" \
> -    "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
> +    "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
>      "inserting software breakpoint in read-only memory fails"
>  
>  delete_breakpoints
> 
> base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
> -- 
> 2.14.5
>