From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com [209.85.221.49]) by sourceware.org (Postfix) with ESMTPS id 0D1933858400 for ; Fri, 26 Nov 2021 22:54:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0D1933858400 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f49.google.com with SMTP id d24so21663046wra.0 for ; Fri, 26 Nov 2021 14:54:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=B9jKta1XDeCZovdB3gDBWnYuhF75HLnsFNEkRYrMtVA=; b=ORLGWAWTqgsU/9p9zFm3sORN0eoSfQBQ49EOCqADCTcjLsJEZgx/gicJAVeAz9ugD8 /OuLsELBbjHWibOd45M7yeu3Pal3fBKRKObuzaDIUNnY8bIa7fnVop4idcSuROjLWX5Q x0oFUxhAQTzXCcfehuky/Wji+CexYDw2ZZKYcOiguQa2EBg/4T/hKjC19nkLiPu15T7e JsXYB/xAkumUmODPl7AfQUibG9YF5QYqjBG8OnkKhBAfLS3gxIe16lIZF5hFQHs9qy5R NMwDEJbiC0Wf3RyQSWlbcP9ic25A0mO5Qfm2Y1nIZoerVCKPls0YLpoUAEofN11hpWE9 xpGw== X-Gm-Message-State: AOAM533GWu5aeBNfgNeTC3QdVoVC9Ig4ye6q6UzyZ/Eb2mgvIyL/A9hF 6EOS2a7PlNuywMHhtJmAAwqoCjS/c+s= X-Google-Smtp-Source: ABdhPJxAZ/Csn2yAwC56wAsUUqf72u+acx5ARWfab5Y/XQdWUhPZS+0U0QiMhLRoLuBQPHkwDGneUw== X-Received: by 2002:a05:6000:252:: with SMTP id m18mr17181233wrz.117.1637967254117; Fri, 26 Nov 2021 14:54:14 -0800 (PST) Received: from ?IPV6:2001:8a0:f912:1a00:d3db:ac91:4b9e:1449? ([2001:8a0:f912:1a00:d3db:ac91:4b9e:1449]) by smtp.gmail.com with ESMTPSA id 4sm8711607wrz.90.2021.11.26.14.54.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 26 Nov 2021 14:54:13 -0800 (PST) Message-ID: Date: Fri, 26 Nov 2021 22:54:12 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Subject: Re: [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Content-Language: en-US To: 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: Pedro Alves In-Reply-To: <20211124200444.614978-4-simon.marchi@efficios.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, 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: Fri, 26 Nov 2021 22:54:16 -0000 On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote: > GDB-side, make the linux-nat and remote targets detach of fork children > known because of pending fork events. These pending fork events can be > stored in thread_info::pending_waitstatus, if the core has consumed the > event but then saved it for later. They can also be stored, for the > linux-nat target, in lwp_info::status and lwp_info::waitstatus. ... > For the > remote target, I suppose that a pending fork event could maybe be in the > stop reply queue, but I could generate a case where would could detach > while having pending events there, so I didn't handle that case in this > patch. 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. > There is a known limitation: we don't remove breakpoints from the > children before detaching it. So the children, could hit a trap > instruction after being detached and crash. I know this is wrong, and > it should be fixed, but I would like to handle that later. The current > patch doesn't fix everything, but it's a step in the right direction. *nod* > } > 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); > + 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. > + > + int fork_child_pid = fork_child_process->pid; > + > + if (detach_inferior (fork_child_process) != 0) > + warning (_("Failed to detach fork child %s, child of %s"), > + target_pid_to_str (ptid_t (fork_child_pid)).c_str (), > + target_pid_to_str (thread->id).c_str ()); > + } > + > if (detach_inferior (process) != 0) > write_enn (own_buf); > else >