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

[Simon Marchi <simark@simark.ca>]

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

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

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.

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

>>> +
>>> +
>>> +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"?

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

> 
>>> +
>>> +    # 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.

>>> +{
>>> +  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).

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

Simon