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

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

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?


>> >> >+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.
>>
>> I keep stumbling over this.
>>
>> When looking up a filter, we are clearly only interested in one thread.
>> Just looking up that one and creating it when it is missing seems a lot
>> more straight forward.
>>
>> Lacking a thread exit notification, we could still add a _prune_filters
>> function that we call every now and then that just removes filters for
>> exited threads.
>>
>> Does that sound reasonable?  We'd need to find good places to call
>> it from.
>
>To me that is kind of what I have implemented now. Just not with a
>separate _prune_filters() and doing it in "two good places".
>
>But I just realized that having it in get_filter() would only improve
>performance if someone would call get_filter() from python directly.
>Which probably isn't a scenario worth optimizing for.
>
>Are you okay with changing get_filter and inlining _update_filter_dict?

I can't say whether performance is an issue.  I stumbled over it while
trying to understand the code and wondering why we are doing all
this.

I think it would be cleaner to have a prune function and otherwise
just do what's necessary.


> def _clear_traces(thread_list):
>     """Helper function to clear the trace of all threads in THREAD_LIST."""
>     current_thread = gdb.selected_thread()
>@@ -74,12 +55,26 @@ def register_filter(filter):
>     _ptwrite_filter.clear()
>     _ptwrite_filter["global"] = filter
>
>-    _update_filter_dict(thread_list)
>+    # 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)

We just cleared the filters.  There are no existing filters.

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

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

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