RE: [PATCH v3 09/12] btrace, python: Enable ptwrite listener registration.

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

Thanks, Felix,


>+  apply_ext_lang_ptwrite_listener (inferior_ptid);

Should this be called ptwrite_filter?


>+  /* PyObject pointer to the ptwrite listener function.  */
>+  void *ptw_listener = nullptr;
>+

The comment says that this is a PyObject.  Why is it declared void *?


>diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
>index 77f23e0f911..36da4211738 100644
>--- a/gdb/extension-priv.h
>+++ b/gdb/extension-priv.h
>@@ -183,6 +183,10 @@ struct extension_language_ops
>      enum ext_lang_frame_args args_type,
>      struct ui_out *out, int frame_low, int frame_high);
>
>+  /* Used for registering the ptwrite listener to the current thread.  */
>+  enum ext_lang_bt_status (*apply_ptwrite_listener)
>+    (const struct extension_language_defn *, ptid_t inferior_ptid);

There is a parameter name for the second but not for the first parameter.  Also, the name shadows a global variable.  Let's call it just 'ptid'.


>+/* Used for registering the ptwrite listener to the current thread.  */
>+
>+enum ext_lang_bt_status

This enum is specifically for frame filters.  We'd need to either generalize it or add our own for ptwrite filters.


>+apply_ext_lang_ptwrite_listener (ptid_t inferior_ptid)
>+{
>+  for (const struct extension_language_defn *extlang : extension_languages)
>+    {
>+      enum ext_lang_bt_status status;
>+
>+      if (extlang->ops == nullptr
>+	  || extlang->ops->apply_ptwrite_listener == NULL)
>+	continue;
>+      status = extlang->ops->apply_ptwrite_listener (extlang, inferior_ptid);
>+
>+      if (status != EXT_LANG_BT_NO_FILTERS)
>+	return status;

I would have inserted the empty line after the 'continue' from the if statement since obtaining the status and checking it belong together.

IIUC, this installs the ptwrite filter.  The code that actually calls it on a PTWRITE event is not part of this patch.  I think the installation of the filter and calling it should be inside one patch to make it clear how this works.


>+# Ptwrite utilities.
>+# Copyright (C) 2020 Free Software Foundation, Inc.

Year.


>+def default_listener(payload, ip):
>+    """Default listener that is active upon starting GDB."""
>+    return "{:x}".format(payload)
>+
>+# This dict contains the per thread copies of the listener function and the
>+# global template listener, from which the copies are created.
>+_ptwrite_listener = {"global" : default_listener}
>+
>+
>+def _update_listener_dict(thread_list):
>+    """Helper function to update the listener dict.
>+
>+    Discards listener copies of threads that already exited and registers
>+    copies of the listener 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 listeners
>+    for key in _ptwrite_listener.keys():
>+      if key not in lwp_list and key != "global":
>+        _ptwrite_listener.pop(key)
>+
>+    # Register listener for new threads
>+    for key in lwp_list:
>+        if key not in _ptwrite_listener.keys():
>+            _ptwrite_listener[key] = deepcopy(_ptwrite_listener["global"])

So we can only update all at once?  In non-stop mode, I think we want to be able to update just the one for the current thread.

What's the purpose of this deepcopy?  For the default filter, this doesn't seem necessary.  For user-defined filters, this is initializing the per-thread filter from a template, correct?  Should 'global' be spelled 'template' in that case?

Should the template filter be part of the dict, at all?


>+def _clear_traces(thread_list):
>+    """Helper function to clear the trace of all threads."""

The comment doesn't match the code.  The function clears the recordings of threads in THREAD_LIST.


>+    current_thread = gdb.selected_thread()
>+
>+    recording = gdb.current_recording()
>+
>+    if (recording is not None):
>+        for thread in thread_list:
>+            thread.switch()
>+            recording.clear()
>+
>+        current_thread.switch()

Should there be a try around the above to ensure we switch back in case of exceptions?


>+def register_listener(listener):
>+    """Register the ptwrite listener function."""
>+    if listener is not None and not callable(listener):
>+        raise TypeError("Listener must be callable!")
>+
>+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
>+    _clear_traces(thread_list)

Please add a comment as to why we need to clear the recordings for all threads?


>+def get_listener():
>+    """Returns the listeners of the current thread."""

The comment says 'listeners' but there can be only one, correct?

>+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
>+    _update_listener_dict(thread_list)

Why do we need to update the filters for all threads?


>+/* Helper function returning the current ptwrite listener.  Returns nullptr
>+   in case of errors.  */
>+
>+PyObject *
>+get_ptwrite_listener ()
>+{
>+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
>+
>+  if (PyErr_Occurred ())
>+  {
>+    gdbpy_print_stack ();
>+    return nullptr;
>+  }
>+
>+  PyObject *default_ptw_listener = PyObject_CallMethod (module,
>	"get_listener",
>+							NULL);
>+
>+  gdb_Py_DECREF (module);
>+
>+  if (PyErr_Occurred ())
>+    gdbpy_print_stack ();
>+
>+  return default_ptw_listener;

This is not necessarily the default listener, correct?


>+/* Helper function to initialize the ptwrite listener.  Returns nullptr in
>+   case of errors.  */
>+
>+PyObject *
>+recpy_initialize_listener (ptid_t inferior_ptid)

The argument name shadows a global variable.  Let's just call it 'ptid'.


>+  process_stratum_target *proc_target = current_inferior ()->process_target ();

Any chance we can get here without having a current inferior?


>+  struct thread_info * const tinfo = find_thread_ptid (proc_target, inferior_ptid);
>+
>+  tinfo->btrace.ptw_listener = get_ptwrite_listener ();

Let's check tinfo.  Or pass it in as parameter.


>+  return (PyObject *) tinfo->btrace.ptw_listener;

It is called only once and that call ignores the result.


>+/* Used for registering the default ptwrite listener to the current thread.  A
>+   pointer to this function is stored in the python extension interface.  */
>+
>+enum ext_lang_bt_status
>+gdbpy_load_ptwrite_listener (const struct extension_language_defn *extlang,
>+			     ptid_t inferior_ptid)
>+{
>+  struct gdbarch *gdbarch = NULL;
>+
>+  if (!gdb_python_initialized)
>+    return EXT_LANG_BT_NO_FILTERS;
>+
>+  try
>+    {
>+      gdbarch = target_gdbarch ();

Is this a precaution or did you run into cases where target_gdbarch() throws?  Should the rest also be done inside a try block?


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