From: Sergey Korolev <s.korolev@ndmsystems.com>
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] MIPS/GDB/linux-nat.c: Fix a child detach condition for uClibc-ng
Date: Wed, 04 Jul 2018 13:36:00 -0000 [thread overview]
Message-ID: <CAKo4hFKqvM3C2XXhm+v=Y+3nCWmOsqb_Lx5K+WoNST57GCWD6w@mail.gmail.com> (raw)
In-Reply-To: <20180703193753.GC2675@embecosm.com>
Andrew,
thanks for your feedback. I agree that your explanation and variable names
are more descriptive.
I tested your patch in my environment and did not see any problems.
On Tue, Jul 3, 2018 at 10:37 PM, Andrew Burgess <andrew.burgess@embecosm.com
> wrote:
> * 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
>
>
next prev parent reply other threads:[~2018-07-04 13:36 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-14 22:54 Sergey Korolev
2018-07-03 19:38 ` Andrew Burgess
2018-07-04 13:36 ` Sergey Korolev [this message]
2018-07-05 12:32 ` Maciej W. Rozycki
2018-07-05 21:05 ` Sergey Korolev
2018-07-06 22:10 ` Andrew Burgess
2018-07-09 11:20 ` Maciej W. Rozycki
2018-08-01 21:31 ` Ping! " Andrew Burgess
2018-08-01 21:38 ` Sergey Korolev
2018-08-02 20:03 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKo4hFKqvM3C2XXhm+v=Y+3nCWmOsqb_Lx5K+WoNST57GCWD6w@mail.gmail.com' \
--to=s.korolev@ndmsystems.com \
--cc=andrew.burgess@embecosm.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).