From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17970 invoked by alias); 8 May 2008 01:22:33 -0000 Received: (qmail 17959 invoked by uid 22791); 8 May 2008 01:22:31 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 08 May 2008 01:22:10 +0000 Received: (qmail 19058 invoked from network); 8 May 2008 01:22:07 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 May 2008 01:22:07 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: remove global stop_bpstat dependency from breakpoints module Date: Thu, 08 May 2008 11:41:00 -0000 User-Agent: KMail/1.9.9 MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_8WlII/9WbamkAhg" Message-Id: <200805080222.04976.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-05/txt/msg00272.txt.bz2 --Boundary-00=_8WlII/9WbamkAhg Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 3540 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 --Boundary-00=_8WlII/9WbamkAhg Content-Type: text/x-diff; charset="utf-8"; name="refcount_bp_locations.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="refcount_bp_locations.diff" Content-length: 14753 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 (); --Boundary-00=_8WlII/9WbamkAhg--