This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Fix regression in always-inserted breakpoints mode.
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org, Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Date: Fri, 20 Nov 2009 02:11:18 +0000
- Subject: Fix regression in always-inserted breakpoints mode.
I forward ported to mainline a patch that makes
always-inserted mode work with watchpoint locations too,
and surprisingly, GDB was still removing/inserting watchpoints
at every step/event. The original patch worked correctly, but
it predated the update_global_location_list rework to qsort
the locations.
I traced it down to the use of bp_location_compare in
update_global_location_list. All else being equal,
bp_location_compare compares the input pointers.
Often, after a remove/insert sequence, we reach the
bp_location_compare call line shown in the patch below,
while (locp < bp_location + bp_location_count
&& bp_location_compare (*locp, old_loc) < 0)
locp++;
with `*locp' and `old_loc' having the same address, but
bp_location_compare would still return < 0, due to the
fallback pointer comparision. This made it so that
locp++ statement was done one time too much. The
`found_object' check would fail, and GDB would uninsert
a location from the target, that shouldn't be removed.
There's for loop a bit below, that starts iterating on
`locp', and that would misses a few locations, due to the
spurious increment mentioned above. Here's the loop
in question:
for (loc2p = locp;
loc2p < bp_location + bp_location_count
&& breakpoint_address_match ((*loc2p)->pspace->aspace,
(*loc2p)->address,
old_loc->pspace->aspace,
old_loc->address);
loc2p++)
There's another problem with this for loop. The
breakpoint_address_match is too strict when debugging
more than one inferior. Even if breakpoint_address_match
fails due to address space mismatch, there could be other
locations with the same address following loc2p.
Here's a fix for all this. Tested on x86-64-linux, native
and gdbserver, no regressions.
Jan, could you double check?
--
Pedro Alves
2009-11-20 Pedro Alves <pedro@codesourcery.com>
* breakpoint.c (update_global_location_list): Fix duplicate
locations detection.
---
gdb/breakpoint.c | 51 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 19 deletions(-)
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c 2009-11-20 00:38:12.000000000 +0000
+++ src/gdb/breakpoint.c 2009-11-20 01:02:18.000000000 +0000
@@ -8249,10 +8249,11 @@ update_global_location_list (int should_
old_locp++)
{
struct bp_location *old_loc = *old_locp;
+ struct bp_location **loc2p;
/* Tells if 'old_loc' is found amoung the new locations. If not, we
have to free it. */
- int found_object;
+ int found_object = 0;
/* Tells if the location should remain inserted in the target. */
int keep_in_target = 0;
int removed = 0;
@@ -8260,9 +8261,20 @@ update_global_location_list (int should_
/* Skip LOCP entries which will definitely never be needed. Stop either
at or being the one matching OLD_LOC. */
while (locp < bp_location + bp_location_count
- && bp_location_compare (*locp, old_loc) < 0)
+ && (*locp)->address < old_loc->address)
locp++;
- found_object = locp < bp_location + bp_location_count && *locp == old_loc;
+
+ for (loc2p = locp;
+ (loc2p < bp_location + bp_location_count
+ && (*loc2p)->address == old_loc->address);
+ loc2p++)
+ {
+ if (*loc2p == old_loc)
+ {
+ found_object = 1;
+ break;
+ }
+ }
/* If this location is no longer present, and inserted, look if there's
maybe a new location at the same address. If so, mark that one
@@ -8288,27 +8300,28 @@ update_global_location_list (int should_
if (breakpoint_address_is_meaningful (old_loc->owner))
{
- struct bp_location **loc2p;
-
for (loc2p = locp;
- loc2p < bp_location + bp_location_count
- && breakpoint_address_match ((*loc2p)->pspace->aspace,
- (*loc2p)->address,
- old_loc->pspace->aspace,
- old_loc->address);
+ (loc2p < bp_location + bp_location_count
+ && (*loc2p)->address == old_loc->address);
loc2p++)
{
struct bp_location *loc2 = *loc2p;
- /* For the sake of should_be_inserted.
- Duplicates check below will fix up this later. */
- loc2->duplicate = 0;
- if (loc2 != old_loc && should_be_inserted (loc2))
- {
- loc2->inserted = 1;
- loc2->target_info = old_loc->target_info;
- keep_in_target = 1;
- break;
+ if (breakpoint_address_match (loc2->pspace->aspace,
+ loc2->address,
+ old_loc->pspace->aspace,
+ old_loc->address))
+ {
+ /* For the sake of should_be_inserted.
+ Duplicates check below will fix up this later. */
+ loc2->duplicate = 0;
+ if (loc2 != old_loc && should_be_inserted (loc2))
+ {
+ loc2->inserted = 1;
+ loc2->target_info = old_loc->target_info;
+ keep_in_target = 1;
+ break;
+ }
}
}
}