Re: [PATCH] Handle signals sent to a fork/vfork child before it has a chance to first run (Re: [PATCH] Fix gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw) (timeout) with Linux 2.6.32 and older version)

[Hui Zhu <hui_zhu@mentor.com>]

On 07/05/14 01:50, Pedro Alves wrote:
> On 07/03/2014 05:24 PM, Hui Zhu wrote:
>
>> After this patch, I still got fail with this test in Linux 2.6.32.
>> The cause this:
>> 	      signo = WSTOPSIG (status);
>> #In linux 2.6.32, signo will be SIGSTOP.
>> 	      if (signo != 0
>> 		  && !signal_pass_state (gdb_signal_from_host (signo)))
>> 		signo = 0;
>> #SIGSTOP will send to child and make it stop.
>> 	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
>>
>> So I make a patch to fix it.
>>
>> Thanks,
>> Hui
>>
>> 2014-07-04  Hui Zhu  <hui@codesourcery.com>
>>
>> 	* linux-nat.c(linux_child_follow_fork): Add check if signo
>> 	is SIGSTOP.
>>
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -472,8 +472,9 @@ holding the child stopped.  Try \"set de
>>    	      int signo;
>>
>>    	      signo = WSTOPSIG (status);
>> -	      if (signo != 0
>> -		  && !signal_pass_state (gdb_signal_from_host (signo)))
>> +	      if (signo == SIGSTOP
>> +		  || (signo != 0
>> +		      && !signal_pass_state (gdb_signal_from_host (signo))))
>>    		signo = 0;
>>    	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
>>    	    }
>>
>
> Hmm.  We should actually be passing the signal the fork child had stopped
> for earlier, when we step it for the workaround.  But we lose that signal...
>
> /me hacks.
>
> Does this patch fix the issue too ?
>
> 8<-----------
>  From 4184b4d0ffca89776d06d6b7d1241e9c0d01017a Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 4 Jul 2014 17:31:41 +0100
> Subject: [PATCH] Handle signals sent to a fork/vfork child before it has a
>   chance to first run
>
> This is a WIP.
>
> We handle this for clone, but not for fork/vfork.  For clone it's
> easier as we just store the status in the new child's lwp immediately.
> But for fork/vfork we don't add the child to the list until the next
> resume, when we either follow or detach the child, depending on
> follow-fork mode, in linux_child_follow_fork.  So store the child's
> status in a new simple_pid_list list.
>
> I haven't tried to write a test yet.  I think we have one for clones
> somewhere, which we can model on.
>
> Tested on x86_64 Fedora 20 with no regressions.

Hi Pedro,

Thanks for your work.
The patch fixed the issue and pass regression test in Ubuntu 10.04 that 
use Linux 2.6.32.

Best,
Hui


>
> gdb/
> 2014-07-04  Pedro Alves  <palves@redhat.com>
>
> 	* linux-nat.c (forked_pids): New global.
> 	(save_new_child_stop_status): New function.
> 	(get_status_pass_signal): New function.
> 	(linux_child_follow_fork): Retrieve the child's stop status from
> 	the forked pids list, and pass the signal to the child if
> 	detaching from it, or leave it pending if not detaching.
> 	(cancel_pending_sigstop): New function, factored out from
> 	detach_callback.
> 	(detach_callback): Adjust to use cancel_pending_sigstop.
> 	(linux_handle_extended_wait): Store the fork/vfork child's
> 	status.  Adjust comment.
> ---
>   gdb/linux-nat.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 118 insertions(+), 25 deletions(-)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 0ab0362..1435079 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -223,6 +223,10 @@ struct simple_pid_list
>   };
>   struct simple_pid_list *stopped_pids;
>
> +/* The status of forked pids we've already seen the
> +   PTRACE_EVENT_FORK/VFORK event for, but haven't followed yet.  */
> +static struct simple_pid_list *forked_pids;
> +
>   /* Async mode support.  */
>
>   /* The read/write ends of the pipe registered as waitable file in the
> @@ -371,12 +375,71 @@ delete_lwp_cleanup (void *lp_voidp)
>     delete_lwp (lp->ptid);
>   }
>
> +/* CHILD_LP is a new clone/fork child that stopped with STATUS.  If
> +   STATUS is not SIGSTOP, store the status for later, and mark the lwp
> +   as signalled (there's a SIGSTOP pending in the kernel queue).  This
> +   can happen if someone starts sending signals to the new thread
> +   before it gets a chance to run, which have a lower number than
> +   SIGSTOP (e.g. SIGUSR1).  */
> +
> +static void
> +save_new_child_stop_status (struct lwp_info *child_lp, int *status_p)
> +{
> +  int status = *status_p;
> +
> +  if (status != 0)
> +    {
> +      gdb_assert (WIFSTOPPED (status));
> +
> +      if (WSTOPSIG (status) != SIGSTOP)
> +	{
> +	  child_lp->signalled = 1;
> +
> +	  /* Save the wait status to report later.  */
> +	  if (debug_linux_nat)
> +	    fprintf_unfiltered (gdb_stdlog,
> +				"SCSS: waitpid of new LWP %ld, "
> +				"saving status %s\n",
> +				ptid_get_lwp (child_lp->ptid),
> +				status_to_str (status));
> +	}
> +      else
> +	status = 0;
> +    }
> +
> +  child_lp->status = status;
> +  *status_p = status;
> +}
> +
> +/* We're about to detach from a child that had stopped with STATUS.
> +   Return the host signal number to pass, if any.  */
> +
> +static int
> +get_pass_signal_from_status (int status)
> +{
> +  if (status != 0 && WIFSTOPPED (status))
> +    {
> +      int signo;
> +
> +      signo = WSTOPSIG (status);
> +      if (signo != 0
> +	  && !signal_pass_state (gdb_signal_from_host (signo)))
> +	signo = 0;
> +      return signo;
> +    }
> +
> +  return 0;
> +}
> +
> +static void cancel_pending_sigstop (struct lwp_info *lp);
> +
>   static int
>   linux_child_follow_fork (struct target_ops *ops, int follow_child,
>   			 int detach_fork)
>   {
>     int has_vforked;
>     int parent_pid, child_pid;
> +  int child_status;
>
>     has_vforked = (inferior_thread ()->pending_follow.kind
>   		 == TARGET_WAITKIND_VFORKED);
> @@ -404,6 +467,11 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>         return 1;
>       }
>
> +  /* If we haven't already seen the new PID stop, wait for it now.  */
> +  if (!pull_pid_from_list (&forked_pids, child_pid, &child_status))
> +    internal_error (__FILE__, __LINE__,
> +		    _("forked pid %d not found in forked list"), child_pid);
> +
>     if (! follow_child)
>       {
>         struct lwp_info *child_lp = NULL;
> @@ -414,7 +482,6 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>         if (detach_fork)
>   	{
>   	  struct cleanup *old_chain;
> -	  int status = W_STOPCODE (0);
>
>   	  /* Before detaching from the child, remove all breakpoints
>   	     from it.  If we forked, then this has already been taken
> @@ -444,8 +511,12 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	  child_lp = add_lwp (inferior_ptid);
>   	  child_lp->stopped = 1;
>   	  child_lp->last_resume_kind = resume_stop;
> +	  save_new_child_stop_status (child_lp, &child_status);
>   	  make_cleanup (delete_lwp_cleanup, child_lp);
>
> +	  /* If there is a pending SIGSTOP, get rid of it.  */
> +	  cancel_pending_sigstop (child_lp);
> +
>   	  if (linux_nat_prepare_to_resume != NULL)
>   	    linux_nat_prepare_to_resume (child_lp);
>
> @@ -455,26 +526,26 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	     process starts with the TIF_SINGLESTEP/X86_EFLAGS_TF bits
>   	     set if the parent process had them set.
>   	     To work around this, single step the child process
> -	     once before detaching to clear the flags.  */
> +	     once before detaching to clear the flags.
> +	     Be careful not to lose any signal though.  */
>
>   	  if (!gdbarch_software_single_step_p (target_thread_architecture
> -						   (child_lp->ptid)))
> +					       (child_lp->ptid)))
>   	    {
> +	      int signo = get_pass_signal_from_status (child_status);
> +
>   	      linux_disable_event_reporting (child_pid);
> -	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
> +	      signo = get_pass_signal_from_status (child_status);
> +	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, signo) < 0)
>   		perror_with_name (_("Couldn't do single step"));
> -	      if (my_waitpid (child_pid, &status, 0) < 0)
> +	      if (my_waitpid (child_pid, &child_status, 0) < 0)
>   		perror_with_name (_("Couldn't wait vfork process"));
>   	    }
>
> -	  if (WIFSTOPPED (status))
> +	  if (child_status != 0 && WIFSTOPPED (child_status))
>   	    {
> -	      int signo;
> +	      int signo = get_pass_signal_from_status (child_status);
>
> -	      signo = WSTOPSIG (status);
> -	      if (signo != 0
> -		  && !signal_pass_state (gdb_signal_from_host (signo)))
> -		signo = 0;
>   	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
>   	    }
>
> @@ -502,6 +573,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	  child_lp = add_lwp (inferior_ptid);
>   	  child_lp->stopped = 1;
>   	  child_lp->last_resume_kind = resume_stop;
> +	  save_new_child_stop_status (child_lp, &child_status);
>   	  child_inf->symfile_flags = SYMFILE_NO_READ;
>
>   	  /* If this is a vfork child, then the address-space is
> @@ -696,6 +768,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>         child_lp = add_lwp (inferior_ptid);
>         child_lp->stopped = 1;
>         child_lp->last_resume_kind = resume_stop;
> +      save_new_child_stop_status (child_lp, &child_status);
>
>         /* If this is a vfork child, then the address-space is shared
>   	 with the parent.  If we detached from the parent, then we can
> @@ -1510,27 +1583,41 @@ get_pending_status (struct lwp_info *lp, int *status)
>     return 0;
>   }
>
> -static int
> -detach_callback (struct lwp_info *lp, void *data)
> -{
> -  gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
> -
> -  if (debug_linux_nat && lp->status)
> -    fprintf_unfiltered (gdb_stdlog, "DC:  Pending %s for %s on detach.\n",
> -			strsignal (WSTOPSIG (lp->status)),
> -			target_pid_to_str (lp->ptid));
> +/* If LP was signalled, can't the pending SIGSTOP with a SIGCONT.  */
>
> +static void
> +cancel_pending_sigstop (struct lwp_info *lp)
> +{
>     /* If there is a pending SIGSTOP, get rid of it.  */
>     if (lp->signalled)
>       {
> +      /* This can happen if someone starts sending signals to
> +	 the new thread before it gets a chance to run, which
> +	 have a lower number than SIGSTOP (e.g. SIGUSR1).  */
> +
> +      /* There is a pending SIGSTOP; get rid of it.  */
>         if (debug_linux_nat)
>   	fprintf_unfiltered (gdb_stdlog,
> -			    "DC: Sending SIGCONT to %s\n",
> +			    "Sending SIGCONT to %s\n",
>   			    target_pid_to_str (lp->ptid));
>
>         kill_lwp (ptid_get_lwp (lp->ptid), SIGCONT);
>         lp->signalled = 0;
>       }
> +}
> +
> +static int
> +detach_callback (struct lwp_info *lp, void *data)
> +{
> +  gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
> +
> +  if (debug_linux_nat && lp->status)
> +    fprintf_unfiltered (gdb_stdlog, "DC:  Pending %s for %s on detach.\n",
> +			strsignal (WSTOPSIG (lp->status)),
> +			target_pid_to_str (lp->ptid));
> +
> +  /* If there is a pending SIGSTOP, get rid of it.  */
> +  cancel_pending_sigstop (lp);
>
>     /* We don't actually detach from the LWP that has an id equal to the
>        overall process id just yet.  */
> @@ -2061,6 +2148,14 @@ linux_handle_extended_wait (struct lwp_info *lp, int status,
>   	  return 0;
>   	}
>
> +      if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> +	{
> +	  /* The child may have stopped with a signal other than
> +	     SIGSTOP.  Store the status for later, so we don't lose
> +	     the signal.  */
> +	  add_to_pid_list (&forked_pids, new_pid, status);
> +	}
> +
>         if (event == PTRACE_EVENT_FORK)
>   	ourstatus->kind = TARGET_WAITKIND_FORKED;
>         else if (event == PTRACE_EVENT_VFORK)
> @@ -2086,10 +2181,8 @@ linux_handle_extended_wait (struct lwp_info *lp, int status,
>   	      /* This can happen if someone starts sending signals to
>   		 the new thread before it gets a chance to run, which
>   		 have a lower number than SIGSTOP (e.g. SIGUSR1).
> -		 This is an unlikely case, and harder to handle for
> -		 fork / vfork than for clone, so we do not try - but
> -		 we handle it for clone events here.  We'll send
> -		 the other signal on to the thread below.  */
> +		 We'll send the other signal on to the thread
> +		 below.  */
>
>   	      new_lp->signalled = 1;
>   	    }
>