[RFA] Support moribund breakpoint locations.

[Vladimir Prus <vladimir at codesourcery dot com>]

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