[Bug remote/25741] New: GDB tries to set breakpoint using Z0, but remove it using z1

[Bug remote/25741] New: GDB tries to set breakpoint using Z0, but remove it using z1

[keno at juliacomputing dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

            Bug ID: 25741
           Summary: GDB tries to set breakpoint using Z0, but remove it
                    using z1
           Product: gdb
           Version: HEAD
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: remote
          Assignee: unassigned at sourceware dot org
          Reporter: keno at juliacomputing dot com
  Target Milestone: ---

I'm using GDB to talk to the rr (github.com/mozilla/rr) remote server.
In one particular instance, I'm experiencing what I believe to be a bug
in GDB.
In particular, if I print out the GDB remote log (`set debug remote 1`), I
see in relevant part (extracting all Zz packets):


```
(gdb) set debug remote 1
(gdb) handle SIGSEGV pass nostop noprint
Signal        Stop      Print   Pass to program Description
SIGSEGV       No        No      Yes             Segmentation fault
(gdb) handle SIGTRAP pass nostop noprint
SIGTRAP is used by the debugger.
Are you sure you want to change it? (y or n) y
Signal        Stop      Print   Pass to program Description
SIGTRAP       No        No      Yes             Trace/breakpoint trap
(gdb) hbreak *0xf7f736db
Sending packet: $mf7f736db,1#33...Packet received: E01
Hardware assisted breakpoint 1 at 0xf7f736db
(gdb) c
Continuing.
Sending packet: $Z1,f7f736db,1#7d...Packet received: OK
Packet Z1 (hardware-breakpoint) is supported
[...]
Sending packet: $z1,f7f736db,1#9d...Packet received: OK
[...]
Sending packet: $Z0,f7f736db,1#7c...Packet received: OK
Packet Z0 (software-breakpoint) is supported
[...]
Breakpoint 1,
0xf7f736db in ?? ()
(gdb) c
[...]
Sending packet: $z1,f7f736db,1#9d...
```

Full log at: https://gist.github.com/Keno/5c8345356d3124419bdfb8a80173c7a3.
In particular, note that after hitting the watchpoint, GDB tries to
re-establish
the watchpoint with `Z0` rather than `Z1`. This causes rr to complain on the
subsequent `z1` (since no such watchpoint was set only a sw breakpoint).

The above trace if from GDB HEAD, compiled just now:
GNU gdb (GDB) 10.0.50.20200328-git

but I originally saw the issue in the GDB that ships with my OS:
GNU gdb (Ubuntu 9.0.50.20191119-0ubuntu1) 9.0.50.20191119-git

I have also reported this issue to rr at
https://github.com/mozilla/rr/issues/2474,
though I believe it is likely to be a GDB issue.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1

[keno at juliacomputing dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

--- Comment #1 from Keno Fischer <keno at juliacomputing dot com> ---
I have debugged this and have a proposed patch:

```
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461b..9854754c7d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1,
     /* We compare bp_location.length in order to cover ranged breakpoints.  */
     return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
                                     loc2->pspace->aspace, loc2->address)
-           && loc1->length == loc2->length);
+           && loc1->length == loc2->length && loc1->loc_type ==
loc2->loc_type);
 }

 static void
```

The issue is that gdb thinks it can just switch over the breakpoint,
since there's a breakpoint already there (a bp_hp_step_resume breakpoint),
but that shouldn't work if one of them is a hardware and one of them is
a software breakpoint. I don't have a enough of a global view to know if
this is the right place to fix that, but it does solve my issue.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1

[cbiesinger at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

Christian Biesinger <cbiesinger at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cbiesinger at google dot com

--- Comment #2 from Christian Biesinger <cbiesinger at google dot com> ---
Hi Keno,

Thanks for providing a patch! Could you send it to gdb-patches@sourceware.org
following the instructions at
https://sourceware.org/gdb/wiki/ContributionChecklist?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1

[keno at juliacomputing dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

--- Comment #3 from Keno Fischer <keno at juliacomputing dot com> ---
Sent as https://marc.info/?l=gdb-patches&m=158571310180513&w=2. Haven't gotten
any comments on it yet though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

--- Comment #4 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <palves@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7f32a4d5aef1891813d5e4a4bd97151797edc82d

commit 7f32a4d5aef1891813d5e4a4bd97151797edc82d
Author: Pedro Alves <palves@redhat.com>
Date:   Sun May 17 19:17:56 2020 +0100

    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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25741

Pedro Alves <palves at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
                 CC|                            |palves at redhat dot com
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |10.1

--- Comment #5 from Pedro Alves <palves at redhat dot com> ---
Fix merged.

-- 
You are receiving this mail because:
You are on the CC list for the bug.