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

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

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 12. Juli 2022 14:24
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> 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.

I get your argumentation. I still think ptw_filter is perfectly fine. As that is
what it will always be, even for other extension languages. I see it more
from a "global GDB" perspective, rather than "btrace vs python" or from a
"callback concept" POV.
Regardless, I will call it to ptw_context in the next revision. Is that ok?


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

To me that is kind of what I have implemented now. Just not with a
separate _prune_filters() and doing it in "two good places".

But I just realized that having it in get_filter() would only improve
performance if someone would call get_filter() from python directly.
Which probably isn't a scenario worth optimizing for.

Are you okay with changing get_filter and inlining _update_filter_dict?

--- a/gdb/python/lib/gdb/ptwrite.py
+++ b/gdb/python/lib/gdb/ptwrite.py
@@ -29,25 +29,6 @@ def default_filter(payload, ip):
 _ptwrite_filter = {"global" : default_filter}
 
 
-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"])
-
-
 def _clear_traces(thread_list):
     """Helper function to clear the trace of all threads in THREAD_LIST."""
     current_thread = gdb.selected_thread()
@@ -74,12 +55,26 @@ def register_filter(filter):
     _ptwrite_filter.clear()
     _ptwrite_filter["global"] = filter
 
-    _update_filter_dict(thread_list)
+    # 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"])
 
 
 def get_filter():
     """Returns the filters of the current thread."""
-    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
-    _update_filter_dict(thread_list)
+    key = gdb.selected_thread().ptid[1]
+
+    # This could be a new thread.
+    if key not in _ptwrite_filter.keys():
+        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
 
-    return _ptwrite_filter[gdb.selected_thread().ptid[1]]
+    return _ptwrite_filter[key]

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