2008-05-08 Pedro Alves * 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 ();