[RFC] Support moribund breakpoint locations.

[Vladimir Prus <vladimir at codesourcery dot com>]

If we allow some threads to run while we interact with
some other stopped threads, we'll have some race condition
when removing breakpoints. Say we remove a breakpoint,
and communicate that to a target. However, as we do it,
some running thread hits the breakpoint we're removing.
It's somewhat hard to avoid this on the target, so the
question is how GDB will cope.

The simple solution is to do nothing -- in which case GDB
will report unexplained SIGTRAP. However, another solution
is possible. When we remove breakpoint location
from target, we should not forget about this location, but 
keep it for a while in a list of 'moribund' locations. When 
we get otherwise unexplained SIGTRAP, we'll check the list
of moribund locations and if we find the stopped address there,
silently continue.

The question is for how long to keep a location. Strictly
speaking, if we delete a location, we need to keep it until
every single thread reports an event. However, in real programs
some threads might be waiting for other threads forever, so
we'll never retire a location. I think a good practical
solution is to keep location until N inferior events are 
processed. This way we guarantee that a location won't be kept
forever. With N big enough, the chances of getting unexplained
SIGTRAP due to deleted breakpoint will be close to zero. And
since the problem here with due to threads, N better be proportional
to the number of threads. 

Attached is a patch that implements this idea. It's not directly
testable -- so I've tested it by making breakpoints never removed
from target, and making sure that hits of those breakpoints are
indeed ignored.

It would be best to handle moribund breakpoints only in
non-stop mode. This patch does not do that yet, but otherwise
I think it's ready.

Comments?

- 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 |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/breakpoint.h |   16 ++++++++++++++
 gdb/gdbthread.h  |    2 +
 gdb/infrun.c     |    2 +
 gdb/thread.c     |   12 ++++++++++
 5 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b25a62b..f9277eb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -299,6 +299,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;
@@ -2889,10 +2894,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)
   {
@@ -2961,6 +2968,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 */
 
@@ -2975,6 +2994,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))
@@ -3144,6 +3164,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:
@@ -6774,7 +6797,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);
 
@@ -6863,9 +6888,24 @@ update_global_location_list (void)
 	{
 	  /* FIXME? Handle error? */
 	  remove_breakpoint (loc, mark_uninserted);
+	  VEC_safe_push (bp_location_p, moribund_locations, loc);
+	  /* In non-stop mode, if we remove a breakpoint, 
+	     it's still possible that some threads will hit 
+	     it exactly when we're removing it, so we keep
+	     breakpoint for a bit.
+	     We'll 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;
 	}
-
-      if (!found_object)
+      else if (!found_object)
 	free_bp_location (loc);
     }
     
@@ -6877,6 +6917,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 1738e49..b3d21cc 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,
@@ -856,4 +867,9 @@ extern void breakpoint_restore_shadows (gdb_byte *buf,
 					ULONGEST memaddr, 
 					LONGEST len);
 
+/* 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 403e7e3..9a6b950 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -109,6 +109,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 1cd5e98..81e687d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1281,6 +1281,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 33c3f25..5710628 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -215,6 +215,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