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 B167D3858C54 for ; Wed, 5 Apr 2023 20:27:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B167D3858C54 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.11] (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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 521DA1E110; Wed, 5 Apr 2023 16:27:06 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1680726426; bh=JuYZII/v/I9KOQSzvfTiiT+Mb7V0DJ2/LKdrrLn9XOA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=n4Ds3V8nE8/+YcKEXKNIkDQeqTrWfJHIJq7eF3NPfyJTp4pF7OcHdSHHeMnXbl4Wo eCKRdPaLTU65gTP4FnPOf8fu0OHSH7xRf1rFYIBayv98Tg3/thEbCJXyUTcH4VHZgQ adejah0Flw/vKVQSdfgq7qVuU+wpI8eH88YtEC6E= Message-ID: <274306f0-79a8-f6bd-51a1-bc53b2245b6b@simark.ca> Date: Wed, 5 Apr 2023 16:27:05 -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 v8 09/10] btrace, python: Enable ptwrite filter registration. Content-Language: en-US To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" Cc: "Metzger, Markus T" References: <20230321154626.448816-1-felix.willgerodt@intel.com> <20230321154626.448816-10-felix.willgerodt@intel.com> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 4/5/23 06:20, Willgerodt, Felix wrote: > > >> -----Original Message----- >> From: Simon Marchi >> Sent: Dienstag, 4. April 2023 17:06 >> To: Willgerodt, Felix ; gdb- >> patches@sourceware.org >> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter >> registration. >> >>> I agree on the doc part. It would indeed be nice to write this down. >>> >>> Though, I think you misunderstood some things (or maybe I did). >>> Gdb.current_recording().clear(), as added in patch 5, does clear on a >>> per-thread basis. Not on a per inferior basis as you write. >>> But maybe you meant _clear_traces() here? This is supposed to be >>> an internal function, as the leading underscore suggests. It indeed >>> does clear all recordings for all threads. As that is needed for ptwrite, >>> which is the only intended use case of this internal function. >>> >>> So I don't quite see what would be unexpected. >> My misunderstanding comes from where you said: >> >> >>> Not sure if it is worth exiting early per inferior. I couldn't find >>> a scenario where you can selectively record (or not record) >>> one thread of a multi-threaded inferior. >> >> By "exiting early", I thought you meant that it would be enough to clear >> the data for one thread in each inferior. >> >>>> I understand that you want to have one copy of the filter per thread. >>>> >>>> But doing a copy seems like a strange instantiation strategy. The >>>> "global" filter isn't ever used, right? It only serves to make copies? >>>> I think a more common pattern would be for the user to register a >>>> factory function (really, ny callable that fits the bill) which would be >>>> called whenever we need to create an instance. >>> >>> Mhm, I think that would make this a bit too complex for users. >>> I don't think they want to care about providing a generator as well >>> that returns a callable. I don't see any new feature that >>> is being unlocked with that, just more tasks for the user. >> >> I don't think it adds significant complexity, this is pretty typical >> Python. It is similar to what we have for the pretty printers. You >> register a function that gets called to instantiate the right pretty >> printer. >> >> At the simplest, it would be: >> >> class MyFilter: >> ... >> >> def make_one_filter(): >> return MyFilter() >> >> register_filter(make_one_filter) >> >> or even: >> >> class MyFilter: >> ... >> >> register_filter(lambda: MyFilter()) >> >> An advantage is that it makes it easy to return a different filter per >> thread, if needed. Since filters are on a per-thread basis, I think it >> would even make sense to pass the thread to the factory function, in >> case the user wants to select a filter based on that. >> >> Not that the fact that filters are callable themselves is just a design >> choice you made, you could very well decide to call something else than >> __call__ on those objects. If you want to make things more explicit, >> you can make it so filters have to implement a "get_aux_data" method, >> instead of __call__. The advantage of using __call__ is just that it >> allows freely passing functions as well as objects implementing >> __call__, so that gives the user a bit more flexibility. >> >> Simon > > Sorry, I still don't understand this or see the advantage of the > factory pattern here. > And I only want to use a pattern if there is a clear advantage to it. > > I assume you want to move the "housekeeping" to the user side, right? Not sure what you mean by "housekeeping". In the end, with what I propose, we still end up with one filter object per thread, it just changes how they are instantiated. Instead of copying a "template" object, we call the "get me a new instance" function. > Why do we need a factory pattern for that? > Can't you achieve the same if we just have one callable filter object > instead of one per thread? And have the user do it in that object, if > he needs to? (Per thread filters would still allow the same, though not > as clean, as they would have to call the singular object.) > Passing the thread can also be done with the filter function directly. > And is just a convenience, as the GDB API is available to the user. > (I am not saying, it is a bad convenience feature.) > > AFAIK, you use a factory pattern when you want to abstract what > type of exact object is returned. I don't see that we need that. > And we already have a bit of freedom anyway, with allowing > callables. > > If you don't like the per thread copy, we could still just not do that. With what I propose, there would still be one copy (instance) of the filter per thread. So maybe we're not on the same page. > And move the per-thread or per-inferior housekeeping to the user. > I had this at some point. The documentation I wrote still gives a short > example for how to do something for one thread but not for another. > > As far as I remember, Markus wanted per-thread state in an easier way > though. Simon