[PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741)

[Pedro Alves <palves@redhat.com>]

On 4/19/20 7:49 PM, Pedro Alves via Gdb-patches wrote:

> 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.

While looking at the test_single_step part of 
gdb.base/breapoint-in-ro-region.exp, I realized that the change mentioned
above isn't correct as is.  The problem is that we would be warning and
disabling the location, but if that location belongs to e.g., a software
single-step location, GDB would end up not installing the
location (because it was disabled), and then the inferior would be 
resumed, and GDB would lose control of the inferior, as it run free
with no software single-step breakpoint installed.

So instead I've moved the "set breakpoint auto-hw ON" handling to
update_global_location_list, and left the "set breakpoint auto-hw OFF"
where it was, in insert_bp_location.  This means there's no longer
a user-visible change.

I have not tested this on a software single-step target (I don't
think I have such a target readily handy), but I believe that this
new version will pass there, and the previous version would fail.

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

This here missed checking should_be_inserted -- there's no
point in calling handle_automatic_hardware_breakpoints for
disabled locations.  It no longer makes that much of a 
difference with the "set breakpoint auto-hw off" warning
removed from handle_automatic_hardware_breakpoints, but it
doesn't hurt to be correct.

I've also realized that before:

 commit 7c16b83e0521a007e4d86fc30e334b41b01668b4
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Wed Oct 15 20:18:31 2014 +0100

    Put single-step breakpoints on the bp_location chain

we were already inserting hw breakpoints and single-step
breakpoints at the same address.  But not hw breakpoints
and regular software breakpoints, though.  So it was
still broken, but I thought that was interesting in the
sense that it just seems like the hw-and-sw-breakpoints
at the same address case just wasn't thought about much
in the early days.

I've added a new testcase too now.  It fails against 
gdbserver on current master, passes with the fix.

I've also now updated the git commit log and write ChangeLog entries.

WDYT of this version?

From 0a08d49d0aa0cdfb1e6f4da044df2084d3403dc3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 2 May 2020 21:04:00 +0100
Subject: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates
 (PR gdb/25741)

In the following conditions:

  - A target with hardware breakpoints available, and
  - A target that uses software single stepping,
  - An instruction at ADDRESS loops back to itself,

Now consider the following steps:

  1. The user places a hardware breakpoint at ADDRESS (an instruction
  that loops to itself),

  2. The inferior runs and hits the breakpoint at ADDRESS,

  3. The user tells GDB to 'continue'.

In #3 when the user tells GDB to continue, GDB first disables the
hardware breakpoint at ADDRESS, and then inserts a software
single-step breakpoint at ADDRESS.  The original user-created
breakpoint was a hardware breakpoint, while the single-step breakpoint
will be a software breakpoint.

GDB continues and immediately hits the software single-step
breakpoint.

GDB then deletes the software single-step breakpoint by calling
delete_single_step_breakpoints, which eventually calls
delete_breakpoint, which, once the breakpoint (and its locations) are
deleted, calls update_global_location_list.

During update_global_location_list GDB spots that we have an old
location (the software single step breakpoint location) that is
inserted, but being deleted, and a location (the original hardware
breakpoint) at the same address which we are keeping, but which is not
currently inserted, GDB then calls breakpoint_locations_match on these
two locations.

Currently the locations do match, and so GDB calls swap_insertion
which swaps the "inserted" state of the two locations.  The user
created hardware breakpoint is marked as inserted, while the GDB
internal software single step breakpoint is now marked as not
inserted.  After this GDB returns through the call stack and leaves
delete_single_step_breakpoints.

After this GDB continues with its normal "stopping" process, as part
of this stopping process GDB removes all the breakpoints from the
target.  Due to the swap it is now the user-created hardware
breakpoint that is marked as inserted, so it is this breakpoint GDB
tries to remove.

The problem is that GDB inserted the software single-step breakpoint
as a software breakpoint, but is now trying to remove the hardware
breakpoint.  The problem is removing a software breakpoint is very
different to removing a hardware breakpoint, this could result is some
undetected undefined behaviour, or as in the original bug report (PR
gdb/25741), could result in the target throwing an error.

With "set breakpoint always-inserted on", we can easily reproduce this
against GDBserver.  E.g.:

  (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
  Packet Z1 (hardware-breakpoint) is supported

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

  (gdb) del
  Delete all breakpoints? (y or n) y
  Sending packet: $z0,400736,1#67...Packet received: E01
  warning: Error removing breakpoint 2

This patch adds a testcase that does exactly that.

Trying to enhance GDB to handle this scenario while continuing to
avoid inserting redundant software and hardware breakpoints at the
same address turns out futile, because, given non-stop and breakpoints
always-inserted, if the user:

 #1 - inserts a hw breakpoint, then
 #2 - inserts a sw breakpoint at the same address, and then
 #3 - removes the original hw breakpoint,

GDB would have to make sure to insert the sw breakpoint before
removing the hw breakpoint, to avoid running threads missing the
breakpoint.  I.e., there's always going to be a window where a target
needs to be able to handle both sw and a hw breakpoints installed at
the same address.  You can see more detailed description of that issue
here:
https://sourceware.org/pipermail/gdb-patches/2020-April/167738.html

So the fix here is to just stop considering software breakpoints and
hw breakpoints duplicates, and let GDB insert sw and hw breakpoints at
the same address.

The central change is to make breakpoint_locations_match consider the
location's type too.  There are several other changes necessary to
actually make that that work correctly, however:

- 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 on" handling is moved out of
  insert_bp_location to update_global_location_list, before the
  duplicates determination.

  Moving "set breakpoint auto-hw off" handling as well and downgrading
  it to a warning+'disabling the location' was considered too, but in
  the end discarded, because we want to error out with internal and
  momentary breakpoints, like software single-step breakpoints.
  Disabling such locations at update_global_location_list time would
  make GDB lose control of the inferior.

- 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.

- Review all ALL_BP_LOCATIONS_AT_ADDR uses, and update those that
  might need to be updated, and update comments for those that don't.
  Note that that macro walks all locations at a given address, and
  doesn't call breakpoint_locations_match.

The result against GDBserver (with "set breakpoint
condition-evaluation host" to avoid seeing confusing reinsertions) is:

 (gdb) hbreak main
 Sending packet: $m400736,1#fe...Packet received: 48
 Hardware assisted breakpoint 1 at 0x400736: file main.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 4 at 0x400736: file main.c, line 57.
 Sending packet: $Z0,400736,1#47...Packet received: OK

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

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
	    Andrew Burgess  <andrew.burgess@embecosm.com>
	    Keno Fischer  <keno@juliacomputing.com>

	PR gdb/25741
	* breakpoint.c (build_target_condition_list): Update comments.
	(build_target_command_list): Update comments and skip matching
	locations.
	(insert_bp_location): Move "set breakpoint auto-hw on" handling to
	a separate function.  Simplify "set breakpoint auto-hw off"
	handling.
	(insert_breakpoints): Update comment.
	(tracepoint_locations_match): New parameter.  For breakpoints,
	compare location types too, if the caller wants to.
	(handle_automatic_hardware_breakpoints): New functions.
	(bp_location_is_less_than): Also sort by location type and
	hardware breakpoint length.
	(update_global_location_list): Handle "set breakpoint auto-hw on"
	here.
	(update_breakpoint_locations): Ask breakpoint_locations_match to
	ignore location types.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/25741
	* gdb.base/hw-sw-break-same-address.exp: New file.
---
 gdb/breakpoint.c                                   | 253 ++++++++++++++-------
 .../gdb.base/hw-sw-break-same-address.exp          |  73 ++++++
 2 files changed, 240 insertions(+), 86 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/hw-sw-break-same-address.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 858f4c75691..3feed929e78 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
@@ -2451,72 +2470,31 @@ insert_bp_location (struct bp_location *bl,
       bl->needs_update = 0;
     }
 
+  /* If "set breakpoint auto-hw" is "on" and a software breakpoint was
+     set at a read-only address, then a breakpoint location will have
+     been changed to hardware breakpoint before we get here.  If it is
+     "off" however, error out before actually trying to insert the
+     breakpoint, with a nicer error message.  */
   if (bl->loc_type == bp_loc_software_breakpoint
-      || bl->loc_type == bp_loc_hardware_breakpoint)
+      && !automatic_hardware_breakpoints)
     {
-      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;
+      mem_region *mr = lookup_mem_region (bl->address);
 
-		      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;
-		}
-	    }
+      if (mr != nullptr && 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;
 	}
-        
+    }
+
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
+    {
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
 	  || bl->section == NULL
@@ -2830,7 +2808,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 +6795,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 +6820,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);
 }
 
@@ -8517,6 +8505,61 @@ mention (struct breakpoint *b)
 
 static bool bp_loc_is_permanent (struct bp_location *loc);
 
+/* Handle "set breakpoint auto-hw on".
+
+   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.
+
+   - 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.
+*/
+
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+  if (automatic_hardware_breakpoints
+      && bl->owner->type != bp_hardware_breakpoint
+      && (bl->loc_type == bp_loc_software_breakpoint
+	  || bl->loc_type == bp_loc_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)
+	{
+	  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;
+		}
+	    }
+	}
+    }
+}
+
 static struct bp_location *
 add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
@@ -11451,6 +11494,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 +11680,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 +11729,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 && should_be_inserted (loc))
+	handle_automatic_hardware_breakpoints (loc);
+    }
+
   std::sort (bp_locations, bp_locations + bp_locations_count,
 	     bp_location_is_less_than);
 
@@ -11776,6 +11846,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 +11863,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 +13580,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/hw-sw-break-same-address.exp b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp
new file mode 100644
index 00000000000..92896ff4db5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp
@@ -0,0 +1,73 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that inserting a hardware and a software breakpoint at the same
+# address behaves as expected.  GDB used to consider hw and sw
+# breakpoint locations as duplicate locations, which would lead to bad
+# behavior.  See PR gdb/25741.
+
+if {[skip_hw_breakpoint_tests]} {
+    return 0
+}
+
+set test hbreak
+set srcfile ${test}.c
+if { [prepare_for_testing "failed to prepare" ${test} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return -1
+}
+
+delete_breakpoints
+
+gdb_test_no_output "set breakpoint always-inserted on"
+gdb_test_no_output "set breakpoint condition-evaluation host"
+gdb_test_no_output "set confirm off"
+
+# Test inserting a hw breakpoint first, then a sw breakpoint at the
+# same address.
+with_test_prefix "hw-sw" {
+    gdb_test "hbreak main" \
+	"Hardware assisted breakpoint .* at .*" \
+	"hbreak"
+
+    gdb_test "break main" \
+	"Note: breakpoint .* also set at .*\r\nBreakpoint .* at .*" \
+	"break"
+
+    # A bad GDB debugging against GDBserver would output a warning
+    # here:
+    #  delete breakpoints
+    #  warning: Error removing breakpoint 3
+    #  (gdb) FAIL: gdb.base/hw-sw-break-same-address.exp: hw-sw: delete breakpoints
+    gdb_test_no_output "delete breakpoints"
+}
+
+# Now the opposite: test inserting a sw breakpoint first, then a hw
+# breakpoint at the same address.
+with_test_prefix "sw-hw" {
+    gdb_test "break main" \
+	"Breakpoint .* at .*" \
+	"break"
+
+    gdb_test "hbreak main" \
+	"Note: breakpoint .* also set at .*\r\nHardware assisted breakpoint .* at .*" \
+	"hbreak"
+
+    gdb_test_no_output "delete breakpoints"
+}

base-commit: 8c164434186b471fc43a47055af9632c56affdcd
-- 
2.14.5