Re: This a couple problems around PTRACE_EVENT_CLONE handling of signals in the new clone

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 10 October 2011 17:51:42, Pedro Alves wrote:
> This fixes two problems around PTRACE_EVENT_CLONE handling of
> signals in the new clone.
> 
>  - if some other signal != SIGSTOP comes out first on the new
>    clone (because a signal with a number < SIGSTOP was pending,
>    and the LWP dequeued it), we're currently just passing it on
>    to the inferior.  GDBserver will need fixing too, and even
>    has a comment to that effect:
> 
> 	    /* Pass the signal on.  This is what GDB does - except
> 	       shouldn't we really report it instead?  */
> 
>  - It's possible for the core to see the new thread/lwp, and
>    ask it to stop, queueing it a SIGSTOP, before we
>    have a chance of handling the PTHREAD_EVENT_CLONE event.
>    (e.g., non-stop; info threads; interrupt -a; while threads 
>    are spawning) (see comment in patch).  In that case, we'd
>    always resume the new thread, while the core would forever
>    expect it to report a stop.  I trigger this more easily with
>    some later patches that make the core stop all threads, rather
>    than the target.
> 
> Tested on x86_64-linux, and applied.

Actually, that patch was outdated (forgot quilt refresh).  This is
what I applied.  Only difference is in the debug output, I think.

-- 
Pedro Alves

2011-10-10  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* linux-nat.c (linux_handle_extended_wait): Don't resume the new
	new clone lwp if the core asked it to stop.  Don't pass on
	unexpected signals to the new clone; leave them pending instead.

---
 gdb/linux-nat.c |   68 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 24 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2011-10-10 16:19:27.187658836 +0100
+++ src/gdb/linux-nat.c	2011-10-10 17:38:18.857657965 +0100
@@ -2237,7 +2237,29 @@ linux_handle_extended_wait (struct lwp_i
 	      new_lp->signalled = 1;
 	    }
 	  else
-	    status = 0;
+	    {
+	      struct thread_info *tp;
+
+	      /* When we stop for an event in some other thread, and
+		 pull the thread list just as this thread has cloned,
+		 we'll have seen the new thread in the thread_db list
+		 before handling the CLONE event (glibc's
+		 pthread_create adds the new thread to the thread list
+		 before clone'ing, and has the kernel fill in the
+		 thread's tid on the clone call with
+		 CLONE_PARENT_SETTID).  If that happened, and the core
+		 had requested the new thread to stop, we'll have
+		 killed it with SIGSTOP.  But since SIGSTOP is not an
+		 RT signal, it can only be queued once.  We need to be
+		 careful to not resume the LWP if we wanted it to
+		 stop.  In that case, we'll leave the SIGSTOP pending.
+		 It will later be reported as TARGET_SIGNAL_0.  */
+	      tp = find_thread_ptid (new_lp->ptid);
+	      if (tp != NULL && tp->stop_requested)
+		new_lp->last_resume_kind = resume_stop;
+	      else
+		status = 0;
+	    }
 
 	  if (non_stop)
 	    {
@@ -2266,39 +2288,37 @@ linux_handle_extended_wait (struct lwp_i
 		}
 	    }
 
+	  if (status != 0)
+	    {
+	      /* We created NEW_LP so it cannot yet contain STATUS.  */
+	      gdb_assert (new_lp->status == 0);
+
+	      /* Save the wait status to report later.  */
+	      if (debug_linux_nat)
+		fprintf_unfiltered (gdb_stdlog,
+				    "LHEW: waitpid of new LWP %ld, "
+				    "saving status %s\n",
+				    (long) GET_LWP (new_lp->ptid),
+				    status_to_str (status));
+	      new_lp->status = status;
+	    }
+
 	  /* Note the need to use the low target ops to resume, to
 	     handle resuming with PT_SYSCALL if we have syscall
 	     catchpoints.  */
 	  if (!stopping)
 	    {
-	      enum target_signal signo;
-
-	      new_lp->stopped = 0;
 	      new_lp->resumed = 1;
-	      new_lp->last_resume_kind = resume_continue;
-
-	      signo = (status
-		       ? target_signal_from_host (WSTOPSIG (status))
-		       : TARGET_SIGNAL_0);
 
-	      linux_ops->to_resume (linux_ops, pid_to_ptid (new_pid),
-				    0, signo);
-	    }
-	  else
-	    {
-	      if (status != 0)
+	      if (status == 0)
 		{
-		  /* We created NEW_LP so it cannot yet contain STATUS.  */
-		  gdb_assert (new_lp->status == 0);
-
-		  /* Save the wait status to report later.  */
 		  if (debug_linux_nat)
 		    fprintf_unfiltered (gdb_stdlog,
-					"LHEW: waitpid of new LWP %ld, "
-					"saving status %s\n",
-					(long) GET_LWP (new_lp->ptid),
-					status_to_str (status));
-		  new_lp->status = status;
+					"LHEW: resuming new LWP %ld\n",
+					GET_LWP (new_lp->ptid));
+		  linux_ops->to_resume (linux_ops, pid_to_ptid (new_pid),
+					0, TARGET_SIGNAL_0);
+		  new_lp->stopped = 0;
 		}
 	    }