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

["cvs-commit at gcc dot gnu.org" <sourceware-bugzilla@sourceware.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.