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: Montag, 3. April 2023 22:45
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> >>> +def ptwrite_exit_handler(event):
> >>> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> >>> +    for key in list(_ptwrite_filter.keys()):
> >>
> >> I don't think the list() is needed.
> >
> > It actually is needed, as I re-size the array:
> >
> >>>> mydict = {1: "one", 2: "two"}
> >>>> for k in mydict.keys():
> > ...     if k == 2:
> > ...         del mydict[k]
> > ...
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> > RuntimeError: dictionary changed size during iteration
> Ah ok, it's to take a "snapshot" of the keys.
> 
> >> I guess that answers my question on the previous patch about the use
> >> case of that clear method.  If that's the only goal of the clear method,
> >> and if there are no use case for the user to call it, I think it would
> >> be better to have an internal function instead.
> >>
> >> Can you explain why its needed to switch threads and call clear on the
> >> same recording object, but once with each thread selected?  I thought
> >> the recording would be per-inferior, and one call to clear would clear
> >> the data for all threads of that inferior.
> >
> > The recordings are per thread not per Inferior. Though I don't think you
> > can actually record only one thread of an inferior, as GDB will always
> > start a recording for all threads, or start recording a new thread if the
> > existing thread is already recording.
> >
> > We want to allow internal state of a filter function to be maintained
> > per thread. Hence the copy and clearing for all threads of the inferior.
> > I implemented the clear() patch based on feedback from Markus.
> > Note that there is already "maintenance btrace clear" in CLI.
> >
> > That said, you are right, I shouldn't clear the same object. In the first
> > version of this series I still had it right, but changed it for V2.
> > I can't exactly remember why.
> >
> > This still leaves the problem of multiple inferiors, that you also
> > commented on below.
> > I think our actual idea was to have this globally, for all inferiors.
> > But that means I would need to clear all traces for all inferiors above.
> > Not just all threads of the current inferior.
> > That can easily be done, unless our other discussions go into a different
> > direction, and we make this per-inferior.
> >
> > So I would change this to something like this:
> >
> > def _clear_traces():
> >     """Helper function to clear the trace of all threads."""
> >     current_thread = gdb.selected_thread()
> >
> >     for inferior in gdb.inferiors():
> >         for thread in inferior.threads():
> >             thread.switch()
> >             recording = gdb.current_recording()
> >             if recording is not None:
> >                 recording.clear()
> >
> >     current_thread.switch()
> >
> > 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.
> 
> For our internal Python helper code, it's fine to do things based on
> some internal knowledge of how GDB is implemented.  So, if we know what
> doing "clear" on one thread clears the recording for the whole inferior,
> then we can do just one thread per inferior.  If GDB changes, we can
> adjust that code at the same time.
> 
> But seeing this, I think that from a Python user point of view, the
> clear method you are proposing is a bit confusing.  It is called on a
> Record object, which is per-thread, but it clears the recording for all
> threads of the inferior.  This would be unexpected, unless documented as
> such in the doc (but even then, it feels a bit awkward).
> 
> The doc talks about "the current recording" here and there, but it's
> unclear what the scope of the recording is.

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.

> >>> +
> >>> +        current_thread.switch()
> >>> +
> >>> +
> >>> +def register_filter(filter_):
> >>> +    """Register the ptwrite filter function."""
> >>> +    if filter_ is not None and not callable(filter_):
> >>> +        raise TypeError("The filter must be callable or 'None'.")
> >>> +
> >>> +    # Clear the traces of all threads to force re-decoding with
> >>> +    # the new filter.
> >>> +    thread_list = gdb.selected_inferior().threads()
> >>> +    _clear_traces(thread_list)
> >>
> >> Could we avoid relying on the selected inferior?  For instance, it could
> >> be a method in Inferior:
> >>
> >>   inf.register_ptwrite_filter(f)
> >>
> >> or pass an inferior to this function:
> >>
> >>   ptwrite.register_filter(inf, f)
> >>
> >> I don't like having to switch inferior all the time in GDB to do
> >> per-inferior stuff, I don't want to impose that to our Python users :).
> >
> > I get where you are coming from. Though one could argue that this is
> already
> > the case today. Starting recordings is already per inferior.
> > If we have two inferiors and start recording one we don't automatically
> > record the second one. And if we have one inferior that is being recorded,
> > and add a second, the second also won't be recorded. You also don't have
> > any option, apart from switching inferiors and starting the recording
> manually.
> 
> I think it's a flaw in the current API that you need to switch current
> inferior to start recording on a specific inferior.  And also, it's a
> flaw in the documentation that it doesn't state that the recording is
> started for the current inferior.  The user is left to guess (or must
> experiment, or read the GDB source code, or have bad surprises) about
> what happens when you have multiple inferiors.

I agree.

> But if I understood you clearly, register_filter is really meant to
> install a global filter (valid for all inferiors, current and future).
> And the call to _clear_traces is meant to clear all traces of all
> inferiors.  In that case, it's fine for register_ptwrite_filter to be
> global.

That is right.

> > For threads that is different. Spawning threads will also be recorded
> > automatically. And if you have two threads and start recording, both
> > will be recorded.
> >
> > What you suggest is definitely possible, years ago I had a version that
> > would allow registering different filters per thread, but we decided against
> > that.
> 
> I don't think that is what I suggested.  If your experience suggests
> that having just one global filter is enough, I can't argue with that.
> Plus, it would probably be possible to implement per-inferior or
> per-thread filters in the future if needed, that would take precedence
> on the global filter.
> 
> My point is just that an API that requires the user to switch context,
> then make an API call, then restore the original context, is bad.  It's
> unclear and error prone.  I know we have some of that already, but I
> would like to avoid adding more.

I again see your points. Above you wrote:

Simon> In that case, it's fine for register_ptwrite_filter to be
Simon> global.

So I won't change anything here now. Extending it in the future should be
simple, I agree.

> >>> +
> >>> +
> >>> +def get_filter():
> >>> +    """Returns the filters of the current thread."""
> >>> +    # The key is of this format to enable an per-inferior cleanup when an
> >>> +    # inferior exits.
> >>> +    key =
> f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
> >>
> >> I don't think we can use f-strings, we claim to support back to Python
> >> 3.4.
> >
> > Did we write down what we support clearly? The gdb/README actually
> says 3.2.
> 
> Sorry, I said 3.4 because I remembered sending a patch that raised the
> minimum level to 3.4.  But that patch was rejected, so it's plausible we
> still support 3.2:
> 
>   https://inbox.sourceware.org/gdb-patches/20220107152921.2858909-3-
> simon.marchi@polymtl.ca/#t
> 
> > And I think it might be good to mention this here:
> > https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-
> Standards
> 
> Feel free to edit that page.  However, the risk of having it documented
> at multiple places is that we can easily forget to update one spot.
> Maybe put a link to the README, and say "make sure your changes are
> compatible with the minimum Python version mentioned in the README"?

Good point, yes a single source of truth is much better.
I am no longer thinking it is a must and I don't think I can edit it anyway.

> > It seems like there are already some f strings in the master branch:
> >
> > ./lib/gdb/dap/io.py:63:            header = f"Content-Length:
> {len(body_bytes)}\r\n\r\n"
> > ./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
> > ./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")
> >
> > I just did a quick grep and didn't look at all occasions.
> >
> > That said, I will remove f strings.
> 
> Probably erroneous then.  Since this is specifically in DAP code, would
> someone using Python 3.4 hit an error when _not_ using DAP?  I would
> guess not, probably why this wasn't noticed.  Or, nobody tests with
> older Python versions...
> 
> >> You could perhaps also pass the key or the pid + lwp from the C++ code
> >> (taken from inferior_ptid).  Calling selected_inferior twice is just
> >> more churn to get the same info.
> >
> > This isn't supposed to be limited to be called from C++ code.
> 
> Ok, I didn't consider that.
> 
> One thing to note is that there isn't always a selected thread, so you
> might want to handle that appropriately.
> 
> > And I think having it without any arguments is a bit easier to use from
> python.
> > I changed it according to your prior suggestions and made it a tuple.
> > But I guess this also depends on our per-inferior discussion above.
> 
> Indeed.  I had not noticed previously, but what I said earlier is true
> for get_filter as well.  I would much rather see get_filter take a
> thread as argument, than relying on the selected thread.

Okay, I will add this. Probably as an optional argument, as I think
that makes it much easier for single threaded users.

> >
> >>> +
> >>> +    # Create a new filter for new threads.
> >>> +    if key not in _ptwrite_filter.keys():
> >>> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> >>
> >> Why is the deepcopy needed.  Because users could register a complex
> >> object with a __call__ method, in which case a simple copy would not do
> >> the same as deepcopy?  Just wondering if that's what users would expect.
> >
> > We wanted to maintain internal state per thread in an easy fashion.
> > As you already saw in Patch 10.
> 
> 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.

And if it does at some point, the code should be straightforward to
extend in the future.

> >>> +{
> >>> +  std::string result;
> >>> +
> >>> +  if ((PyObject *) ptw_filter == nullptr)
> >>> +    error (_("No valid ptwrite filter."));
> >>
> >> I think this can only happen if get_ptwrite_filter has failed, in
> >> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
> >> written as:
> >>
> >>     btinfo->ptw_context = get_ptwrite_filter ();
> >>     if (btinfo->ptw_context != nullptr)
> >>       btinfo->ptw_callback_fun = &recpy_call_filter;
> >>
> >> ... such that recpy_call_filter is only installed when there is
> >> something in ptw_context.  recpy_call_filter could then do:
> >>
> >>   gdb_assert (ptw_filter != nullptr);
> >
> > I guess I could do that. Yet we check ptw_callback_fun before calling it
> > in btrace.c. See patch 10. So that alone would actually hide problems to the
> user.
> > And just not print any ptwrite string silently. Even if the user gave a custom
> > listener function. So we would also need to remove that check.
> >
> > I am also wondering what the advantage you see is.
> > That we have an assert instead of an error? I guess I could do that even
> > without the change in gdbpy_load_ptwrite_filter.
> 
> Just trying to choose the right tool for the job, which in turns helps
> build the right mental model.  error() calls are for errors that aren't
> GDB's fault, errors that are not the result of a bug in GDB.  gdb_assert
> is to catch things programming errors in GDB.
> 
> If I see:
> 
>   gdb_assert (foo == nullptr);
> 
> I conclude that there should be not situation where foo is nullptr.  If
> I see:
> 
>   if (foo -- nullptr)
>     error (...)
> 
> Then I conclude there are some valid situations where foo is nullptr
> (and then we have to worry about how they are handled).

I will change the error in recpy_call_filter to an assert.
I still don't see the point of not registering ptw_callback_fun().
I fear that will just hide things.
Anyway, I will look at this again when implementing the printing in C++
that you suggested for builds without Python. I think the picture
might be clearer once I have that.

> >>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-
> internal.h
> >>> index 258f5c42537..c21232c07dc 100644
> >>> --- a/gdb/python/python-internal.h
> >>> +++ b/gdb/python/python-internal.h
> >>> @@ -376,6 +376,9 @@ extern enum ext_lang_rc
> >> gdbpy_apply_val_pretty_printer
> >>>     struct ui_file *stream, int recurse,
> >>>     const struct value_print_options *options,
> >>>     const struct language_defn *language);
> >>> +extern void gdbpy_load_ptwrite_filter
> >>> +  (const struct extension_language_defn *extlang,
> >>> +   struct btrace_thread_info *btinfo);
> >>
> >> Can this declaration be in py-record-btrace.h?  I really don't see why
> >> all the declarations are stuffed into python.h.  Also, please move the
> >> comment to the declaration, and put the appropriate /* See ... */
> >> comment on the definition.
> >
> > I don't think so. I modelled this after the other extension language
> interfaces.
> > These are also all in here, without any comment. This function is being
> called
> > via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
> > As far as I understand it, that is how you interact with the python layer.
> > To also allow the same things to be done in guile, if someone feels like it.
> > Please tell me if I misunderstood the architecture.
> 
> I think you're right that everything goes through python/python.h.  I
> don't know why though, we we can't have functions declared in various .h
> files.  Perhaps it's easier to manage for when building with Python
> disabled, I don't know.  In any case, not really important right now,
> sorry for bringing that up.

No worries, I appreciate the review!

Thanks,
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