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 5D8ED3858408 for ; Mon, 27 Mar 2023 20:57:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5D8ED3858408 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 [10.0.0.170] (unknown [217.28.27.60]) (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 1BC141E0D3; Mon, 27 Mar 2023 16:57:40 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1679950660; bh=A3LbXbPTO8/PszJJ61/DUoXpVFcgFloe9s0xfaQ3wH8=; h=Date:Subject:To:References:From:In-Reply-To:From; b=fBs34l+Xf16ScAlGZttwgMg9gpd3bB0AjpqYcI+RiK5/TR20AsqLbeG1iX3rvchU1 5Pa3DZdcEEnBRJuI4rFoU4PwOSrQDcV7v5sC3RyrSp87kbC3zmRwWC/5pLFS16sOEY EpfGWfZWphPV7Z2Mhxf1idlEBUsgA6lOICYzJDc4= Message-ID: <87e5349e-56b4-c8b8-5ebf-bc53146c2593@simark.ca> Date: Mon, 27 Mar 2023 16:57:39 -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> <8e52f898-c853-df46-52a8-6a4a4c292c5c@simark.ca> <1f5bad14-378e-47df-5386-b2c48710cf64@FreeBSD.org> From: Simon Marchi In-Reply-To: <1f5bad14-378e-47df-5386-b2c48710cf64@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.7 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 3/27/23 16:24, John Baldwin wrote: > 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. Ok that's fine with me. >> 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. That's ok. I review patches linearly, making comments as I go. I don't have the mental swap space to look at everything and then comment based on the whole series (even though it would be better). So it's appropriate to tell me "it's irrelevant, it changes later". > 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(). Indeed. Simon