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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Felix,

>> Why to we call the void * parameter ptw_filter instead of the usual context?
>> We probably want to call the callback itself ptw_filter and the void *
>> argument
>> context.
>>
>> We also seem to mix the terms ptwrite callback and ptwrite filter.
>
>I think the problem here is that both are callbacks. The first one
>(ptw_callback_fun)
>is used in btrace.c to call python/py-record-btrace.c:recpy_call_filter, or another
>extension language that would provide this functionality,
>see extension.c:apply_ext_lang_ptwrite_filter.
>The second one (ptw_filter) is what recpy_call_filter will use to do the python call
>via
>PyObject_CallFunctionObjArgs().
>
>So we call a callback with another callback as the argument (which is the actual
>ptw_filter).
>Therefore the current naming seems correct to me. As the ptw_filter to me
>clearly is
>the void *. 

That's from python's perspective.  The fact that we pass the python object that
implements the python filter to the btrace callback is a detail of the python
implementation, though.

From btrace's perspective, this is the ptwrite callback/filter and the void * is
whatever context that callback needs passed as argument.  I'm fine to call the
function ptw_callback but the context shouldn't be called ptwrite_filter.

In proper C++, we'd probably have a single member for a callable object that
may store its context inside.  Would this even work for python?


>I am open to suggestions, but calling the actual filter function context
>and the
>gdb internal callback the filter seems wrong to me.

Not from btrace's perspective and the code is in btrace.  Python just uses
it in a particular way.


>> >+def default_filter(payload, ip):
>> >+    """Default filter that is active upon starting GDB."""
>> >+    return "{:x}".format(payload)
>> >+
>> >+# This dict contains the per thread copies of the filter function and the
>> >+# global template filter, from which the copies are created.
>> >+_ptwrite_filter = {"global" : default_filter}
>>
>> Why those leading underscores?
>
>GDB follows the PEP8 coding standards:
>https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards
>These say that the leading underscore is a "weak internal user indicator":
>https://pep8.org/#descriptive-naming-styles
>See also:
>https://docs.python.org/3/tutorial/classes.html#private-variables
>
>Other files in GDB use the style as well. It is supposed to mean that
>the variable shouldn't be used directly.

OK.  Thanks.


>> >+def _update_filter_dict(thread_list):
>> >+    """Helper function to update the filter dict.
>> >+
>> >+    Discards filter copies of threads that already exited and registers
>> >+    copies of the filter for new threads."""
>> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
>> >+    lwp_list = [i.ptid[1] for i in thread_list]
>> >+
>> >+    # clean-up old filters
>> >+    for key in _ptwrite_filter.keys():
>> >+      if key not in lwp_list and key != "global":
>> >+        _ptwrite_filter.pop(key)
>> >+
>> >+    # Register filter for new threads
>> >+    for key in lwp_list:
>> >+        if key not in _ptwrite_filter.keys():
>> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>>
>> This function is called two times: once after we cleared all filters, and
>> once when looking up the filter for a given thread.  The first time, we
>> know that there are no existing filters; the second time, we are really
>> only interested in a single filter.
>>
>> Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
>> exist, create a new one?
>
>Yes, we could get rid of the call to _update_filter_dict() in register_filter().
>The main reason I added it was to clean the obsolete filters whenever
>possible. I don't see a clear performance advantage if we would remove
>the call (without having a thread exit notification).
>We need to clean up the same amount of filters at some point.
>
>> That leaves removing obsolete filters.  Could this be done with some
>> thread notification?
>
>IIRC, you suggested this previously. I replied that there is no python API
>that I am aware of that can do this. The python events API doesn't expose
>thread exited events.

I keep stumbling over this.

When looking up a filter, we are clearly only interested in one thread.
Just looking up that one and creating it when it is missing seems a lot
more straight forward.

Lacking a thread exit notification, we could still add a _prune_filters
function that we call every now and then that just removes filters for
exited threads.

Does that sound reasonable?  We'd need to find good places to call
it from.

regards,
markus.
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