Re: [PATCH] gdbserver: Set flag `suspended' properly for new lwp.

[Pedro Alves <palves at redhat dot com>]

On 05/07/2012 11:24 AM, Yao Qi wrote:

> @@ -461,6 +461,22 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
>  	 before calling linux_resume_one_lwp.  */
>        new_lwp->stopped = 1;
>  
> +      /* When `stopping_threads' is 1, it means GDBserver is stopping
> +	 all lwps (see stop_all_lwps).  So there is a new lwp added
> +	 during this all-lwps stopping, and its `suspended' flag should
> +	 be set carefully.  We are using EVENT_CHILD's `suspend' to
> +	 determine all lwps are stopped with suspended or stopped without
> +	 suspended.  Note that the lwp EXCEPT(the 2nd parameter of
> +	 stop_all_lwps)'s `suspended' flag can be zero, even GDBserver is
> +	 stop-with-suspend all lwp, but there should be no event for lwp
> +	 EXCEPT (that is to say, EXCEPT != EVENT_CHILD), because it is
> +	 not SIGSTOP'ed at all.  We are correct here.
> +	 Even GDBserver stop-without-suspend all lwps, but EVENT_CHILD's
> +	 `suspended' flag is non-zero, we'll set new lwp's `suspended to
> +	 1.  It also looks correct.  '*/

> +      if (stopping_threads)

> +	new_lwp->suspended = event_child->suspended ? 1 : 0;


This looks correct, but I had some trouble groking the comment.

I think doing it in this form makes it harder to understand and justify.

I went to so many different strategies for this step-over/suspending
mechanism, that I sometimes confuse myself on the properties of the
version that I ended up with, but I think that nested stop_all_lwps
where the inner stop_all_lwps starts before the outer stop_all_lwps
finishes stopping all threads _can't_ happen nowadays.  (I had
at one point tracepoints be collected immediately if a thread would
hit a tracepoint while stop_all_threads was trying to stop it,
leading to such scenarios, and it was hard to impossible to
get right.)  In that case, checking event_child->suspended would be
wrong, and we'd need to set new_lwp->suspended to the current
nested level of suspension.  Ugh.

But if nothing leads to a stop_all_lwps call while another is
already in effect, then it is sufficient to know whether we're presently
suspending threads, and if yes, it is right to set the new lwp's
suspend count to 1 (instead of for example the suspend count of the
event_child).

That is effectively what you have, but in a roundabout way, I think.

There's a simpler way.  We're already checking the `stopping_threads'
global.  `stopping_threads' is exclusively set by stop_all_lwps.  So we
_already_ use global state to let leaf code know what stop_all_lwps
is going.  Which leads me to the variant of the patch below.

WDYT?

2012-05-10  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	* linux-low.c (enum stopping_threads_kind): New.
	(stopping_threads): Change type to `enum stopping_threads_kind'.
	(handle_extended_wait): If stopping and suspending threads, leave
	the new_lwp suspended too.
	(linux_wait_for_event): Adjust.
	(stop_all_lwps): Set `stopping_threads' to
	STOPPING_AND_SUSPENDING_THREADS or STOPPING_THREADS depending on
	whether we're suspending threads or just stopping them.
---

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bbb0693..ed01255 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -172,8 +172,20 @@ pull_pid_from_list (struct simple_pid_list **listp, int pid, int *statusp)
   return 0;
 }

-/* FIXME this is a bit of a hack, and could be removed.  */
-int stopping_threads;
+enum stopping_threads_kind
+  {
+    /* Not stopping threads presently.  */
+    NOT_STOPPING_THREADS,
+
+    /* Stopping threads.  */
+    STOPPING_THREADS,
+
+    /* Stopping and suspending threads.  */
+    STOPPING_AND_SUSPENDING_THREADS
+  };
+
+/* This is set while stop_all_lwps is in effect.  */
+enum stopping_threads_kind stopping_threads = NOT_STOPPING_THREADS;

 /* FIXME make into a target method?  */
 int using_threads = 1;
@@ -461,12 +473,17 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
 	 before calling linux_resume_one_lwp.  */
       new_lwp->stopped = 1;

+     /* If we're suspending all threads, leave this one suspended
+	too.  */
+      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
+	new_lwp->suspended = 1;
+
       /* Normally we will get the pending SIGSTOP.  But in some cases
 	 we might get another signal delivered to the group first.
 	 If we do get another signal, be sure not to lose it.  */
       if (WSTOPSIG (status) == SIGSTOP)
 	{
-	  if (stopping_threads)
+	  if (stopping_threads != NOT_STOPPING_THREADS)
 	    new_lwp->stop_pc = get_stop_pc (new_lwp);
 	  else
 	    linux_resume_one_lwp (new_lwp, 0, 0, NULL);
@@ -475,7 +492,7 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
 	{
 	  new_lwp->stop_expected = 1;

-	  if (stopping_threads)
+	  if (stopping_threads != NOT_STOPPING_THREADS)
 	    {
 	      new_lwp->stop_pc = get_stop_pc (new_lwp);
 	      new_lwp->status_pending_p = 1;
@@ -1815,7 +1832,7 @@ linux_wait_for_event (ptid_t ptid, int *wstat, int options)
     {
       requested_child = find_lwp_pid (ptid);

-      if (!stopping_threads
+      if (stopping_threads == NOT_STOPPING_THREADS
 	  && requested_child->status_pending_p
 	  && requested_child->collecting_fast_tracepoint)
 	{
@@ -1963,7 +1980,7 @@ linux_wait_for_event (ptid_t ptid, int *wstat, int options)
 	  event_child->stop_expected = 0;

 	  should_stop = (current_inferior->last_resume_kind == resume_stop
-			 || stopping_threads);
+			 || stopping_threads != NOT_STOPPING_THREADS);

 	  if (!should_stop)
 	    {
@@ -3072,14 +3089,16 @@ lwp_running (struct inferior_list_entry *entry, void *data)
 static void
 stop_all_lwps (int suspend, struct lwp_info *except)
 {
-  stopping_threads = 1;
+  stopping_threads = (suspend
+		      ? STOPPING_AND_SUSPENDING_THREADS
+		      : STOPPING_THREADS);

   if (suspend)
     find_inferior (&all_lwps, suspend_and_send_sigstop_callback, except);
   else
     find_inferior (&all_lwps, send_sigstop_callback, except);
   for_each_inferior (&all_lwps, wait_for_sigstop);
-  stopping_threads = 0;
+  stopping_threads = NOT_STOPPING_THREADS;
 }

 /* Resume execution of the inferior process.