Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741)

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:
2020-05-17  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:
2020-05-17  Pedro Alves  <palves@redhat.com>

	PR gdb/25741
	* gdb.base/hw-sw-break-same-address.exp: New file.