Re: [PATCH RFA] lin-lwp.c pending events.

[Mark Kettenis <kettenis at wins dot uva dot nl>]

   Date: Mon, 04 Jun 2001 20:29:01 -0700
   From: Michael Snyder <msnyder@cygnus.com>

   Mark, even though we are 'co-maintainers' of this code,
   I do like to get your approval and feedback.  Linus threads
   are so horrendously complicated, it's good to have two
   pairs of eyes.

Defenitely!

   This change catches the somewhat rare circumstance where GDB
   calls target_resume with a specific PTID, but then calls
   target_wait with a wild-card.  You could argue that GDB
   shouldn't do that, but it turns out that it would be very
   difficult to prevent it.  When it happens, we want to
   make sure that we don't try to handle a 'pending' event
   from a different LWP, because breakpoints may not be inserted
   and that different LWP will run away.

Sounds reasonable.  I'm not quite happy with your solution though: I'm
a bit allergic to global variables like solo_resume_pid.  I'd prefer
adding a flag to `struct lwp_info', say `resumed', that would record
whether the LWP has been resumed or not.  How about the attached patch?
In light of your remark above I think we should revert part of your
2001-05-25 patch.  The code dealing with newly attached threads in
lin_lwp_wait isn't used when we use the thread_db layer so I don't
think your patch had any effect anyway.  I left a comment in place.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* lin-lwp.c (struct lwp_info): Add member `resumed'.
	(iterate_over_lwps): Make sure we can handle CALLBACK deleting the
	LWP it's called for.
	(lin_lwp_attach): Mark LWP as resumed to make sure the fake
	SIGSTOP is reported.
	(resume_clear_callback): New function.
	(resume_set_callback): New function.
	(lin_lwp_resume): Mark all LWP's that we're going to resume as
	resumed, and unmark all others.
	(status_callback): Only report a pending wait status if we pretend
	that LP has been resumed.
	(resumed_callback): New function.
	(lin_lwp_wait): Add assertions to check that LWP's are properly
	marked as resumed.  Partially revert 2001-05-25 patch by Michael
	Snyder: do not resume all threads.  Add comment explaining the
	problems associated with this bit of code.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.23
diff -u -p -r1.23 lin-lwp.c
--- lin-lwp.c 2001/06/06 16:31:32 1.23
+++ lin-lwp.c 2001/06/06 19:45:09
@@ -82,6 +82,14 @@ struct lwp_info
   /* Non-zero if this LWP is stopped.  */
   int stopped;
 
+  /* Non-zero if this LWP will be/has been resumed.  Note that an LWP
+     can be marked both as stopped and resumed at the same time.  This
+     happens if we try to resume an LWP that has a wait status
+     pending.  We shouldn't let the LWP run until that wait status has
+     been processed, but we should not report that wait status if GDB
+     didn't try to let the LWP run.  */
+  int resumed;
+
   /* If non-zero, a pending wait status.  */
   int status;
 
@@ -249,11 +257,14 @@ find_lwp_pid (ptid_t ptid)
 struct lwp_info *
 iterate_over_lwps (int (*callback) (struct lwp_info *, void *), void *data)
 {
-  struct lwp_info *lp;
+  struct lwp_info *lp, *lpnext;
 
-  for (lp = lwp_list; lp; lp = lp->next)
-    if ((*callback) (lp, data))
-      return lp;
+  for (lp = lwp_list; lp; lp = lpnext)
+    {
+      lpnext = lp->next;
+      if ((*callback) (lp, data))
+	return lp;
+    }
 
   return NULL;
 }
@@ -357,6 +368,7 @@ lin_lwp_attach (char *args, int from_tty
 
   /* Fake the SIGSTOP that core GDB expects.  */
   lp->status = W_STOPCODE (SIGSTOP);
+  lp->resumed = 1;
 }
 
 static int
@@ -475,6 +487,20 @@ resume_callback (struct lwp_info *lp, vo
   return 0;
 }
 
+static int
+resume_clear_callback (struct lwp_info *lp, void *data)
+{
+  lp->resumed = 0;
+  return 0;
+}
+
+static int
+resume_set_callback (struct lwp_info *lp, void *data)
+{
+  lp->resumed = 1;
+  return 0;
+}
+
 static void
 lin_lwp_resume (ptid_t ptid, int step, enum target_signal signo)
 {
@@ -487,6 +513,11 @@ lin_lwp_resume (ptid_t ptid, int step, e
      processes, but give the signal only to this one'.  */
   resume_all = (PIDGET (ptid) == -1) || !step;
 
+  if (resume_all)
+    iterate_over_lwps (resume_set_callback, NULL);
+  else
+    iterate_over_lwps (resume_clear_callback, NULL);
+
   /* If PID is -1, it's the current inferior that should be
      handled specially.  */
   if (PIDGET (ptid) == -1)
@@ -500,6 +531,9 @@ lin_lwp_resume (ptid_t ptid, int step, e
       /* Remember if we're stepping.  */
       lp->step = step;
 
+      /* Mark this LWP as resumed.  */
+      lp->resumed = 1;
+
       /* If we have a pending wait status for this thread, there is no
          point in resuming the process.  */
       if (lp->status)
@@ -663,7 +697,9 @@ stop_wait_callback (struct lwp_info *lp,
 static int
 status_callback (struct lwp_info *lp, void *data)
 {
-  return (lp->status != 0);
+  /* Only report a pending wait status if we pretend that this has
+     indeed been resumed.  */
+  return (lp->status != 0 && lp->resumed);
 }
 
 /* Return non-zero if LP isn't stopped.  */
@@ -674,6 +710,14 @@ running_callback (struct lwp_info *lp, v
   return (lp->stopped == 0);
 }
 
+/* Return non-zero if LP has been resumed.  */
+
+static int
+resumed_callback (struct lwp_info *lp, void *data)
+{
+  return lp->resumed;
+}
+
 static ptid_t
 lin_lwp_wait (ptid_t ptid, struct target_waitstatus *ourstatus)
 {
@@ -691,6 +735,9 @@ lin_lwp_wait (ptid_t ptid, struct target
 
  retry:
 
+  /* Make sure there is at least one thread that has been resumed.  */
+  gdb_assert (iterate_over_lwps (resumed_callback, NULL));
+
   /* First check if there is a LWP with a wait status pending.  */
   if (pid == -1)
     {
@@ -754,6 +801,7 @@ lin_lwp_wait (ptid_t ptid, struct target
       child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step,
                     TARGET_SIGNAL_0);
       lp->stopped = 0;
+      gdb_assert (lp->resumed);
 
       /* This should catch the pending SIGSTOP.  */
       stop_wait_callback (lp, NULL);
@@ -840,6 +888,7 @@ lin_lwp_wait (ptid_t ptid, struct target
 	      child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step,
 	                    TARGET_SIGNAL_0);
 	      lp->stopped = 0;
+	      gdb_assert (lp->resumed);
 
 	      /* Discard the event.  */
 	      status = 0;
@@ -883,14 +932,13 @@ lin_lwp_wait (ptid_t ptid, struct target
 	  && signal_print_state (signo) == 0
 	  && signal_pass_state (signo) == 1)
 	{
-	  /* First mark this LWP as "not stopped", so that
-	     resume_callback will not resume it. */
-	  lp->stopped = 0;
-	  /* Resume all threads except this one
-	     (mainly to get the newly attached ones). */
-	  iterate_over_lwps (resume_callback, NULL);
-	  /* Now resume this thread, forwarding the signal to it. */
+	  /* FIMXE: kettenis/2001-06-06: Should we resume all threads
+             here?  It is not clear we should.  GDB may not expect
+             other threads to run.  On the other hand, not resuming
+             newly attached threads may cause an unwanted delay in
+             getting them running.  */
 	  child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step, signo);
+	  lp->stopped = 0;
 	  status = 0;
 	  goto retry;
 	}