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

Message ID de9d0e43-f43c-7ce8-0163-8bbadcc14401@redhat.com
State New
Headers show
Series
  • Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)
Related show

Commit Message

Rogerio Alves via Gdb-patches April 19, 2020, 6:49 p.m.
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.

- 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(-)


base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
-- 
2.14.5

Comments

Andrew Burgess April 20, 2020, 9:02 a.m. | #1
* 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)

>  }

>  

>  

> +/* 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

>
Rogerio Alves via Gdb-patches April 21, 2020, 4:24 p.m. | #2
On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> 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);

> +


Should this return a bool?

>  static int breakpoint_location_address_match (struct bp_location *bl,

>                                               const struct address_space *aspace,

>                                               CORE_ADDR addr);


And this, I guess.

> @@ -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)

>  }

>

>

> +/* 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

>
Rogerio Alves via Gdb-patches April 21, 2020, 6:31 p.m. | #3
On 4/21/20 5:24 PM, Christian Biesinger via Gdb-patches wrote:
> On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches

> <gdb-patches@sourceware.org> wrote:

>> 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);

>> +

> 

> Should this return a bool?

> 

>>  static int breakpoint_location_address_match (struct bp_location *bl,

>>                                               const struct address_space *aspace,

>>                                               CORE_ADDR addr);

> 

> And this, I guess.


Yeah, they should.  Old, pre-C++ code.
It feels like an orthogonal change though, particularly since I think we
could change all of those two, plus watchpoint_locations_match at the
same time.

Thanks,
Pedro Alves

Patch

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)
 }
 
 
+/* 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