This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
remove global stop_bpstat dependency from breakpoints module
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Thu, 8 May 2008 02:22:04 +0100
- Subject: remove global stop_bpstat dependency from breakpoints module
Hi,
Currently, GDB assumes there can only be one stop_bpstat. This is OK in
all-stop mode, as there can only be one even that the user has to care for.
There are two uses of stop_bpstat outside of infrun.c, that is, outside
of the path that handles an event.
The first, is in infcmd.c:continue_command, where we
implement setting a proceed count on the breakpoint we're stopped at:
(gdb) help continue
Continue program being debugged, after signal or breakpoint.
If proceeding from breakpoint, a number N may be used as an argument,
which means to set the ignore count of that breakpoint to N - 1 (so that
the breakpoint won't break until the Nth time it is reached).
/* If have argument (besides '&'), set proceed count of breakpoint
we stopped at. */
if (proc_count_exp != NULL)
{
bpstat bs = stop_bpstat;
int num, stat;
int stopped = 0;
while ((stat = bpstat_num (&bs, &num)) != 0)
if (stat > 0)
{
set_ignore_count (num,
parse_and_eval_long (proc_count_exp) - 1,
from_tty);
/* set_ignore_count prints a message ending with a period.
So print two spaces before "Continuing.". */
if (from_tty)
printf_filtered (" ");
stopped = 1;
}
if (!stopped && from_tty)
{
printf_filtered
("Not stopped at any breakpoint; argument ignored.\n");
}
}
I had missed this on the non-stop series. I'll need to context switch
stop_bpstat in non-stop mode, because there, we'll have simultaneous
independant stop events, one per thread, and each should have its own
stop_bpstat. That's a small change to patch 4 in series I posted.
The second place is in breakpoints.c:delete_breakpoint. When we delete
a breakpoint, we're looking through the stop_bpstat chain for a matching
breakpoint, so we can clear the location from it:
void
delete_breakpoint (struct breakpoint *bpt)
{
...
/* Be sure no bpstat's are pointing at it after it's been freed. */
/* FIXME, how can we find all bpstat's?
We just check stop_bpstat for now. Note that we cannot just
remove bpstats pointing at bpt from the stop_bpstat list
entirely, as breakpoint commands are associated with the bpstat;
if we remove it here, then the later call to
bpstat_do_actions (&stop_bpstat);
in event-top.c won't do anything, and temporary breakpoints
with commands won't work. */
for (bs = stop_bpstat; bs; bs = bs->next)
if (bs->breakpoint_at && bs->breakpoint_at->owner == bpt)
{
bs->breakpoint_at = NULL;
bs->old_val = NULL;
/* bs->commands will be freed later. */
}
...
}
(Nevermind that bs->old_val is leaking.)
Now, if we make a stop_bpstat per-thread, due to context-switching
this will only update the stop_stat that corresponds to the
current thread, leaving the stop_bpstats of the other threads,
if stopped at the breakpoint we're deleting, with dangling pointers.
One way to fix it, would be to also loop through all threads to update
their version of stop_bpstat, but I'd like better.
Another form, is to remove the dependency on knowing about stop_bpstat
at all. That is what this patch implements. To do that, I added
reference counting to bp_locations, and made sure the counts are updated
at the appropriate places. To mark the location as ready for garbage
collection, I've added a new bp_loc_dead location type.
Does this look sane? I've tested this on x86_64-unknown-linux
with and without breakpoints always-inserted on, without
any regressions.
--
Pedro Alves
2008-05-08 Pedro Alves <pedro@codesourcery.com>
* breakpoint.h (enum bp_loc_type): Add bp_loc_dead.
(struct bp_location): Add refcount.
(struct bpstats): Remove const qualifier from breakpoint_at.
* breakpoint.c (invalid_bp_location): New global.
(bp_location_is_dead, bp_location_ref, bp_location_unref): New
functions.
(bpstat_free): Decrement breakpoint_at's reference count.
(bpstat_copy): Increment breakpoint_at's reference count.
(bpstat_find_breakpoint): Check for dead bp_location instead of
checking for valid pointer.
(bpstat_find_step_resume_breakpoint, bpstat_num)
(print_it_typical): Ditto.
(print_bp_stop_message): If location is dead, return
PRINT_UNKNOWN.
(bpstat_alloc): Remove const qualifier from BL parameter.
Increment breakpoint_at's reference count.
(bpstat_stop_status): When updating watchpoints locations, don't
check bs->stop. Decrement breakpoint_at's reference count, and
point bpstat at invalid_bp_location.
(bpstat_what): Check for dead bp_location instead of checking for
NULL pointer.
(allocate_bp_location): Set refcount to 1.
(free_bp_location): Decrement location's reference count and mark
it as dead instead of freeing it.
(breakpoint_auto_delete): Check for dead bp_location instead of
checking for NULL pointer.
(delete_breakpoint): Remove redundant checks. Don't look over the
bsstats for a matching breakpoint to clear it.
---
gdb/breakpoint.c | 177 +++++++++++++++++++++++++++++--------------------------
gdb/breakpoint.h | 17 ++++-
2 files changed, 109 insertions(+), 85 deletions(-)
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h 2008-05-08 01:32:05.000000000 +0100
+++ src/gdb/breakpoint.h 2008-05-08 01:55:30.000000000 +0100
@@ -215,6 +215,10 @@ struct bp_target_info
enum bp_loc_type
{
+ /* This bp_location is now dead, but there are still references to
+ it somewhere. A dead location will never appear in the global
+ location list. */
+ bp_loc_dead,
bp_loc_software_breakpoint,
bp_loc_hardware_breakpoint,
bp_loc_hardware_watchpoint,
@@ -230,7 +234,14 @@ struct bp_location
/* Pointer to the next breakpoint location, in a global
list of all breakpoint locations. */
struct bp_location *global_next;
-
+
+ /* The lifetime of a bp_location object is managed with refcounting.
+ This is because pointers to the same bp_location object may be
+ stored in several places (e.g., struct breakpoint and bpstats).
+ A bp_location that is not useful anymore is identified by having
+ bp_loc_dead type. */
+ int refcount;
+
/* Type of this breakpoint location. */
enum bp_loc_type loc_type;
@@ -273,7 +284,7 @@ struct bp_location
bp_loc_other. */
CORE_ADDR address;
- /* For hardware watchpoints, the size of data ad ADDRESS being watches. */
+ /* For hardware watchpoints, the size of data at ADDRESS being watched. */
int length;
/* Type of hardware watchpoint. */
@@ -632,7 +643,7 @@ struct bpstats
place, and a bpstat reflects the fact that both have been hit. */
bpstat next;
/* Breakpoint that we are at. */
- const struct bp_location *breakpoint_at;
+ struct bp_location *breakpoint_at;
/* Commands left to be done. */
struct command_line *commands;
/* Old value associated with a watchpoint. */
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c 2008-05-08 01:32:05.000000000 +0100
+++ src/gdb/breakpoint.c 2008-05-08 01:38:11.000000000 +0100
@@ -114,7 +114,7 @@ static void breakpoints_info (char *, in
static void breakpoint_1 (int, int);
-static bpstat bpstat_alloc (const struct bp_location *, bpstat);
+static bpstat bpstat_alloc (struct bp_location *, bpstat);
static int breakpoint_cond_eval (void *);
@@ -203,6 +203,36 @@ static int is_hardware_watchpoint (struc
static void insert_breakpoint_locations (void);
+/* This exists so we can make bpstat->breakpoint_at point somewhere in
+ bpstat_stop_status, when rebuilding the locations of
+ watchpoints. */
+static struct bp_location invalid_bp_location = { NULL, NULL, 0, bp_loc_dead };
+
+static int
+bp_location_is_dead (struct bp_location *bl)
+{
+ return bl->loc_type == bp_loc_dead;
+}
+
+static void
+bp_location_ref (struct bp_location *bl)
+{
+ if (bl == &invalid_bp_location)
+ return;
+
+ bl->refcount++;
+}
+
+static void
+bp_location_unref (struct bp_location *bl)
+{
+ if (bl == &invalid_bp_location)
+ return;
+
+ if (--bl->refcount == 0)
+ xfree (bl);
+}
+
static const char *
bpdisp_text (enum bpdisp disp)
{
@@ -863,7 +893,7 @@ fetch_watchpoint_value (struct expressio
}
/* Assuming that B is a hardware watchpoint:
- - Reparse watchpoint expression, is REPARSE is non-zero
+ - Reparse watchpoint expression, if REPARSE is non-zero
- Evaluate expression and store the result in B->val
- Update the list of values that must be watched in B->loc.
@@ -1956,6 +1986,7 @@ bpstat_free (bpstat bs)
if (bs->old_val != NULL)
value_free (bs->old_val);
free_command_lines (&bs->commands);
+ bp_location_unref (bs->breakpoint_at);
xfree (bs);
}
@@ -1997,6 +2028,7 @@ bpstat_copy (bpstat bs)
{
tmp = (bpstat) xmalloc (sizeof (*tmp));
memcpy (tmp, bs, sizeof (*tmp));
+ bp_location_ref (tmp->breakpoint_at);
if (bs->commands != NULL)
tmp->commands = copy_command_lines (bs->commands);
if (bs->old_val != NULL)
@@ -2023,7 +2055,8 @@ bpstat_find_breakpoint (bpstat bsp, stru
for (; bsp != NULL; bsp = bsp->next)
{
- if (bsp->breakpoint_at && bsp->breakpoint_at->owner == breakpoint)
+ if (!bp_location_is_dead (bsp->breakpoint_at)
+ && bsp->breakpoint_at->owner == breakpoint)
return bsp;
}
return NULL;
@@ -2048,10 +2081,10 @@ bpstat_find_step_resume_breakpoint (bpst
for (; bsp != NULL; bsp = bsp->next)
{
- if ((bsp->breakpoint_at != NULL) &&
- (bsp->breakpoint_at->owner->type == bp_step_resume) &&
- (bsp->breakpoint_at->owner->thread == current_thread ||
- bsp->breakpoint_at->owner->thread == -1))
+ if (!bp_location_is_dead (bsp->breakpoint_at)
+ && bsp->breakpoint_at->owner->type == bp_step_resume
+ && (bsp->breakpoint_at->owner->thread == current_thread
+ || bsp->breakpoint_at->owner->thread == -1))
return bsp->breakpoint_at->owner;
}
@@ -2072,17 +2105,19 @@ int
bpstat_num (bpstat *bsp, int *num)
{
struct breakpoint *b;
+ bpstat s = *bsp;
- if ((*bsp) == NULL)
+ if (s == NULL)
return 0; /* No more breakpoint values */
- /* We assume we'll never have several bpstats that
- correspond to a single breakpoint -- otherwise,
- this function might return the same number more
- than once and this will look ugly. */
- b = (*bsp)->breakpoint_at ? (*bsp)->breakpoint_at->owner : NULL;
*bsp = (*bsp)->next;
- if (b == NULL)
+
+ /* We assume we'll never have several bpstats that correspond to a
+ single breakpoint -- otherwise, this function might return the
+ same number more than once and this will look ugly. */
+ if (!bp_location_is_dead (s->breakpoint_at))
+ b = s->breakpoint_at->owner;
+ else
return -1; /* breakpoint that's been deleted since */
*num = b->number; /* We have its number */
@@ -2247,9 +2282,8 @@ print_it_typical (bpstat bs)
int bp_temp = 0;
stb = ui_out_stream_new (uiout);
old_chain = make_cleanup_ui_out_stream_delete (stb);
- /* bs->breakpoint_at can be NULL if it was a momentary breakpoint
- which has since been deleted. */
- if (bs->breakpoint_at == NULL)
+ if (bp_location_is_dead (bs->breakpoint_at))
+ /* A momentary breakpoint which has since been deleted. */
return PRINT_UNKNOWN;
bl = bs->breakpoint_at;
b = bl->owner;
@@ -2462,13 +2496,16 @@ print_bp_stop_message (bpstat bs)
case print_it_normal:
{
- const struct bp_location *bl = bs->breakpoint_at;
- struct breakpoint *b = bl ? bl->owner : NULL;
-
+ struct breakpoint *b;
+
+ if (bp_location_is_dead (bs->breakpoint_at))
+ return PRINT_UNKNOWN;
+
+ b = bs->breakpoint_at->owner;
+
/* Normal case. Call the breakpoint's print_it method, or
print_it_typical. */
- /* FIXME: how breakpoint can ever be NULL here? */
- if (b != NULL && b->ops != NULL && b->ops->print_it != NULL)
+ if (b->ops != NULL && b->ops->print_it != NULL)
return b->ops->print_it (b);
else
return print_it_typical (bs);
@@ -2542,13 +2579,14 @@ breakpoint_cond_eval (void *exp)
/* Allocate a new bpstat and chain it to the current one. */
static bpstat
-bpstat_alloc (const struct bp_location *bl, bpstat cbs /* Current "bs" value */ )
+bpstat_alloc (struct bp_location *bl, bpstat cbs /* Current "bs" value */ )
{
bpstat bs;
bs = (bpstat) xmalloc (sizeof (*bs));
cbs->next = bs;
bs->breakpoint_at = bl;
+ bp_location_ref (bs->breakpoint_at);
/* If the condition is false, etc., don't do the commands. */
bs->commands = NULL;
bs->old_val = NULL;
@@ -3011,7 +3049,7 @@ bpstat
bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
{
struct breakpoint *b = NULL;
- const struct bp_location *bl;
+ struct bp_location *bl;
/* Root of the chain of bpstat's */
struct bpstats root_bs[1];
/* Pointer to the last thing in the chain currently. */
@@ -3026,10 +3064,9 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
/* For hardware watchpoints, we look only at the first location.
The watchpoint_check function will work on entire expression,
- not the individual locations. For read watchopints, the
- watchpoints_triggered function have checked all locations
- alrea
- */
+ not the individual locations. For read watchpoints, the
+ watchpoints_triggered function has checked all locations
+ already. */
if (b->type == bp_hardware_watchpoint && bl != b->loc)
continue;
@@ -3097,15 +3134,13 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
if (bs == NULL)
for (bs = root_bs->next; bs != NULL; bs = bs->next)
- if (!bs->stop
- && (bs->breakpoint_at->owner->type == bp_hardware_watchpoint
- || bs->breakpoint_at->owner->type == bp_read_watchpoint
- || bs->breakpoint_at->owner->type == bp_access_watchpoint))
+ if (is_hardware_watchpoint (bs->breakpoint_at->owner))
{
/* remove/insert can invalidate bs->breakpoint_at, if this
location is no longer used by the watchpoint. Prevent
further code from trying to use it. */
- bs->breakpoint_at = NULL;
+ bp_location_unref (bs->breakpoint_at);
+ bs->breakpoint_at = &invalid_bp_location;
remove_breakpoints ();
insert_breakpoints ();
break;
@@ -3258,7 +3293,7 @@ bpstat_what (bpstat bs)
for (; bs != NULL; bs = bs->next)
{
enum class bs_class = no_effect;
- if (bs->breakpoint_at == NULL)
+ if (bp_location_is_dead (bs->breakpoint_at))
/* I suspect this can happen if it was a momentary breakpoint
which has since been deleted. */
continue;
@@ -4257,6 +4292,7 @@ allocate_bp_location (struct breakpoint
loc->cond = NULL;
loc->shlib_disabled = 0;
loc->enabled = 1;
+ loc->refcount = 1;
switch (bp_type)
{
@@ -4296,15 +4332,19 @@ allocate_bp_location (struct breakpoint
return loc;
}
-static void free_bp_location (struct bp_location *loc)
+static void
+free_bp_location (struct bp_location *loc)
{
if (loc->cond)
xfree (loc->cond);
if (loc->function_name)
xfree (loc->function_name);
-
- xfree (loc);
+
+ /* Mark as dead, so bpstats still pointing into it know it was
+ deleted. */
+ loc->loc_type = bp_loc_dead;
+ bp_location_unref (loc);
}
/* Helper to set_raw_breakpoint below. Creates a breakpoint
@@ -6943,7 +6983,8 @@ 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 (!bp_location_is_dead (bs->breakpoint_at)
+ && bs->breakpoint_at->owner->disposition == disp_del
&& bs->stop)
delete_breakpoint (bs->breakpoint_at->owner);
@@ -7117,50 +7158,22 @@ delete_breakpoint (struct breakpoint *bp
}
free_command_lines (&bpt->commands);
- if (bpt->cond_string != NULL)
- xfree (bpt->cond_string);
- if (bpt->addr_string != NULL)
- xfree (bpt->addr_string);
- if (bpt->exp != NULL)
- xfree (bpt->exp);
- if (bpt->exp_string != NULL)
- xfree (bpt->exp_string);
- if (bpt->val != NULL)
- value_free (bpt->val);
- if (bpt->source_file != NULL)
- xfree (bpt->source_file);
- if (bpt->dll_pathname != NULL)
- xfree (bpt->dll_pathname);
- if (bpt->triggered_dll_pathname != NULL)
- xfree (bpt->triggered_dll_pathname);
- if (bpt->exec_pathname != NULL)
- xfree (bpt->exec_pathname);
-
- /* Be sure no bpstat's are pointing at it after it's been freed. */
- /* FIXME, how can we find all bpstat's?
- We just check stop_bpstat for now. Note that we cannot just
- remove bpstats pointing at bpt from the stop_bpstat list
- entirely, as breakpoint commands are associated with the bpstat;
- if we remove it here, then the later call to
- bpstat_do_actions (&stop_bpstat);
- in event-top.c won't do anything, and temporary breakpoints
- with commands won't work. */
- for (bs = stop_bpstat; bs; bs = bs->next)
- if (bs->breakpoint_at && bs->breakpoint_at->owner == bpt)
- {
- bs->breakpoint_at = NULL;
- bs->old_val = NULL;
- /* bs->commands will be freed later. */
- }
-
- /* Now that breakpoint is removed from breakpoint
- list, update the global location list. This
- will remove locations that used to belong to
- this breakpoint. Do this before freeing
- the breakpoint itself, since remove_breakpoint
- looks at location's owner. It might be better
- design to have location completely self-contained,
- but it's not the case now. */
+ xfree (bpt->cond_string);
+ xfree (bpt->addr_string);
+ xfree (bpt->exp);
+ xfree (bpt->exp_string);
+ value_free (bpt->val);
+ xfree (bpt->source_file);
+ xfree (bpt->dll_pathname);
+ xfree (bpt->triggered_dll_pathname);
+ xfree (bpt->exec_pathname);
+
+ /* Now that breakpoint is removed from breakpoint list, update the
+ global location list. This will remove locations that used to
+ belong to this breakpoint. Do this before freeing the breakpoint
+ itself, since remove_breakpoint looks at location's owner. It
+ might be better design to have location completely
+ self-contained, but it's not the case now. */
update_global_location_list ();