From 7f32a4d5aef1891813d5e4a4bd97151797edc82d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Sun, 17 May 2020 19:17:56 +0100 Subject: [PATCH] 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 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 on" 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. --- gdb/ChangeLog | 22 ++ gdb/breakpoint.c | 253 ++++++++++++------ gdb/testsuite/ChangeLog | 5 + .../gdb.base/hw-sw-break-same-address.exp | 73 +++++ 4 files changed, 267 insertions(+), 86 deletions(-) create mode 100644 gdb/testsuite/gdb.base/hw-sw-break-same-address.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index aa42a7a35c..1c2ddcecb7 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,25 @@ +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 on" + here. + (update_breakpoint_locations): Ask breakpoint_locations_match to + ignore location types. + 2020-05-16 Simon Marchi * gdbtypes.h (TYPE_NAME): Remove. Change all cal sites to use diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 15d76d4a9c..aead882acd 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); } @@ -6811,11 +6793,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; @@ -6833,9 +6818,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); } @@ -8515,6 +8503,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) @@ -11438,6 +11481,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. */ @@ -11612,6 +11667,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. @@ -11660,6 +11716,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); @@ -11763,6 +11833,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 @@ -11777,8 +11850,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; @@ -13495,11 +13567,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/ChangeLog b/gdb/testsuite/ChangeLog index 0be2fd3386..2a3e72c022 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2020-05-17 Pedro Alves + + PR gdb/25741 + * gdb.base/hw-sw-break-same-address.exp: New file. + 2020-05-16 Pedro Alves * gdb.multi/multi-re-run.exp (test_re_run): Switch 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 0000000000..92896ff4db --- /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 . + +# 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" +} -- 2.34.1