From patchwork Tue Jul 3 19:37:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 28215 Received: (qmail 2951 invoked by alias); 3 Jul 2018 19:38:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 2915 invoked by uid 89); 3 Jul 2018 19:38:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-spam-relays-external:74.125.82.67, corner, H*RU:74.125.82.67 X-HELO: mail-wm0-f67.google.com Received: from mail-wm0-f67.google.com (HELO mail-wm0-f67.google.com) (74.125.82.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Jul 2018 19:37:58 +0000 Received: by mail-wm0-f67.google.com with SMTP id l15-v6so11233240wmc.1 for ; Tue, 03 Jul 2018 12:37:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=fhhJetFpyuAAnS1Zln+jhrYn3AKje3Ugg161bZ8isK8=; b=YgoYDZXW5zZoc/UMk5MSKWWWZDPsGIZZI07DvAeV2UPpf4JB9zkq44BiareILuR5Ba QZb5AMisW52qrkUCy9SWTl+G4+Iej/uq5tekoI5888XY+wgkaeRabmWsiw0Ka3VgdY5p LKgb0qnov8P7waVgyZrMG+ueAF5Gdt9EC6NwWAS1WiyLfPH7rhJIVvqAsVABglq287vn rkzdZRQovL8JiNqyIF9/EpkuJ4IrDNC+2w2DfbzIino282mvg+FLpg4GhS2oU6OAmmqW qslrWr56q0Z2p6UrucL5p9gLRD3jcWVROELvxp7d0LyF5SJKZ87OHRfKX66D6rsD0TWv zwFQ== Return-Path: Received: from localhost ([176.12.107.132]) by smtp.gmail.com with ESMTPSA id e17-v6sm2658534wrr.85.2018.07.03.12.37.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 03 Jul 2018 12:37:55 -0700 (PDT) Date: Tue, 3 Jul 2018 20:37:53 +0100 From: Andrew Burgess To: Sergey Korolev Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] MIPS/GDB/linux-nat.c: Fix a child detach condition for uClibc-ng Message-ID: <20180703193753.GC2675@embecosm.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: Even a cabbage may look at a king. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * 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;