From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 89C24385840B for ; Mon, 29 Nov 2021 18:28:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 89C24385840B Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 1ATIRwXV009494 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 29 Nov 2021 13:28:02 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 1ATIRwXV009494 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D20781EDF0; Mon, 29 Nov 2021 13:27:57 -0500 (EST) Message-ID: Date: Mon, 29 Nov 2021 13:27:57 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Content-Language: en-US To: Pedro Alves , Simon Marchi , gdb-patches@sourceware.org References: <20211029203332.69894-1-simon.marchi@polymtl.ca> <20211124200444.614978-1-simon.marchi@efficios.com> <20211124200444.614978-4-simon.marchi@efficios.com> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 29 Nov 2021 18:27:58 +0000 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2021 18:28:08 -0000 > Yes, this can certainly happen. I don't see a way to reliably stop the > program with an event held in the stop reply queue. GDB constantly drains the > events asynchronously. So for a testcase, I think you'd need to take > "constantly hammer" approach. Something like: > > - enable non-stop / target non-stop > - make the test program have a few threads constantly fork and exit the child > I say multiple threads so that the stop reply queue has a good chance of > holding an event unprocessed > - make the test program call alarm() to set a timeout. See why below. > - make the testcase constantly attach / continue -a & / detach > > Determining whether the fork child was detached correctly would be done by: > > - the parent/child sharing a pipe. You'd have an array of pipes, one > entry per thread. > - the child writing to the pipe before exiting > - the parent reading from the pipe after forking > > If GDB fails to detach the child, then the parent thread hangs in > the pipe read, and eventually, the parent gets a SIGALRM. > > To avoid the case of the test failing, then the parent dying with SIGALRM, and > GDB attaching to the wrong process (because the kernel reused the PID for > something else), you'd make the SIGALRM signal handler set a "timed_out" > global flag, and sleep for a while before exiting the process. Enough time > so that GDB can always reattach before the process disappears due to SIGALRM. > > When GDB reattaches, it first always consults that "timed_out" global, to > know whether the detach-from-children succeeded. If it is set, the test > FAILs. Ok, that sounds good (although not trivial!). >> } >> diff --git a/gdbserver/server.cc b/gdbserver/server.cc >> index 7963f2faf267..2f3d5341758f 100644 >> --- a/gdbserver/server.cc >> +++ b/gdbserver/server.cc >> @@ -1250,6 +1250,35 @@ handle_detach (char *own_buf) >> /* We'll need this after PROCESS has been destroyed. */ >> int pid = process->pid; >> >> + /* If this process has an unreported fork child, that child is not known to >> + GDB, so detach it as well. >> + >> + Here, we specifically don't want to use "safe iteration", as detaching >> + another process might delete the next thread in the iteration, which is >> + the one saved by the safe iterator. We will never delete the currently >> + iterated on thread, so standard iteration should be safe. */ >> + for (thread_info *thread : all_threads) >> + { >> + /* Only threads that are of the process we are detaching. */ >> + if (thread->id.pid () != pid) >> + continue; >> + >> + /* Only threads that have a pending fork event. */ >> + if (thread->fork_child == nullptr) >> + continue; >> + >> + process_info *fork_child_process >> + = find_process_pid (thread->fork_child->id.pid ()); > > This can be written as: > > process_info *fork_child_process > = get_thread_process (thread->fork_child); Done. > >> + gdb_assert (fork_child_process != nullptr); > > > Hmm, in my clone events work, I'm making clone events also set > the fork_parent/fork_child link, since clone events and fork/vfork > events are so similar. In this spot however, we'll need to skip > detaching the child if the child is a clone child, not a fork child. > I wonder how best to check that. See comments in the review of patch #1. Right. In my new versions, the loop has: /* Only threads that have a pending fork event. */ thread_info *child = target_thread_pending_child (thread); if (child == nullptr) continue; In your series, you'll make target_thread_pending_child return the kind, and you can add a check for the kind. Simon