remove global stop_bpstat dependency from breakpoints module

[Pedro Alves <pedro at codesourcery dot com>]

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 ();