From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 5C7CF386F83E; Sun, 17 May 2020 18:24:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5C7CF386F83E From: "cvs-commit at gcc dot gnu.org" To: gdb-prs@sourceware.org Subject: [Bug remote/25741] GDB tries to set breakpoint using Z0, but remove it using z1 Date: Sun, 17 May 2020 18:24:26 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gdb X-Bugzilla-Component: remote X-Bugzilla-Version: HEAD X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: cvs-commit at gcc dot gnu.org X-Bugzilla-Status: UNCONFIRMED X-Bugzilla-Resolution: X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: unassigned at sourceware dot org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://sourceware.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-BeenThere: gdb-prs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-prs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 May 2020 18:24:26 -0000 https://sourceware.org/bugzilla/show_bug.cgi?id=3D25741 --- Comment #4 from cvs-commit at gcc dot gnu.org --- The master branch has been updated by Pedro Alves : https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D7f32a4d5aef1= 891813d5e4a4bd97151797edc82d commit 7f32a4d5aef1891813d5e4a4bd97151797edc82d Author: Pedro Alves Date: Sun May 17 19:17:56 2020 +0100 Stop considering hw and sw breakpoint locations duplicates (PR gdb/2574= 1) 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 Andrew Burgess Keno Fischer 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 o= n" here. (update_breakpoint_locations): Ask breakpoint_locations_match to ignore location types. gdb/testsuite/ChangeLog: 2020-05-17 Pedro Alves PR gdb/25741 * gdb.base/hw-sw-break-same-address.exp: New file. --=20 You are receiving this mail because: You are on the CC list for the bug.=