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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----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?
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.
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.

Felix

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928