Ping! Re: [PATCH] MIPS/GDB/linux-nat.c: Fix a child detach condition for uClibc-ng

[Andrew Burgess <andrew.burgess@embecosm.com>]

Ping!

I would propose my version of the patch, but one of the two versions
should probably be merged.

Original patch:
    https://sourceware.org/ml/gdb-patches/2018-06/msg00382.html

My revised proposal:
    https://sourceware.org/ml/gdb-patches/2018-07/msg00061.html

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-07-03 20:37:53 +0100]:

> * Sergey Korolev <s.korolev@ndmsystems.com> [2018-06-15 01:54:12 +0300]:
> 
> > Current implementation expects that WIFSTOPPED (W_STOPCODE (0)) is true,
> > but in the uClibc-ng it is false on MIPS. The patch adds a "detach"
> > helper variable to avoid this corner case: WIFSTOPPED applied
> > only to a status filled by a waitpid call that should never return
> > the status with zero stop signal.
> 
> I took a quick look through this patch, and have a little feedback.
> 
> You might want to expand your commit message to explain _why_ MIPS is
> different (having a signal 127), I didn't know this, and it puzzled me
> for a while as to why your above text didn't just indicate a bug in
> uClibc-ng :)
> 
> > ---
> >  gdb/linux-nat.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> > index 445b59fa4a..916de2d335 100644
> > --- a/gdb/linux-nat.c
> > +++ b/gdb/linux-nat.c
> > @@ -468,6 +468,7 @@ linux_nat_target::follow_fork (int follow_child, int
> > detach_fork)
> >        /* Detach new forked process?  */
> >        if (detach_fork)
> >   {
> > +   int detach = 1;
> 
> There's already a detach_fork variable in this function, so maybe a
> more descriptive name would make thing clearer.
> 
> >     struct cleanup *old_chain = make_cleanup (delete_lwp_cleanup,
> >       child_lp);
> > 
> > @@ -492,9 +493,11 @@ linux_nat_target::follow_fork (int follow_child, int
> > detach_fork)
> >   perror_with_name (_("Couldn't do single step"));
> >         if (my_waitpid (child_pid, &status, 0) < 0)
> >   perror_with_name (_("Couldn't wait vfork process"));
> > +       else
> > + detach = WIFSTOPPED (status);
> >       }
> > 
> > -   if (WIFSTOPPED (status))
> > +   if (detach)
> >       {
> >         int signo;
> 
> I wonder if we should move the status into the scope of the call to
> WIFSTOPPED, and avoid making any calls on it unless we know it has
> been filled in.
> 
> Such a thing might look like this:
> 
> 
> [PATCH] gdb: Avoid using W_STOPCODE(0) as this is ambiguous on MIPS
> 
> The MIPS target supports 127 signals, and this can create an ambiguity
> in process wait statuses.  A status value of 0x007f could potentially
> indicate a process that has exited with signal 127, or a process that
> has stopped with signal 0.
> 
> In uClibc-ng the interpretation of 0x007f is that the process has
> exited with signal 127 rather than stopped with signal 0, and so,
> WIFSTOPPED (W_STOPCODE (0)) will be false rather than true as it would
> be on most other platforms.
> 
> Given that it's pretty easy to avoid using W_STOPCODE (0), lets do that.
> 
> gdb/ChangeLog:
> 
> 	* linux-nat.c (linux_nat_target::follow_fork): Avoid using
> 	'W_STOPCODE (0)' as this could be ambiguous.
> ---
>  gdb/ChangeLog   |  6 ++++++
>  gdb/linux-nat.c | 15 +++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index af38a2a1a5b..4de19770f42 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -448,7 +448,6 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
>    if (!follow_child)
>      {
>        struct lwp_info *child_lp = NULL;
> -      int status = W_STOPCODE (0);
>        int has_vforked;
>        ptid_t parent_ptid, child_ptid;
>        int parent_pid, child_pid;
> @@ -468,6 +467,8 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
>        /* Detach new forked process?  */
>        if (detach_fork)
>  	{
> +	  int child_stop_signal = 0;
> +	  bool detach_child = true;
>  	  struct cleanup *old_chain = make_cleanup (delete_lwp_cleanup,
>  						    child_lp);
>  
> @@ -487,18 +488,24 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
>  	  if (!gdbarch_software_single_step_p (target_thread_architecture
>  					       (parent_ptid)))
>  	    {
> +	      int status;
> +
>  	      linux_disable_event_reporting (child_pid);
>  	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
>  		perror_with_name (_("Couldn't do single step"));
>  	      if (my_waitpid (child_pid, &status, 0) < 0)
>  		perror_with_name (_("Couldn't wait vfork process"));
> +	      else
> +		{
> +		  detach_child = WIFSTOPPED (status);
> +		  child_stop_signal = WSTOPSIG (status);
> +		}
>  	    }
>  
> -	  if (WIFSTOPPED (status))
> +	  if (detach_child)
>  	    {
> -	      int signo;
> +	      int signo = child_stop_signal;
>  
> -	      signo = WSTOPSIG (status);
>  	      if (signo != 0
>  		  && !signal_pass_state (gdb_signal_from_host (signo)))
>  		signo = 0;
> -- 
> 2.14.4
>