From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11587 invoked by alias); 1 Aug 2018 21:38:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 10643 invoked by uid 89); 1 Aug 2018 21:38:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=H*r:a0c, H*c:alternative X-HELO: mail-qt0-f169.google.com Received: from mail-qt0-f169.google.com (HELO mail-qt0-f169.google.com) (209.85.216.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 01 Aug 2018 21:38:06 +0000 Received: by mail-qt0-f169.google.com with SMTP id t5-v6so119963qtn.3 for ; Wed, 01 Aug 2018 14:38:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ndmsystems-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=9upo6iaNWzC+MoD6HGAlCxc6UQgrQ+B3BTLXuncXDnI=; b=H44F8uES6+7upTVLx0Y5MXECwM11uf7QQU61HTB6g8JCESczw4lXGNexd8Zr9x7ZeN t6+44R2AgrnRTu5ReYzDSrmWQhAu043+o5RsXDEYTgzaSHmNLL68/jlmsU40rdqeu1Yo oiTjyb2w/pvxP8nGDwTYoZoD06ZBRdFXwt4bKg0gS9y63QiMy0OLcvnNpnw8mgpbXMgz Az8TQQM4pD2x/YHaUVk/8BUerjfdEhkZb6ArLESDKoYo6dnZl8neBDHwBQMbUgv71hjv w9nTxy1XaFYVev9zplicRFUk9IkMINEH8wl98uGF4q4n+a/E5Us+K3XGVm2rV8Uc65Th c4MA== MIME-Version: 1.0 Received: by 2002:a0c:f691:0:0:0:0:0 with HTTP; Wed, 1 Aug 2018 14:37:24 -0700 (PDT) In-Reply-To: <20180801213129.GH3155@embecosm.com> References: <20180703193753.GC2675@embecosm.com> <20180801213129.GH3155@embecosm.com> From: Sergey Korolev Date: Wed, 01 Aug 2018 21:38:00 -0000 Message-ID: Subject: Re: Ping! Re: [PATCH] MIPS/GDB/linux-nat.c: Fix a child detach condition for uClibc-ng To: Andrew Burgess Cc: gdb-patches@sourceware.org, "Maciej W. Rozycki" Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00026.txt.bz2 Andrew, I would personally prefer your revised version due to more detailed description. On Thu, Aug 2, 2018 at 12:31 AM, Andrew Burgess 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 [2018-07-03 20:37:53 > +0100]: > > > * Sergey Korolev [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 > > >