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: Mittwoch, 13. Juli 2022 17:21
> 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,
> 
> >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?
> 
> That sounds OK.  How about avoiding the problem by doing it with a C++
> callable?

I don't think I completely follow what you are proposing or what we would
gain. I guess I can make the callback_fun e.g. a std::function or a
callable class/struct. We will still need to save a pointer for the
ptw_context/filter in btinfo (or the callable class/struct member), as we pass
that to the Python C API. To me this doesn't avoid the naming problem.

>+    # Clean-up old filters.
> >+    for key in _ptwrite_filter.keys():
> >+      if key not in lwp_list and key != "global":
> >+        _ptwrite_filter.pop(key)
> 
> We just cleared the filters.  There are no existing filters.

Right.

> >+
> >+    # Register filter for new threads.
> >+    for key in lwp_list:
> >+        if key not in _ptwrite_filter.keys():
> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> 
> New filters are added on-demand below.  I don't think this is necessary.

Right again. Thanks for the help, these two things make it much simpler.

> >
> >
> > 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]
> 
> That looks good.
> 
> We're no longer pruning filters for exited threads.  We could do so on
> inferior exit.  Would that suffice?


Pruning on inferior exit only helps in certain cases.
We can't get a complete "all-past-threads" list from GDB for an exited
inferior though. And we can't just remove every dict entry except for
"global", as we could be debugging multiple inferiors.

What I could do is add e.g. the inferior id to the dict. And prune only these on
inferior exit:

+def ptw_exit_handler(event):
+    """Exit handler to prune _ptwrite_filter on inferior exit."""
+    for key in _ptwrite_filter.keys():
+        if key.startswith(f"{event.inferior.pid}."):
+            del _ptwrite_filter[key]
+
+
 def _clear_traces(thread_list):
     """Helper function to clear the trace of all threads in THREAD_LIST."""
     current_thread = gdb.selected_thread()
@@ -59,10 +66,13 @@ def register_filter(filter):
 
 def get_filter():
     """Returns the filters of the current thread."""
-    key = gdb.selected_thread().ptid[1]
+    key = f"{gdb.selected_inferior().pid}.{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[key]
+
+
+gdb.events.exited.connect(ptw_exit_handler)

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