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

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

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Freitag, 13. August 2021 12:36
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 09/12] btrace, python: Enable ptwrite listener
> registration.
> 
> Thanks, Felix,
> 
> 
> >+  apply_ext_lang_ptwrite_listener (inferior_ptid);
> 
> Should this be called ptwrite_filter?
> 

No, that wasn't my intention. I can rename "listener" to "filter" if you
want, but that would affect the whole series and documentation.
Or are you just requesting the extension functions to be updated?

> 
> >+  /* PyObject pointer to the ptwrite listener function.  */
> >+  void *ptw_listener = nullptr;
> >+
> 
> The comment says that this is a PyObject.  Why is it declared void *?
>

Because I didn't want to include a python header for PyObject here.
Afaik, btrace.h should be kept free of those.

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

The first parameter never seems to be named in this file. Apart from one
occasion. That said, I think it might be only for formatting reasons, and we have
enough space here. So I added a name. I also changed it to 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.
> 

Good catch. Since we are not checking it anyway and allow nullptr/None
as a valid listener I decided to make this return void.

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

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

Done.

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

Done

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

We discussed this offline last year. To summarize:
We deepcopy to allow every thread to keep it's own internal state. We also
discussed the architecture repeatedly. We decided to update every thread
if we update the listener. I had a version some years ago that allowed to
specify which threads to update, but we decided against that.

I didn't change anything.

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

I updated the comment.


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

I am not sure. I don't think clear() can really throw. If switch throws, it
might be because of the thread_list being wrong. But we got the list from
gdb itself just before this call. It might also be that there is a problem with
switch(), but in that case trying to switch again seems dangerous.
 
> 
> >+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?
> 

Done.

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

Because we are registering a new copy of the new listener for every
thread.  An earlier (internal) version that I had allowed to register
different listeners per thread, but we decided against that.
This is needed to enable per-thread internal states, even if the user
can only register one listener globally.

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

Correct, I renamed it.


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

I rewrote this a bit so this is no longer an issue.
 
> 
> >+  process_stratum_target *proc_target = current_inferior ()-
> >process_target ();
> 
> Any chance we can get here without having a current inferior?
>

I rewrote this to pass btinfo from btrace.c. That made this function
redundant.
 
> 
> >+  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.
>

See above, I rewrote this. In the new version I pass and check btinfo directly.

> 
> >+  return (PyObject *) tinfo->btrace.ptw_listener;
> 
> It is called only once and that call ignores the result.
>

I rewrote this. Since we allow nullptr/None to silence the listener I decided
to make everything return void.
 
> 
> >+/* 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?
>

I blindly copied this from py-framefilter.c:gdbpy_apply_frame_filter() years ago.
But looking at git and some recent changes to enter_py I don't think any of this
is necessary anymore. I rewrote this.

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