From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [96.47.72.81]) by sourceware.org (Postfix) with ESMTPS id 131843858C54 for ; Mon, 27 Mar 2023 20:25:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 131843858C54 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4Plknl55dXz45bL; Mon, 27 Mar 2023 20:24:59 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Plknl4BKhz3LyL; Mon, 27 Mar 2023 20:24:59 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1679948699; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Nhj4AOCTMqOKqqgbQOV518dguF+e585hqdnMbjAF4Xw=; b=JH2jxtpFal/xYuAadRGdD2nDrHACDL9d2Mldzh5VyDyDKzpo650qMLASx2ziBoqwJFonCo CN3vsFba74cOJe126THdXtUaa0rrUSNpcU4bQgew7YQl7nfHJtn+YxwGtkocM6UVnkw5qd TvrhpQNNDDmY1xWSRS29qVKzFgIuHddIay5Sste0Mp3eqtgXPvTQNt/4PHG3w0RHKCt+ZP 6pirME6M3427AYhBMP/7b7vKua6tTk//J/Yvaet3RkfprvU+iFkdeMwAvCaCpn66RQrC+n gxzgQ8SD6RLU0vMxEH6etPhsuRMmdj0QnES6PwyGE6mY0zaedLmGwqU/9GQxVQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1679948699; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Nhj4AOCTMqOKqqgbQOV518dguF+e585hqdnMbjAF4Xw=; b=ftTv8Okyq/pv3q0We77inrXMIv+QUsVVJpyifq3Ok9uDubXMr++X7hO6U8PWaisTDDrsqO vG6LiH0z3WqpW8ZmR6yIqUKmSMaG+FXqZgdvPoqnS9HPqXNTRHuAhEP1Q6BQEneadG+O6T ORyfU/GmLdSuFqNfyU1+Or+BsOFBAsfOJCFB2g9NJrvOm768uZcPgvwZ2xOOzfv3Kovf29 C9oVLejJv+nbcaihn5XevNJoyrWXsj/6OllX8J1zbSl2b+nKGFKfKGf/EbwJj9gkltu0jN ASbWZ72thqiAsjUUTvrfg4IIp8tU4TQRRrJW/ADB4uKmYS+uK98abVuhnxX2CA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1679948699; a=rsa-sha256; cv=none; b=WJ5DkbsH6VLqSRJDF0kKjl6S+4KYsZwZ/81uJWnXSoXBp3naBmYMkWd4ZfWvRAwuPKFaGQ iGGJDWkDjm4BwoyONxsosQzBs83eH1eKRKO8YhU9CwmY6MuekQMEdct4fkwHWJfp8z/rXn wQ97jfKulrk/bYLjVTOzd5qTM4V6NX/hmWPcGsk9Ijp6v73Ln+9ELq8LFAxxJeiSRjjfKl TMQZmqqWJo49e3NL+afds0TmzpXkJbTizfbUOQsrd8827kHjSg+8m+WNKaQTyjriTumJy9 YbfbixMjIW5WdyY5bSWTVWpdAyyhre0H8nHbPyN8TsW3J+5drJy0zCWQWFfivg== Received: from [IPV6:2601:648:8680:16b0:c10c:3358:4516:c03f] (unknown [IPv6:2601:648:8680:16b0:c10c:3358:4516:c03f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4Plknl1HwCzK4g; Mon, 27 Mar 2023 20:24:59 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <1f5bad14-378e-47df-5386-b2c48710cf64@FreeBSD.org> Date: Mon, 27 Mar 2023 13:24:57 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org References: <20230228181845.99936-1-jhb@FreeBSD.org> <20230228181845.99936-5-jhb@FreeBSD.org> <8e52f898-c853-df46-52a8-6a4a4c292c5c@simark.ca> From: John Baldwin Subject: Re: [PATCH 4/9] fbsd-nat: Add a list of pending events. In-Reply-To: <8e52f898-c853-df46-52a8-6a4a4c292c5c@simark.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/20/23 11:35 AM, Simon Marchi wrote: > On 2/28/23 13:18, John Baldwin wrote: >> The pending_events list stores a queue of deferred events that might >> be reported by the next call to the target's wait method. The set of >> events that are eligible is filtered by the ptid passed to resume. >> >> For now this just replaces the list of vfork_done events. A >> subsequent commit will reuse this to store other events. >> --- >> gdb/fbsd-nat.c | 146 ++++++++++++++++++++++++++++++++----------------- >> gdb/fbsd-nat.h | 2 + >> 2 files changed, 98 insertions(+), 50 deletions(-) >> >> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c >> index 04d67fc5278..ca278b871ef 100644 >> --- a/gdb/fbsd-nat.c >> +++ b/gdb/fbsd-nat.c >> @@ -54,6 +54,66 @@ >> #define PT_SETREGSET 43 /* Set a target register set */ >> #endif >> >> +/* Filter for ptid's allowed to report events from wait. Normally set >> + in resume, but also reset to minus_one_ptid in create_inferior and >> + attach. */ >> + >> +static ptid_t resume_ptid; > > It doesn't change anything conceptually, but I think this should be a > private field of fbsd_nat_target. This does mean making all the *pending_event functions private methods of the class or using friend in various ways. It also doesn't exist at the end of the series. > This resume_ptid approach might work now, but it won't work in general, > since it only records the scope ptid passed to the last call to resume. > > Even today, do you support multi-inferior? If the user does: > > (gdb) inferior 1 > (gdb) continue & > (gdb) inferior 2 > (gdb) continue & > > Then resume_ptid would be (inf2_pid, 0, 0), and you'd be ignoring any > event from inferior 1, right? > > I think the general solution is necessarily to remember the > resume state for each individual thread, somehow. Up to you to see if > you want to implement that now or defer to later. If you only support > all-stop with a single inferior, it might be enough for now. > > For reference, the linux-nat target maintains its own data structures of > lwps (lwp_lwpid_htab), in which the last_resume_kind field is used for > that. It would be unfortunate to have to reimplement all of this for > fbsd-nat as I'm sure that a lot of the logic would be very similar. But > then making the code shareable would also be a lot of work. Yes, patch 6 reworks this further to properly suport multi-process where it tracks this type of state per-process instead of globally. I could squash them all together perhaps but was trying to incrementally solve multiple problems and trying to keep the patches smaller to ease review. In particular, patch 5 aims to solve the problem of a new thread showing up in a process being single-stepped where events are only expected for the thread being stepped. >> + >> +/* If an event is triggered asynchronously (fake vfork_done events) or >> + occurs when the core is not expecting it, a pending event is >> + created. This event is then returned by a future call to the >> + target wait method. */ > > It can probably also happen if two events happen at the "same" time? > Like if you react to a breakpoint hit by one thread, and while stopping > the other threads, another thread reports a breakpoint stop, you have to > store that one as a pending event? With the caveat that on FreeBSD (and I think on other BSDs) you'd have to have those threads be in separate processes, but yes. This patch aims to try to make the pending vfork events list more generic so it can be used to hold other events. >> + >> +struct pending_event >> +{ >> + pending_event (const ptid_t &_ptid, const target_waitstatus &_status) : >> + ptid(_ptid), status(_status) {} > > Spaces after parentheses. Fixed (along with other style bugs you pointed out) >> + >> + ptid_t ptid; >> + target_waitstatus status; >> +}; >> + >> +static std::list pending_events; > > This could use intrusive_list, which is a bit more lightweight than > std::list (it won't really make a difference in practice, but we have > it, so we might as well use it). Note though that this is a list of objects, not a list of pointers, so as a std::list the objects are embedded in a single allocation along with the pointers (IIUC) (e.g. the current code uses emplace_back to construct the object in place in the list node). In order to use an intrusive list, the code would now have to explicitly new an object before doing a push_back, and take_pending_event would have to explicitly delete the object after the call to erase(). >> + matching event, PTID and *STATUS contain the event details, and the >> + event is removed from the pending list. */ >> + >> +static bool >> +take_pending_event (ptid_t &ptid, target_waitstatus *status) >> +{ >> + for (auto it = pending_events.begin (); it != pending_events.end (); it++) >> + if (it->ptid.matches (resume_ptid)) >> + { >> + ptid = it->ptid; >> + *status = it->status; >> + pending_events.erase (it); >> + return true; >> + } >> + return false; >> +} > > This could maybe return a gdb::optional? Ok. >> @@ -1061,47 +1121,20 @@ fbsd_is_child_pending (pid_t pid) >> } >> >> #ifndef PTRACE_VFORK >> -static std::forward_list fbsd_pending_vfork_done; >> - >> /* Record a pending vfork done event. */ >> >> static void >> fbsd_add_vfork_done (ptid_t pid) >> { >> - fbsd_pending_vfork_done.push_front (pid); >> + target_waitstatus status; >> + status.set_vfork_done (); >> + add_pending_event (ptid, status); > > I think you can do this? > > add_pending_event (ptid, target_waitstatus ().set_vfork_done ()); > > Not a C++ expert, but I believe the temporary target_waitstatus is kept > alive long enough for this to work. Yes, I think this is fine. >> @@ -1110,21 +1143,18 @@ fbsd_next_vfork_done (void) >> void >> fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo) > > It would be a good time to rename the first parameter from ptid to > scope_ptid, to match what was done here: > > https://gitlab.com/gnutools/binutils-gdb/-/commit/d51926f06a7f4854bebdd71dcb0a78dbaa2f4168 > > I think it helps understand the meaning of the parameter. Hmm, I might do the rename as a separate commit first to avoid adding noise in this patch. >> { >> -#if defined(TDP_RFPPWAIT) && !defined(PTRACE_VFORK) >> - pid_t pid; >> - >> - /* Don't PT_CONTINUE a process which has a pending vfork done event. */ >> - if (minus_one_ptid == ptid) >> - pid = inferior_ptid.pid (); >> - else >> - pid = ptid.pid (); >> - if (fbsd_is_vfork_done_pending (pid)) >> - return; >> -#endif > > Yeah, this old code seems to misinterpret the target_ops::resume > interface, which was clarified in the commit mentioned above. > >> +void >> +fbsd_nat_target::attach (const char *args, int from_tty) >> +{ >> + /* Expect a wait for the new process. */ >> + resume_ptid = minus_one_ptid; >> + fbsd_nat_debug_printf ("setting resume_ptid to [%s]", >> + target_pid_to_str (resume_ptid).c_str ()); >> + inf_ptrace_target::attach (args, from_tty); >> +} >> + > > With the "general" solution where you keep rack of the resume state of > all threads, this part would be a bit cleaner I think. Any thread > created due to an attach or due to create_inferior would start in the > "resumed" state. So later on in patch 6 what happened is that I ended up with a per-process structure that tracked the state of an inferior and resume_ptid is per-inferior instead of global. -- John Baldwin