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

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

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.


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

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.

>+
>+  /* Function pointer to the ptwrite filter function.  */
>+  void *ptw_filter = nullptr;

Is this the function or some context for the function?


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


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


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

That leaves removing obsolete filters.  Could this be done with some
thread notification?


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


>+/* 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".


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


>+/* 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?


>+/* 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?


>+/* 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'?

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