From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id B7C1E3858D33 for ; Mon, 20 Mar 2023 18:35:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B7C1E3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.146] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6624A1E110; Mon, 20 Mar 2023 14:35:56 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1679337356; bh=KADCPYVZyi14ZSqTydX4h5Lr4rgMxUSkRgPh8gq9h/I=; h=Date:Subject:To:References:From:In-Reply-To:From; b=QaGVqrsksxi/EVPel8HtV+ut83G/bUREoZylRh8uWBXSA7B5O+4+8j3A42E2c7kwo J8uVPHZhyK16f3FMGXB1IRTgGAvg2iKldr0Ljrr8F+EPV6oOaixnjyXHRUJ48R1Evt dKi4BP8X0EEeqrcCJv3jhj/tfA5vqtUAQIQ6cA2o= Message-ID: <8e52f898-c853-df46-52a8-6a4a4c292c5c@simark.ca> Date: Mon, 20 Mar 2023 14:35:56 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH 4/9] fbsd-nat: Add a list of pending events. Content-Language: fr To: John Baldwin , gdb-patches@sourceware.org References: <20230228181845.99936-1-jhb@FreeBSD.org> <20230228181845.99936-5-jhb@FreeBSD.org> From: Simon Marchi In-Reply-To: <20230228181845.99936-5-jhb@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,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 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 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. > + > +/* 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? > + > +struct pending_event > +{ > + pending_event (const ptid_t &_ptid, const target_waitstatus &_status) : > + ptid(_ptid), status(_status) {} Spaces after parentheses. > + > + 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). > + > +/* Add a new pending event to the list. */ > + > +static void > +add_pending_event (const ptid_t &ptid, const target_waitstatus &status) > +{ > + pending_events.emplace_back (ptid, status); > +} > + > +/* Return true if there is a pending event matching FILTER. */ > + > +static bool > +have_pending_event (ptid_t filter) > +{ > + for (const pending_event &event : pending_events) > + if (event.ptid.matches (filter)) > + return true; > + return false; > +} > + > +/* Helper method called by the target wait method. Returns true if > + there is a pending event matching resume_ptid. If there is a resume_ptid -> RESUME_PTID > + 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? > + > /* Return the name of a file that can be opened to get the symbols for > the child process identified by PID. */ > > @@ -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. > @@ -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. > { > -#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. Simon