[PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)
Pedro Alves
palves@redhat.com
Sun Apr 19 18:49:12 GMT 2020
On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote:
> It was only after all the effort that I realized that it's
> pointless to try to make hw and sw breakpoint locations
> match _if we always need to install the new sw break before
> deleting the hw break_. I mean, we go through all that
> effort, but then there's always going to be a small window
> where the remote target needs to be able to handle
> a sw breakpoint and a hw breakpoint at the same address
> anyway (e.g., handle target-side breakpoint conditions
> correctly, handle target-side commands correctly, etc.)
>
> So the patch below is the one I wrote yesterday. It
> wasn't finished, but was well advanced. I'm posting
> it for the archives and for discussion. At this point,
> I don't believe we should follow this patch's approach.
> I'll send another reply with today's new patch.
So here's my proposed patch.
This makes breakpoint_locations_match consider the location's
type too, like Keno's original patch. But then does a bunch
more stuff to actually make that work correctly.
- 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" handling is moved out of insert_bp_location
to update_global_location_list, before the duplicates determination.
This change comes with one visible behavior change with
"set breakpoint auto-hw off", however. Here:
Before:
(gdb) break *0x400487
Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
(gdb) c
Continuing.
Warning:
Cannot insert breakpoint 2.
Cannot set software breakpoint at read-only address 0x400487
Command aborted.
(gdb)
After:
(gdb) break *0x400487
Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22.
warning: Breakpoint 2.1 disabled.
Cannot set software breakpoint at read-only address 0x400487
I.e., we now warn earlier, instead of waiting until resume time to error out.
- 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.
- I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any
needed updating. Note that that macro walks all locations at
a given address, and doesn't call breakpoint_locations_match.
The result against GDBserver is this:
(gdb) hbreak main
Sending packet: $m400700,40#28...Packet received: 89e58b0....
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
(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.
Sending packet: $Z0,400736,1#47...Packet received: OK
Sending packet: $Z1,400736,1#48...Packet received: OK
(gdb) del 1
Sending packet: $z1,400736,1#68...Packet received: OK
Sending packet: $Z0,400736,1#47...Packet received: OK
(gdb)
Or, with "set breakpoint condition-evaluation host",
(gdb) hbreak main
Sending packet: $m400736,1#fe...Packet received: 48
Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57.
Sending packet: $Z1,400736,1#48...Packet received: OK
(gdb) b main
Note: breakpoint 3 also set at pc 0x400736.
Sending packet: $m400736,1#fe...Packet received: 48
Breakpoint 4 at 0x400736: file threads.c, line 57.
Sending packet: $Z0,400736,1#47...Packet received: OK
(gdb) del 3
Sending packet: $z1,400736,1#68...Packet received: OK
>From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 19 Apr 2020 18:25:55 +0100
Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
---
gdb/breakpoint.c | 269 ++++++++++++++-------
gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +-
2 files changed, 183 insertions(+), 88 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..27799e89807 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
@@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl,
if (bl->loc_type == bp_loc_software_breakpoint
|| bl->loc_type == bp_loc_hardware_breakpoint)
{
- 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;
-
- 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;
- }
- }
- }
-
/* First check to see if we have to handle an overlay. */
if (overlay_debugging == ovly_off
|| bl->section == NULL
@@ -2830,7 +2786,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);
}
@@ -6813,11 +6773,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;
@@ -6835,9 +6798,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,8 +8481,99 @@ mention (struct breakpoint *b)
}
+/* Return the location number of LOC. */
+
+static int
+bp_location_number (bp_location *loc)
+{
+ int n = 1;
+ for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
+ {
+ if (l == loc)
+ return n;
+ ++n;
+ }
+
+ gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
+}
+
static bool bp_loc_is_permanent (struct bp_location *loc);
+/* Handle "set breakpoint auto-hw".
+
+ 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.
+
+ And then, if "set breakpoint auto-hw" is enabled:
+
+ - 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.
+
+ However, if "set breakpoint auto-hw" is disabled, and memory is
+ read-only, we warn and disable the breakpoint location. */
+
+static void
+handle_automatic_hardware_breakpoints (bp_location *bl)
+{
+ if (bl->loc_type == bp_loc_software_breakpoint
+ || bl->loc_type == bp_loc_hardware_breakpoint)
+ {
+ if (bl->owner->type != bp_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)
+ {
+ 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 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;
+ }
+ }
+ }
+ else if (bl->loc_type == bp_loc_software_breakpoint
+ && mr->attrib.mode != MEM_RW)
+ {
+ bl->enabled = false;
+
+ warning (_("Breakpoint %d.%d disabled.\n"
+ "Cannot set software breakpoint "
+ "at read-only address %s\n"),
+ bl->owner->number, bp_location_number (bl),
+ paddress (bl->gdbarch, bl->address));
+ }
+ }
+ }
+ }
+}
+
static struct bp_location *
add_location_to_breakpoint (struct breakpoint *b,
const struct symtab_and_line *sal)
@@ -11451,6 +11508,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. */
@@ -11625,6 +11694,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.
@@ -11673,6 +11743,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)
+ handle_automatic_hardware_breakpoints (loc);
+ }
+
std::sort (bp_locations, bp_locations + bp_locations_count,
bp_location_is_less_than);
@@ -11776,6 +11860,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
@@ -11790,8 +11877,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;
@@ -13508,11 +13594,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/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 65f7baae9f5..b950e433ed8 100644
--- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
@@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
# Ensure that inserting a software breakpoint in a known-read-only
# region fails.
gdb_test "break *$main_lo" \
- "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
+ "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
"inserting software breakpoint in read-only memory fails"
delete_breakpoints
base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
--
2.14.5
More information about the Gdb-patches
mailing list