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

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

Hi Markus,

Thanks for your feedback. I added some responses below,
that we probably should agree on before the next revision.

Thanks,
Felix

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 15:59
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> Hello Felix,
> 
> >With this patch a default ptwrite filter is registered upon start of GDB.
> >It prints the plain ptwrite payload as hex.  The default filter can be
> >overwritten by registering a custom filter in python or by registering
> >"None", for no output at all.  Registering a filter function creates per
> >thread copies to allow unique internal states per thread.
> >---
> > gdb/btrace.c                   |   3 +
> > gdb/btrace.h                   |   9 +++
> > gdb/data-directory/Makefile.in |   1 +
> > gdb/extension-priv.h           |   5 ++
> > gdb/extension.c                |  13 ++++
> > gdb/extension.h                |   3 +
> > gdb/guile/guile.c              |   1 +
> > gdb/python/lib/gdb/ptwrite.py  |  86 +++++++++++++++++++++++++
> > gdb/python/py-record-btrace.c  | 111
> +++++++++++++++++++++++++++++++++
> > gdb/python/py-record-btrace.h  |   8 +++
> > gdb/python/python-internal.h   |   3 +
> > gdb/python/python.c            |   2 +
> > 12 files changed, 245 insertions(+)
> > create mode 100644 gdb/python/lib/gdb/ptwrite.py
> 
> 
> > /* For maintenance commands.  */
> >@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
> >   uint64_t offset;
> >   int status;
> >
> >+  apply_ext_lang_ptwrite_filter (btinfo);
> 
> A comment would be nice, here, to explain what this does.

I added one in the next version.

> 
> >+  /* Function pointer to the ptwrite callback.  Returns the string returned
> >+     by the ptwrite filter function or nullptr if no string is supposed to
> >+     be printed.  */
> >+  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
> >+				   const void *ptw_filter);
> 
> The comment doesn't match the code.  It cannot return nullptr.

Will be fixed in the next revision.

> 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 *. I am open to suggestions, but calling the actual filter function context and the
gdb internal callback the filter seems wrong to me.

I could add to the comment, that the callback_fun can be provided by any extension
language. Or rename it somehow.

> >+
> >+  /* Function pointer to the ptwrite filter function.  */
> >+  void *ptw_filter = nullptr;
> 
> Is this the function or some context for the function?
> 

See above. This is the actual python filter object GDB calls in the end.

> 
> >diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> >index 14b191ded62..86f92a476af 100644
> >--- a/gdb/guile/guile.c
> >+++ b/gdb/guile/guile.c
> >@@ -124,6 +124,7 @@ static const struct extension_language_ops
> >guile_extension_ops =
> >   gdbscm_apply_val_pretty_printer,
> >
> >   NULL, /* gdbscm_apply_frame_filter, */
> >+  NULL, /* gdbscm_load_ptwrite_listener, */
> 
> This should probably be called ptwrite_filter.

Fixed locally.

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

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

The notification approach would have the disadvantage that GDB might
make a lot of calls to python to clean up single filters. Of course, the
current approach has the disadvantage that it manually keeps the thread
list and filter dict in sync.

> 
> >+def _clear_traces(thread_list):
> >+    """Helper function to clear the trace of all threads in THREAD_LIST."""
> >+    current_thread = gdb.selected_thread()
> >+
> >+    recording = gdb.current_recording()
> >+
> >+    if (recording is not None):
> 
> Let's remove the empty line to group the statements.

Will be done.

> 
> >+/* Helper function that calls the ptwrite filter PTW_FILTER with
> >+   PAYLOAD and IP as arguments.  Returns a pointer to the string that will
> >+   be printed or nullptr if nothing should be printed.  IP can be nullptr,
> >+   PAYLOAD must point to a valid integer.  */
> >+std::string
> >+recpy_call_filter (const uint64_t payload, const uint64_t ip,
> >+		   const void *ptw_filter)
> 
> The comment doesn't match the code.  It cannot return nullptr.  Also,
> IP and PAYLOAD are integers, not pointers.
> 
> I think we can shorten "calls the ptwrite filter PTW_FILTER" to just
> "calls PTW_FILTER".

Right, I missed that. Will be fixed, thanks.

> 
> >+{
> >+  std::string result;
> >+
> >+  if ((PyObject *) ptw_filter == Py_None)
> >+    return result;
> >+  else if ((PyObject *) ptw_filter == nullptr)
> >+    error (_("No valid ptwrite filter."));
> 
> No need for else.  Should we check nullptr first?

Will be fixed.

> 
> >+/* Helper function returning the current ptwrite filter.  Returns nullptr
> >+   in case of errors.  */
> >+
> >+PyObject *
> >+get_ptwrite_filter ()
> >+{
> >+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
> >+
> >+  if (PyErr_Occurred ())
> >+  {
> >+    gdbpy_print_stack ();
> >+    return nullptr;
> >+  }
> 
> Do we want to print the stack without throwing an error?
> 

I added an error (and unified the errors of the two functions a bit).

> >+/* Used for registering the default ptwrite filter to the current thread.  A
> 
> What does 'default' mean, here?  We're registering the callback that btrace
> calls on PTW.  From btrace's perspective, this is the ptwrite filter.  From
> python's perspective, this is maybe a proxy for the python filter.
> 
> >+   pointer to this function is stored in the python extension interface.  */
> >+
> >+void
> >+gdbpy_load_ptwrite_filter (const struct extension_language_defn
> *extlang,
> >+			   struct btrace_thread_info *btinfo)
> >+{
> >+  if (!gdb_python_initialized || btinfo == nullptr)
> >+    return;
> 
> Isn't this an error or even an internal error case?

gdb_python_initialized is actually checked by gdbpy_enter, so I removed it.
I added an assert for btinfo.

> 
> >+/* Callback function for the ptwrite filter.  */
> >+extern std::string recpy_call_filter (const uint64_t payload,
> >+				      const uint64_t ip,
> >+				      const void *ptw_filter);
> 
> Should the comment say something like 'proxy for the python
> ptwrite filter'?

See discussion above. In the extension language interface in GDB this is
a callback. How about:

-/* Callback function for the ptwrite filter.  */
+/* Helper function to call the ptwrite filter.  */

That makes it more in line with other python/*.h files.
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