Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Simon Marchi <simark@simark.ca>]

On 4/5/23 06:20, Willgerodt, Felix wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Dienstag, 4. April 2023 17:06
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; 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