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

Sergey Korolev
2018-08-01T21:38:10Z

Andrew,

I would personally prefer your revised version due to more detailed
description.

On Thu, Aug 2, 2018 at 12:31 AM, Andrew Burgess <andrew.burgess@embecosm.com
> wrote:

> 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
> >
>