This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFA] Support moribund breakpoint locations.
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: gdb-patches at sources dot redhat dot com
- Date: Fri, 2 May 2008 21:11:08 +0300
- Subject: [RFA] Support moribund breakpoint locations.
Dan,
this is a repost of a patch from December; all dependencies are now
in, and this version is fresh against HEAD. The behaviour has not
changed the slightest bit, as far as I know.
- Volodya
* breakpoint.c (moribund_locations): New.
(bpstat_stop_status): Process moribund locations.
(update_global_location_list): Add removed
locations to moribund_locations.
(breakpoint_retire_moribund): New.
* breakpoint.h (struct bp_location): New field
events_till_retirement.
(breakpoint_retire_moribund): Declare.
* thread.c (thread_count): New.
* infrun.c (handle_inferior_event): Call
breakpoint_retire_moribund.
* gdbthread.h (thread_count): Declare.
---
gdb/breakpoint.c | 103 ++++++++++++++++++++++++++++++++++++++++++++----------
gdb/breakpoint.h | 16 ++++++++
gdb/gdbthread.h | 2 +
gdb/infrun.c | 2 +
gdb/thread.c | 12 ++++++
5 files changed, 116 insertions(+), 19 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 24ef7bf..4180ea1 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -323,6 +323,11 @@ struct breakpoint *breakpoint_chain;
struct bp_location *bp_location_chain;
+/* The locations that no longer correspond to any breakpoint,
+ unlinked from bp_location_chain, but for which a hit
+ may still be reported by a target. */
+VEC(bp_location_p) *moribund_locations = NULL;
+
/* Number of last breakpoint made. */
int breakpoint_count;
@@ -3019,10 +3024,12 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
{
struct breakpoint *b = NULL;
const struct bp_location *bl;
+ struct bp_location *loc;
/* Root of the chain of bpstat's */
struct bpstats root_bs[1];
/* Pointer to the last thing in the chain currently. */
bpstat bs = root_bs;
+ int ix;
ALL_BP_LOCATIONS (bl)
{
@@ -3091,6 +3098,18 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
bs->print_it = print_it_noop;
}
+ for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
+ {
+ if (loc->address == bp_addr)
+ {
+ bs = bpstat_alloc (loc, bs);
+ /* For hits of moribund locations, we should just proceed. */
+ bs->stop = 0;
+ bs->print = 0;
+ bs->print_it = print_it_noop;
+ }
+ }
+
bs->next = NULL; /* Terminate the chain */
bs = root_bs->next; /* Re-grab the head of the chain */
@@ -3105,6 +3124,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
if (bs == NULL)
for (bs = root_bs->next; bs != NULL; bs = bs->next)
if (!bs->stop
+ && bs->breakpoint_at->owner
&& (bs->breakpoint_at->owner->type == bp_hardware_watchpoint
|| bs->breakpoint_at->owner->type == bp_read_watchpoint
|| bs->breakpoint_at->owner->type == bp_access_watchpoint))
@@ -3269,6 +3289,9 @@ bpstat_what (bpstat bs)
/* I suspect this can happen if it was a momentary breakpoint
which has since been deleted. */
continue;
+ if (bs->breakpoint_at->owner == NULL)
+ bs_class = bp_nostop;
+ else
switch (bs->breakpoint_at->owner->type)
{
case bp_none:
@@ -6979,7 +7002,9 @@ breakpoint_auto_delete (bpstat bs)
struct breakpoint *b, *temp;
for (; bs; bs = bs->next)
- if (bs->breakpoint_at && bs->breakpoint_at->owner->disposition == disp_del
+ if (bs->breakpoint_at
+ && bs->breakpoint_at->owner
+ && bs->breakpoint_at->owner->disposition == disp_del
&& bs->stop)
delete_breakpoint (bs->breakpoint_at->owner);
@@ -7027,6 +7052,9 @@ update_global_location_list (void)
/* Tells if 'loc' is found amoung the new locations. If not, we
have to free it. */
int found_object = 0;
+ /* Tells if the location should remain inserted in the target. */
+ int keep_in_target = 0;
+ int removed = 0;
for (loc2 = bp_location_chain; loc2; loc2 = loc2->global_next)
if (loc2 == loc)
{
@@ -7043,13 +7071,12 @@ update_global_location_list (void)
if (loc->inserted)
{
/* If the location is inserted now, we might have to remove it. */
- int keep = 0;
if (found_object && should_be_inserted (loc))
{
/* The location is still present in the location list, and still
should be inserted. Don't do anything. */
- keep = 1;
+ keep_in_target = 1;
}
else
{
@@ -7067,30 +7094,53 @@ update_global_location_list (void)
{
loc2->inserted = 1;
loc2->target_info = loc->target_info;
- keep = 1;
+ keep_in_target = 1;
break;
}
}
}
- if (!keep)
- if (remove_breakpoint (loc, mark_uninserted))
- {
- /* This is just about all we can do. We could keep this
- location on the global list, and try to remove it next
- time, but there's no particular reason why we will
- succeed next time.
-
- Note that at this point, loc->owner is still valid,
- as delete_breakpoint frees the breakpoint only
- after calling us. */
- printf_filtered (_("warning: Error removing breakpoint %d\n"),
- loc->owner->number);
- }
+ if (!keep_in_target)
+ {
+ if (remove_breakpoint (loc, mark_uninserted))
+ {
+ /* This is just about all we can do. We could keep this
+ location on the global list, and try to remove it next
+ time, but there's no particular reason why we will
+ succeed next time.
+
+ Note that at this point, loc->owner is still valid,
+ as delete_breakpoint frees the breakpoint only
+ after calling us. */
+ printf_filtered (_("warning: Error removing breakpoint %d\n"),
+ loc->owner->number);
+ }
+ removed = 1;
+ }
}
if (!found_object)
- free_bp_location (loc);
+ {
+ if (removed)
+ {
+ /* This location was removed from the targets. In non-stop mode,
+ a race condition is possible where we've removed a breakpoint,
+ but stop events for that breakpoint are already queued and will
+ arrive later. To suppress spurious SIGTRAPs reported to user,
+ we keep this breakpoint location for a bit, and will retire it
+ after we see 3 * thread_count events.
+ The theory here is that reporting of events should,
+ "on the average", be fair, so after that many event we'll see
+ events from all threads that have anything of interest, and no
+ longer need to keep this breakpoint. This is just a
+ heuristic, but if it's wrong, we'll report unexpected SIGTRAP,
+ which is usability issue, but not a correctness problem. */
+ loc->events_till_retirement = 3 * (thread_count () + 1);
+ loc->owner = NULL;
+ }
+
+ free_bp_location (loc);
+ }
}
ALL_BREAKPOINTS (b)
@@ -7102,6 +7152,21 @@ update_global_location_list (void)
insert_breakpoint_locations ();
}
+void
+breakpoint_retire_moribund ()
+{
+ struct bp_location *loc;
+ int ix;
+
+ for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
+ if (--(loc->events_till_retirement) == 0)
+ {
+ free_bp_location (loc);
+ VEC_unordered_remove (bp_location_p, moribund_locations, ix);
+ --ix;
+ }
+}
+
static void
update_global_location_list_nothrow (void)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index b31c965..5e8946f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -298,6 +298,17 @@ struct bp_location
/* Similarly, for the breakpoint at an overlay's LMA, if necessary. */
struct bp_target_info overlay_target_info;
+
+ /* In a non-stop mode, it's possible that we delete a breakpoint,
+ but as we do that, some still running thread hits that breakpoint.
+ For that reason, we need to keep locations belonging to deleted
+ breakpoints for a bit, so that don't report unexpected SIGTRAP.
+ We can't keep such locations forever, so we use a heuristic --
+ after we process certain number of inferior events since
+ breakpoint was deleted, we retire all locations of that breakpoint.
+ This variable keeps a number of events still to go, when
+ it becomes 0 this location is retired. */
+ int events_till_retirement;
};
/* This structure is a collection of function pointers that, if available,
@@ -864,4 +875,9 @@ void breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr,
extern int breakpoints_always_inserted_mode (void);
+/* Called each time new event from target is processed.
+ Retires previously deleted breakpoint locations that
+ in our opinion won't ever trigger. */
+extern void breakpoint_retire_moribund ();
+
#endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index e8bfde4..6cfb1f9 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -115,6 +115,8 @@ extern struct thread_info *find_thread_pid (ptid_t ptid);
typedef int (*thread_callback_func) (struct thread_info *, void *);
extern struct thread_info *iterate_over_threads (thread_callback_func, void *);
+extern int thread_count ();
+
/* infrun context switch: save the debugger state for the given thread. */
extern void save_infrun_state (ptid_t ptid,
CORE_ADDR prev_pc,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index f576a59..b9ba2fa 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1277,6 +1277,8 @@ handle_inferior_event (struct execution_control_state *ecs)
int stopped_by_watchpoint;
int stepped_after_stopped_by_watchpoint = 0;
+ breakpoint_retire_moribund ();
+
/* Cache the last pid/waitstatus. */
target_last_wait_ptid = ecs->ptid;
target_last_waitstatus = *ecs->wp;
diff --git a/gdb/thread.c b/gdb/thread.c
index c04c989..03ceaf6 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -226,6 +226,18 @@ iterate_over_threads (int (*callback) (struct thread_info *, void *),
}
int
+thread_count ()
+{
+ int result = 0;
+ struct thread_info *tp;
+
+ for (tp = thread_list; tp; tp = tp->next)
+ ++result;
+
+ return result;
+}
+
+int
valid_thread_id (int num)
{
struct thread_info *tp;
--
1.5.3.5