[v8,09/10] btrace, python: Enable ptwrite filter registration.

  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                   |  4 ++
 gdb/btrace.h                   |  8 ++++
 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  | 80 +++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.c  | 88 ++++++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.h  |  8 ++++
 gdb/python/python-internal.h   |  3 ++
 gdb/python/python.c            |  2 +
 12 files changed, 216 insertions(+)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py
  

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> 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                   |  4 ++
>  gdb/btrace.h                   |  8 ++++
>  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  | 80 +++++++++++++++++++++++++++++++
>  gdb/python/py-record-btrace.c  | 88 ++++++++++++++++++++++++++++++++++
>  gdb/python/py-record-btrace.h  |  8 ++++
>  gdb/python/python-internal.h   |  3 ++
>  gdb/python/python.c            |  2 +
>  12 files changed, 216 insertions(+)
>  create mode 100644 gdb/python/lib/gdb/ptwrite.py
> 
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index f2fc4786e21..37dd0b666d8 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -34,6 +34,7 @@
>  #include "gdbsupport/rsp-low.h"
>  #include "gdbcmd.h"
>  #include "cli/cli-utils.h"
> +#include "extension.h"
>  #include "gdbarch.h"
>  
>  /* For maintenance commands.  */
> @@ -1317,6 +1318,9 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
>    uint64_t offset;
>    int status;
>  
> +  /* Register the ptwrite filter.  */
> +  apply_ext_lang_ptwrite_filter (btinfo);
> +
>    for (;;)
>      {
>        struct pt_insn insn;
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index f6a8274bb16..912cb16056a 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -352,6 +352,14 @@ struct btrace_thread_info
>       displaying or stepping through the execution history.  */
>    std::vector<std::string> aux_data;
>  
> +  /* Function pointer to the ptwrite callback.  Returns the string returned
> +     by the ptwrite filter function.  */
> +  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
> +				   const void *ptw_context) = nullptr;
> +
> +  /* Context for the ptw_callback_fun.  */
> +  void *ptw_context = nullptr;
> +
>    /* The function level offset.  When added to each function's LEVEL,
>       this normalizes the function levels such that the smallest level
>       becomes zero.  */
> diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
> index ff1340c44c0..6ba880c3b6b 100644
> --- a/gdb/data-directory/Makefile.in
> +++ b/gdb/data-directory/Makefile.in
> @@ -75,6 +75,7 @@ PYTHON_FILE_LIST = \
>  	gdb/frames.py \
>  	gdb/printing.py \
>  	gdb/prompt.py \
> +	gdb/ptwrite.py \
>  	gdb/styling.py \
>  	gdb/types.py \
>  	gdb/unwinder.py \
> diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
> index 23a9f646d12..75112afd3ab 100644
> --- a/gdb/extension-priv.h
> +++ b/gdb/extension-priv.h
> @@ -183,6 +183,11 @@ 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 filter to the current thread.  */
> +  void (*apply_ptwrite_filter)
> +    (const struct extension_language_defn *extlang,
> +     struct btrace_thread_info *btinfo);
> +
>    /* Update values held by the extension language when OBJFILE is discarded.
>       New global types must be created for every such value, which must then be
>       updated to use the new types.
> diff --git a/gdb/extension.c b/gdb/extension.c
> index 4ac6e0b6732..8b32c7e1f13 100644
> --- a/gdb/extension.c
> +++ b/gdb/extension.c
> @@ -551,6 +551,19 @@ apply_ext_lang_frame_filter (frame_info_ptr frame,
>    return EXT_LANG_BT_NO_FILTERS;
>  }
>  
> +/* Used for registering the ptwrite filter to the current thread.  */
> +
> +void
> +apply_ext_lang_ptwrite_filter (btrace_thread_info *btinfo)
> +{
> +  for (const struct extension_language_defn *extlang : extension_languages)
> +    {
> +      if (extlang->ops != nullptr
> +	  && extlang->ops->apply_ptwrite_filter != nullptr)
> +	extlang->ops->apply_ptwrite_filter (extlang, btinfo);
> +    }
> +}
> +
>  /* Update values held by the extension language when OBJFILE is discarded.
>     New global types must be created for every such value, which must then be
>     updated to use the new types.
> diff --git a/gdb/extension.h b/gdb/extension.h
> index ab83f9c6a28..639093945e4 100644
> --- a/gdb/extension.h
> +++ b/gdb/extension.h
> @@ -295,6 +295,9 @@ extern enum ext_lang_bt_status apply_ext_lang_frame_filter
>     enum ext_lang_frame_args args_type,
>     struct ui_out *out, int frame_low, int frame_high);
>  
> +extern void apply_ext_lang_ptwrite_filter
> +  (struct btrace_thread_info *btinfo);
> +
>  extern void preserve_ext_lang_values (struct objfile *, htab_t copied_types);
>  
>  extern const struct extension_language_defn *get_breakpoint_cond_ext_lang
> diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> index 887b7fa5dc8..e9b0d4127d5 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_filter, */
>  
>    gdbscm_preserve_values,
>  
> diff --git a/gdb/python/lib/gdb/ptwrite.py b/gdb/python/lib/gdb/ptwrite.py
> new file mode 100644
> index 00000000000..0f5b0473023
> --- /dev/null
> +++ b/gdb/python/lib/gdb/ptwrite.py
> @@ -0,0 +1,80 @@
> +# Ptwrite utilities.
> +# Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Utilities for working with ptwrite filters."""
> +
> +from copy import deepcopy
> +import gdb
> +
> +
> +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}
> +
> +
> +def ptwrite_exit_handler(event):
> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> +    for key in list(_ptwrite_filter.keys()):

I don't think the list() is needed.

> +        if key.startswith(f"{event.inferior.pid}."):

Instead of using a string as a key, you could use a tuple, (pid, lwp).
So here you would check key[0].

> +            del _ptwrite_filter[key]
> +
> +
> +gdb.events.exited.connect(ptwrite_exit_handler)
> +
> +
> +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:
> +        for thread in thread_list:
> +            thread.switch()
> +            recording.clear()

I guess that answers my question on the previous patch about the use
case of that clear method.  If that's the only goal of the clear method,
and if there are no use case for the user to call it, I think it would
be better to have an internal function instead.

Can you explain why its needed to switch threads and call clear on the
same recording object, but once with each thread selected?  I thought
the recording would be per-inferior, and one call to clear would clear
the data for all threads of that inferior.

> +
> +        current_thread.switch()
> +
> +
> +def register_filter(filter_):
> +    """Register the ptwrite filter function."""
> +    if filter_ is not None and not callable(filter_):
> +        raise TypeError("The filter must be callable or 'None'.")
> +
> +    # Clear the traces of all threads to force re-decoding with
> +    # the new filter.
> +    thread_list = gdb.selected_inferior().threads()
> +    _clear_traces(thread_list)

Could we avoid relying on the selected inferior?  For instance, it could
be a method in Inferior:

  inf.register_ptwrite_filter(f)

or pass an inferior to this function:

  ptwrite.register_filter(inf, f)

I don't like having to switch inferior all the time in GDB to do
per-inferior stuff, I don't want to impose that to our Python users :).

> +
> +    _ptwrite_filter.clear()
> +    _ptwrite_filter["global"] = filter_

Hmm, above you (seem to) clear the data for all threads of the selected
inferior, implying that setting a filter affects only the current
inferior.  But here you clear the whole _ptwrite_filter, which could
contain filters for other inferiors.  I don't understand.

> +
> +
> +def get_filter():
> +    """Returns the filters of the current thread."""
> +    # The key is of this format to enable an per-inferior cleanup when an
> +    # inferior exits.
> +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"

I don't think we can use f-strings, we claim to support back to Python
3.4.

You could perhaps also pass the key or the pid + lwp from the C++ code
(taken from inferior_ptid).  Calling selected_inferior twice is just
more churn to get the same info.

> +
> +    # Create a new filter for new threads.
> +    if key not in _ptwrite_filter.keys():

.keys() is not needed

> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])

Why is the deepcopy needed.  Because users could register a complex
object with a __call__ method, in which case a simple copy would not do
the same as deepcopy?  Just wondering if that's what users would expect.

> +
> +    return _ptwrite_filter[key]
> diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
> index 4922154fc1b..100a9ee8578 100644
> --- a/gdb/python/py-record-btrace.c
> +++ b/gdb/python/py-record-btrace.c
> @@ -763,6 +763,94 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
>    return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
>  }
>  
> +/* Helper function that calls PTW_FILTER with PAYLOAD and IP as arguments.
> +   Returns the string that will be printed.  */
> +std::string
> +recpy_call_filter (const uint64_t payload, const uint64_t ip,
> +		   const void *ptw_filter)

Should be static.

> +{
> +  std::string result;
> +
> +  if ((PyObject *) ptw_filter == nullptr)
> +    error (_("No valid ptwrite filter."));

I think this can only happen if get_ptwrite_filter has failed, in
gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
written as:

    btinfo->ptw_context = get_ptwrite_filter ();
    if (btinfo->ptw_context != nullptr)
      btinfo->ptw_callback_fun = &recpy_call_filter;

... such that recpy_call_filter is only installed when there is
something in ptw_context.  recpy_call_filter could then do:

  gdb_assert (ptw_filter != nullptr);

> +  if ((PyObject *) ptw_filter == Py_None)
> +    return result;
> +
> +  gdbpy_enter enter_py;
> +
> +  gdbpy_ref<> py_payload (PyLong_FromUnsignedLongLong (payload));
> +  gdbpy_ref<> py_ip (PyLong_FromUnsignedLongLong (ip));
> +
> +  if (ip == 0)
> +    py_ip = gdbpy_ref<>::new_reference (Py_None);
> +
> +  gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *) ptw_filter,
> +							py_payload.get (),
> +							py_ip.get (),
> +							nullptr));
> +
> +  if (PyErr_Occurred ())
> +    {
> +      gdbpy_print_stack ();
> +      gdbpy_error (_("Couldn't call the ptwrite filter."));
> +    }
> +
> +  /* Py_None is valid and results in no output.  */
> +  if (py_result == Py_None)
> +    return result;
> +
> +  result = gdbpy_obj_to_string (py_result.get ()).get ();
> +
> +  if (PyErr_Occurred ())
> +    {
> +      gdbpy_print_stack ();
> +      gdbpy_error (_("The ptwrite filter didn't return a string."));
> +    }
> +
> +  return result;
> +}
> +
> +/* Helper function returning the current ptwrite filter.  */
> +
> +PyObject *
> +get_ptwrite_filter ()

Should be static.

> +/* Used for registering the default ptwrite filter to the current thread.  A
> +   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)
> +{
> +  gdb_assert (btinfo != nullptr);
> +
> +  gdbpy_enter enter_py;
> +
> +  btinfo->ptw_callback_fun = &recpy_call_filter;
> +  btinfo->ptw_context= get_ptwrite_filter ();

Missing space.

> diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-btrace.h
> index f297772f946..12750b3b1c3 100644
> --- a/gdb/python/py-record-btrace.h
> +++ b/gdb/python/py-record-btrace.h
> @@ -91,4 +91,12 @@ extern PyObject *recpy_bt_func_prev (PyObject *self, void *closure);
>  /* Implementation of RecordFunctionSegment.next [RecordFunctionSegment].  */
>  extern PyObject *recpy_bt_func_next (PyObject *self, void *closure);
>  
> +/* Helper function returning the current ptwrite filter.  */
> +extern PyObject *get_ptwrite_filter ();
> +
> +/* Helper function to call the ptwrite filter.  */
> +extern std::string recpy_call_filter (const uint64_t payload,
> +				      const uint64_t ip,
> +				      const void *ptw_filter);
> +
>  #endif /* PYTHON_PY_RECORD_BTRACE_H */
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 258f5c42537..c21232c07dc 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -376,6 +376,9 @@ extern enum ext_lang_rc gdbpy_apply_val_pretty_printer
>     struct ui_file *stream, int recurse,
>     const struct value_print_options *options,
>     const struct language_defn *language);
> +extern void gdbpy_load_ptwrite_filter
> +  (const struct extension_language_defn *extlang,
> +   struct btrace_thread_info *btinfo);

Can this declaration be in py-record-btrace.h?  I really don't see why
all the declarations are stuffed into python.h.  Also, please move the
comment to the declaration, and put the appropriate /* See ... */
comment on the definition.

Simon
  

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 16:23
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > 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                   |  4 ++
> >  gdb/btrace.h                   |  8 ++++
> >  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  | 80
> +++++++++++++++++++++++++++++++
> >  gdb/python/py-record-btrace.c  | 88
> ++++++++++++++++++++++++++++++++++
> >  gdb/python/py-record-btrace.h  |  8 ++++
> >  gdb/python/python-internal.h   |  3 ++
> >  gdb/python/python.c            |  2 +
> >  12 files changed, 216 insertions(+)
> >  create mode 100644 gdb/python/lib/gdb/ptwrite.py
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index f2fc4786e21..37dd0b666d8 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -34,6 +34,7 @@
> >  #include "gdbsupport/rsp-low.h"
> >  #include "gdbcmd.h"
> >  #include "cli/cli-utils.h"
> > +#include "extension.h"
> >  #include "gdbarch.h"
> >
> >  /* For maintenance commands.  */
> > @@ -1317,6 +1318,9 @@ ftrace_add_pt (struct btrace_thread_info
> *btinfo,
> >    uint64_t offset;
> >    int status;
> >
> > +  /* Register the ptwrite filter.  */
> > +  apply_ext_lang_ptwrite_filter (btinfo);
> > +
> >    for (;;)
> >      {
> >        struct pt_insn insn;
> > diff --git a/gdb/btrace.h b/gdb/btrace.h
> > index f6a8274bb16..912cb16056a 100644
> > --- a/gdb/btrace.h
> > +++ b/gdb/btrace.h
> > @@ -352,6 +352,14 @@ struct btrace_thread_info
> >       displaying or stepping through the execution history.  */
> >    std::vector<std::string> aux_data;
> >
> > +  /* Function pointer to the ptwrite callback.  Returns the string returned
> > +     by the ptwrite filter function.  */
> > +  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t
> ip,
> > +				   const void *ptw_context) = nullptr;
> > +
> > +  /* Context for the ptw_callback_fun.  */
> > +  void *ptw_context = nullptr;
> > +
> >    /* The function level offset.  When added to each function's LEVEL,
> >       this normalizes the function levels such that the smallest level
> >       becomes zero.  */
> > diff --git a/gdb/data-directory/Makefile.in b/gdb/data-
> directory/Makefile.in
> > index ff1340c44c0..6ba880c3b6b 100644
> > --- a/gdb/data-directory/Makefile.in
> > +++ b/gdb/data-directory/Makefile.in
> > @@ -75,6 +75,7 @@ PYTHON_FILE_LIST = \
> >  	gdb/frames.py \
> >  	gdb/printing.py \
> >  	gdb/prompt.py \
> > +	gdb/ptwrite.py \
> >  	gdb/styling.py \
> >  	gdb/types.py \
> >  	gdb/unwinder.py \
> > diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
> > index 23a9f646d12..75112afd3ab 100644
> > --- a/gdb/extension-priv.h
> > +++ b/gdb/extension-priv.h
> > @@ -183,6 +183,11 @@ 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 filter to the current thread.  */
> > +  void (*apply_ptwrite_filter)
> > +    (const struct extension_language_defn *extlang,
> > +     struct btrace_thread_info *btinfo);
> > +
> >    /* Update values held by the extension language when OBJFILE is
> discarded.
> >       New global types must be created for every such value, which must
> then be
> >       updated to use the new types.
> > diff --git a/gdb/extension.c b/gdb/extension.c
> > index 4ac6e0b6732..8b32c7e1f13 100644
> > --- a/gdb/extension.c
> > +++ b/gdb/extension.c
> > @@ -551,6 +551,19 @@ apply_ext_lang_frame_filter (frame_info_ptr
> frame,
> >    return EXT_LANG_BT_NO_FILTERS;
> >  }
> >
> > +/* Used for registering the ptwrite filter to the current thread.  */
> > +
> > +void
> > +apply_ext_lang_ptwrite_filter (btrace_thread_info *btinfo)
> > +{
> > +  for (const struct extension_language_defn *extlang :
> extension_languages)
> > +    {
> > +      if (extlang->ops != nullptr
> > +	  && extlang->ops->apply_ptwrite_filter != nullptr)
> > +	extlang->ops->apply_ptwrite_filter (extlang, btinfo);
> > +    }
> > +}
> > +
> >  /* Update values held by the extension language when OBJFILE is
> discarded.
> >     New global types must be created for every such value, which must then
> be
> >     updated to use the new types.
> > diff --git a/gdb/extension.h b/gdb/extension.h
> > index ab83f9c6a28..639093945e4 100644
> > --- a/gdb/extension.h
> > +++ b/gdb/extension.h
> > @@ -295,6 +295,9 @@ extern enum ext_lang_bt_status
> apply_ext_lang_frame_filter
> >     enum ext_lang_frame_args args_type,
> >     struct ui_out *out, int frame_low, int frame_high);
> >
> > +extern void apply_ext_lang_ptwrite_filter
> > +  (struct btrace_thread_info *btinfo);
> > +
> >  extern void preserve_ext_lang_values (struct objfile *, htab_t
> copied_types);
> >
> >  extern const struct extension_language_defn
> *get_breakpoint_cond_ext_lang
> > diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> > index 887b7fa5dc8..e9b0d4127d5 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_filter, */
> >
> >    gdbscm_preserve_values,
> >
> > diff --git a/gdb/python/lib/gdb/ptwrite.py
> b/gdb/python/lib/gdb/ptwrite.py
> > new file mode 100644
> > index 00000000000..0f5b0473023
> > --- /dev/null
> > +++ b/gdb/python/lib/gdb/ptwrite.py
> > @@ -0,0 +1,80 @@
> > +# Ptwrite utilities.
> > +# Copyright (C) 2022 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +"""Utilities for working with ptwrite filters."""
> > +
> > +from copy import deepcopy
> > +import gdb
> > +
> > +
> > +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}
> > +
> > +
> > +def ptwrite_exit_handler(event):
> > +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> > +    for key in list(_ptwrite_filter.keys()):
> 
> I don't think the list() is needed.

It actually is needed, as I re-size the array:

>>> mydict = {1: "one", 2: "two"}
>>> for k in mydict.keys():
...     if k == 2:
...         del mydict[k]
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: dictionary changed size during iteration

> > +        if key.startswith(f"{event.inferior.pid}."):
> 
> Instead of using a string as a key, you could use a tuple, (pid, lwp).
> So here you would check key[0].

Done. I will use (-1, -1) as the key for the template filter.

> > +            del _ptwrite_filter[key]
> > +
> > +
> > +gdb.events.exited.connect(ptwrite_exit_handler)
> > +
> > +
> > +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:
> > +        for thread in thread_list:
> > +            thread.switch()
> > +            recording.clear()
> 
> I guess that answers my question on the previous patch about the use
> case of that clear method.  If that's the only goal of the clear method,
> and if there are no use case for the user to call it, I think it would
> be better to have an internal function instead.
> 
> Can you explain why its needed to switch threads and call clear on the
> same recording object, but once with each thread selected?  I thought
> the recording would be per-inferior, and one call to clear would clear
> the data for all threads of that inferior.

The recordings are per thread not per Inferior. Though I don't think you
can actually record only one thread of an inferior, as GDB will always
start a recording for all threads, or start recording a new thread if the
existing thread is already recording.

We want to allow internal state of a filter function to be maintained
per thread. Hence the copy and clearing for all threads of the inferior.
I implemented the clear() patch based on feedback from Markus.
Note that there is already "maintenance btrace clear" in CLI.

That said, you are right, I shouldn't clear the same object. In the first
version of this series I still had it right, but changed it for V2.
I can't exactly remember why.

This still leaves the problem of multiple inferiors, that you also
commented on below.
I think our actual idea was to have this globally, for all inferiors.
But that means I would need to clear all traces for all inferiors above.
Not just all threads of the current inferior.
That can easily be done, unless our other discussions go into a different
direction, and we make this per-inferior.

So I would change this to something like this:

def _clear_traces():
    """Helper function to clear the trace of all threads."""
    current_thread = gdb.selected_thread()

    for inferior in gdb.inferiors():
        for thread in inferior.threads():
            thread.switch()
            recording = gdb.current_recording()
            if recording is not None:
                recording.clear()

    current_thread.switch()

Not sure if it is worth exiting early per inferior. I couldn't find
a scenario where you can selectively record (or not record)
one thread of a multi-threaded inferior.

> > +
> > +        current_thread.switch()
> > +
> > +
> > +def register_filter(filter_):
> > +    """Register the ptwrite filter function."""
> > +    if filter_ is not None and not callable(filter_):
> > +        raise TypeError("The filter must be callable or 'None'.")
> > +
> > +    # Clear the traces of all threads to force re-decoding with
> > +    # the new filter.
> > +    thread_list = gdb.selected_inferior().threads()
> > +    _clear_traces(thread_list)
> 
> Could we avoid relying on the selected inferior?  For instance, it could
> be a method in Inferior:
> 
>   inf.register_ptwrite_filter(f)
> 
> or pass an inferior to this function:
> 
>   ptwrite.register_filter(inf, f)
> 
> I don't like having to switch inferior all the time in GDB to do
> per-inferior stuff, I don't want to impose that to our Python users :).

I get where you are coming from. Though one could argue that this is already
the case today. Starting recordings is already per inferior.
If we have two inferiors and start recording one we don't automatically
record the second one. And if we have one inferior that is being recorded,
and add a second, the second also won't be recorded. You also don't have
any option, apart from switching inferiors and starting the recording manually.

For threads that is different. Spawning threads will also be recorded
automatically. And if you have two threads and start recording, both
will be recorded.

What you suggest is definitely possible, years ago I had a version that
would allow registering different filters per thread, but we decided against
that.

> > +
> > +    _ptwrite_filter.clear()
> > +    _ptwrite_filter["global"] = filter_
> 
> Hmm, above you (seem to) clear the data for all threads of the selected
> inferior, implying that setting a filter affects only the current
> inferior.  But here you clear the whole _ptwrite_filter, which could
> contain filters for other inferiors.  I don't understand.

You are right, That seems confusing. See the comment two above for an
answer.

> > +
> > +
> > +def get_filter():
> > +    """Returns the filters of the current thread."""
> > +    # The key is of this format to enable an per-inferior cleanup when an
> > +    # inferior exits.
> > +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
> 
> I don't think we can use f-strings, we claim to support back to Python
> 3.4.

Did we write down what we support clearly? The gdb/README actually says 3.2.

And I think it might be good to mention this here:
https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

It seems like there are already some f strings in the master branch:

./lib/gdb/dap/io.py:63:            header = f"Content-Length: {len(body_bytes)}\r\n\r\n"
./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")

I just did a quick grep and didn't look at all occasions.

That said, I will remove f strings.

> You could perhaps also pass the key or the pid + lwp from the C++ code
> (taken from inferior_ptid).  Calling selected_inferior twice is just
> more churn to get the same info.

This isn't supposed to be limited to be called from C++ code.
And I think having it without any arguments is a bit easier to use from python.
I changed it according to your prior suggestions and made it a tuple.
But I guess this also depends on our per-inferior discussion above.

> > +
> > +    # Create a new filter for new threads.
> > +    if key not in _ptwrite_filter.keys():
> 
> .keys() is not needed

Right, I removed it.

> > +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> 
> Why is the deepcopy needed.  Because users could register a complex
> object with a __call__ method, in which case a simple copy would not do
> the same as deepcopy?  Just wondering if that's what users would expect.

We wanted to maintain internal state per thread in an easy fashion.
As you already saw in Patch 10.

> > +
> > +    return _ptwrite_filter[key]
> > diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-
> btrace.c
> > index 4922154fc1b..100a9ee8578 100644
> > --- a/gdb/python/py-record-btrace.c
> > +++ b/gdb/python/py-record-btrace.c
> > @@ -763,6 +763,94 @@ recpy_bt_function_call_history (PyObject *self,
> void *closure)
> >    return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
> >  }
> >
> > +/* Helper function that calls PTW_FILTER with PAYLOAD and IP as
> arguments.
> > +   Returns the string that will be printed.  */
> > +std::string
> > +recpy_call_filter (const uint64_t payload, const uint64_t ip,
> > +		   const void *ptw_filter)
> 
> Should be static.

Done.

> > +{
> > +  std::string result;
> > +
> > +  if ((PyObject *) ptw_filter == nullptr)
> > +    error (_("No valid ptwrite filter."));
> 
> I think this can only happen if get_ptwrite_filter has failed, in
> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
> written as:
> 
>     btinfo->ptw_context = get_ptwrite_filter ();
>     if (btinfo->ptw_context != nullptr)
>       btinfo->ptw_callback_fun = &recpy_call_filter;
> 
> ... such that recpy_call_filter is only installed when there is
> something in ptw_context.  recpy_call_filter could then do:
> 
>   gdb_assert (ptw_filter != nullptr);

I guess I could do that. Yet we check ptw_callback_fun before calling it
in btrace.c. See patch 10. So that alone would actually hide problems to the user.
And just not print any ptwrite string silently. Even if the user gave a custom
listener function. So we would also need to remove that check.

I am also wondering what the advantage you see is.
That we have an assert instead of an error? I guess I could do that even
without the change in gdbpy_load_ptwrite_filter.

> > +  if ((PyObject *) ptw_filter == Py_None)
> > +    return result;
> > +
> > +  gdbpy_enter enter_py;
> > +
> > +  gdbpy_ref<> py_payload (PyLong_FromUnsignedLongLong (payload));
> > +  gdbpy_ref<> py_ip (PyLong_FromUnsignedLongLong (ip));
> > +
> > +  if (ip == 0)
> > +    py_ip = gdbpy_ref<>::new_reference (Py_None);
> > +
> > +  gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *)
> ptw_filter,
> > +							py_payload.get (),
> > +							py_ip.get (),
> > +							nullptr));
> > +
> > +  if (PyErr_Occurred ())
> > +    {
> > +      gdbpy_print_stack ();
> > +      gdbpy_error (_("Couldn't call the ptwrite filter."));
> > +    }
> > +
> > +  /* Py_None is valid and results in no output.  */
> > +  if (py_result == Py_None)
> > +    return result;
> > +
> > +  result = gdbpy_obj_to_string (py_result.get ()).get ();
> > +
> > +  if (PyErr_Occurred ())
> > +    {
> > +      gdbpy_print_stack ();
> > +      gdbpy_error (_("The ptwrite filter didn't return a string."));
> > +    }
> > +
> > +  return result;
> > +}
> > +
> > +/* Helper function returning the current ptwrite filter.  */
> > +
> > +PyObject *
> > +get_ptwrite_filter ()
> 
> Should be static.

Will be done in the next version.

> > +/* Used for registering the default ptwrite filter to the current thread.  A
> > +   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)
> > +{
> > +  gdb_assert (btinfo != nullptr);
> > +
> > +  gdbpy_enter enter_py;
> > +
> > +  btinfo->ptw_callback_fun = &recpy_call_filter;
> > +  btinfo->ptw_context= get_ptwrite_filter ();
> 
> Missing space.

Fixed in the next version.

> > diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-
> btrace.h
> > index f297772f946..12750b3b1c3 100644
> > --- a/gdb/python/py-record-btrace.h
> > +++ b/gdb/python/py-record-btrace.h
> > @@ -91,4 +91,12 @@ extern PyObject *recpy_bt_func_prev (PyObject
> *self, void *closure);
> >  /* Implementation of RecordFunctionSegment.next
> [RecordFunctionSegment].  */
> >  extern PyObject *recpy_bt_func_next (PyObject *self, void *closure);
> >
> > +/* Helper function returning the current ptwrite filter.  */
> > +extern PyObject *get_ptwrite_filter ();
> > +
> > +/* Helper function to call the ptwrite filter.  */
> > +extern std::string recpy_call_filter (const uint64_t payload,
> > +				      const uint64_t ip,
> > +				      const void *ptw_filter);
> > +
> >  #endif /* PYTHON_PY_RECORD_BTRACE_H */
> > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> > index 258f5c42537..c21232c07dc 100644
> > --- a/gdb/python/python-internal.h
> > +++ b/gdb/python/python-internal.h
> > @@ -376,6 +376,9 @@ extern enum ext_lang_rc
> gdbpy_apply_val_pretty_printer
> >     struct ui_file *stream, int recurse,
> >     const struct value_print_options *options,
> >     const struct language_defn *language);
> > +extern void gdbpy_load_ptwrite_filter
> > +  (const struct extension_language_defn *extlang,
> > +   struct btrace_thread_info *btinfo);
> 
> Can this declaration be in py-record-btrace.h?  I really don't see why
> all the declarations are stuffed into python.h.  Also, please move the
> comment to the declaration, and put the appropriate /* See ... */
> comment on the definition.

I don't think so. I modelled this after the other extension language interfaces.
These are also all in here, without any comment. This function is being called
via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
As far as I understand it, that is how you interact with the python layer.
To also allow the same things to be done in guile, if someone feels like it.
Please tell me if I misunderstood the architecture.

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

>>> +def ptwrite_exit_handler(event):
>>> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
>>> +    for key in list(_ptwrite_filter.keys()):
>>
>> I don't think the list() is needed.
> 
> It actually is needed, as I re-size the array:
> 
>>>> mydict = {1: "one", 2: "two"}
>>>> for k in mydict.keys():
> ...     if k == 2:
> ...         del mydict[k]
> ... 
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> RuntimeError: dictionary changed size during iteration
Ah ok, it's to take a "snapshot" of the keys.

>> I guess that answers my question on the previous patch about the use
>> case of that clear method.  If that's the only goal of the clear method,
>> and if there are no use case for the user to call it, I think it would
>> be better to have an internal function instead.
>>
>> Can you explain why its needed to switch threads and call clear on the
>> same recording object, but once with each thread selected?  I thought
>> the recording would be per-inferior, and one call to clear would clear
>> the data for all threads of that inferior.
> 
> The recordings are per thread not per Inferior. Though I don't think you
> can actually record only one thread of an inferior, as GDB will always
> start a recording for all threads, or start recording a new thread if the
> existing thread is already recording.
> 
> We want to allow internal state of a filter function to be maintained
> per thread. Hence the copy and clearing for all threads of the inferior.
> I implemented the clear() patch based on feedback from Markus.
> Note that there is already "maintenance btrace clear" in CLI.
> 
> That said, you are right, I shouldn't clear the same object. In the first
> version of this series I still had it right, but changed it for V2.
> I can't exactly remember why.
> 
> This still leaves the problem of multiple inferiors, that you also
> commented on below.
> I think our actual idea was to have this globally, for all inferiors.
> But that means I would need to clear all traces for all inferiors above.
> Not just all threads of the current inferior.
> That can easily be done, unless our other discussions go into a different
> direction, and we make this per-inferior.
> 
> So I would change this to something like this:
> 
> def _clear_traces():
>     """Helper function to clear the trace of all threads."""
>     current_thread = gdb.selected_thread()
> 
>     for inferior in gdb.inferiors():
>         for thread in inferior.threads():
>             thread.switch()
>             recording = gdb.current_recording()
>             if recording is not None:
>                 recording.clear()
> 
>     current_thread.switch()
> 
> Not sure if it is worth exiting early per inferior. I couldn't find
> a scenario where you can selectively record (or not record)
> one thread of a multi-threaded inferior.

For our internal Python helper code, it's fine to do things based on
some internal knowledge of how GDB is implemented.  So, if we know what
doing "clear" on one thread clears the recording for the whole inferior,
then we can do just one thread per inferior.  If GDB changes, we can
adjust that code at the same time.

But seeing this, I think that from a Python user point of view, the
clear method you are proposing is a bit confusing.  It is called on a
Record object, which is per-thread, but it clears the recording for all
threads of the inferior.  This would be unexpected, unless documented as
such in the doc (but even then, it feels a bit awkward).

The doc talks about "the current recording" here and there, but it's
unclear what the scope of the recording is.

>>> +
>>> +        current_thread.switch()
>>> +
>>> +
>>> +def register_filter(filter_):
>>> +    """Register the ptwrite filter function."""
>>> +    if filter_ is not None and not callable(filter_):
>>> +        raise TypeError("The filter must be callable or 'None'.")
>>> +
>>> +    # Clear the traces of all threads to force re-decoding with
>>> +    # the new filter.
>>> +    thread_list = gdb.selected_inferior().threads()
>>> +    _clear_traces(thread_list)
>>
>> Could we avoid relying on the selected inferior?  For instance, it could
>> be a method in Inferior:
>>
>>   inf.register_ptwrite_filter(f)
>>
>> or pass an inferior to this function:
>>
>>   ptwrite.register_filter(inf, f)
>>
>> I don't like having to switch inferior all the time in GDB to do
>> per-inferior stuff, I don't want to impose that to our Python users :).
> 
> I get where you are coming from. Though one could argue that this is already
> the case today. Starting recordings is already per inferior.
> If we have two inferiors and start recording one we don't automatically
> record the second one. And if we have one inferior that is being recorded,
> and add a second, the second also won't be recorded. You also don't have
> any option, apart from switching inferiors and starting the recording manually.

I think it's a flaw in the current API that you need to switch current
inferior to start recording on a specific inferior.  And also, it's a
flaw in the documentation that it doesn't state that the recording is
started for the current inferior.  The user is left to guess (or must
experiment, or read the GDB source code, or have bad surprises) about
what happens when you have multiple inferiors.

But if I understood you clearly, register_filter is really meant to
install a global filter (valid for all inferiors, current and future).
And the call to _clear_traces is meant to clear all traces of all
inferiors.  In that case, it's fine for register_ptwrite_filter to be
global.

> For threads that is different. Spawning threads will also be recorded
> automatically. And if you have two threads and start recording, both
> will be recorded.
> 
> What you suggest is definitely possible, years ago I had a version that
> would allow registering different filters per thread, but we decided against
> that.

I don't think that is what I suggested.  If your experience suggests
that having just one global filter is enough, I can't argue with that.
Plus, it would probably be possible to implement per-inferior or
per-thread filters in the future if needed, that would take precedence
on the global filter.

My point is just that an API that requires the user to switch context,
then make an API call, then restore the original context, is bad.  It's
unclear and error prone.  I know we have some of that already, but I
would like to avoid adding more.

>>> +
>>> +
>>> +def get_filter():
>>> +    """Returns the filters of the current thread."""
>>> +    # The key is of this format to enable an per-inferior cleanup when an
>>> +    # inferior exits.
>>> +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
>>
>> I don't think we can use f-strings, we claim to support back to Python
>> 3.4.
> 
> Did we write down what we support clearly? The gdb/README actually says 3.2.

Sorry, I said 3.4 because I remembered sending a patch that raised the
minimum level to 3.4.  But that patch was rejected, so it's plausible we
still support 3.2:

  https://inbox.sourceware.org/gdb-patches/20220107152921.2858909-3-simon.marchi@polymtl.ca/#t

> And I think it might be good to mention this here:
> https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

Feel free to edit that page.  However, the risk of having it documented
at multiple places is that we can easily forget to update one spot.
Maybe put a link to the README, and say "make sure your changes are
compatible with the minimum Python version mentioned in the README"?

> It seems like there are already some f strings in the master branch:
> 
> ./lib/gdb/dap/io.py:63:            header = f"Content-Length: {len(body_bytes)}\r\n\r\n"
> ./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
> ./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")
> 
> I just did a quick grep and didn't look at all occasions.
> 
> That said, I will remove f strings.

Probably erroneous then.  Since this is specifically in DAP code, would
someone using Python 3.4 hit an error when _not_ using DAP?  I would
guess not, probably why this wasn't noticed.  Or, nobody tests with
older Python versions...

>> You could perhaps also pass the key or the pid + lwp from the C++ code
>> (taken from inferior_ptid).  Calling selected_inferior twice is just
>> more churn to get the same info.
> 
> This isn't supposed to be limited to be called from C++ code.

Ok, I didn't consider that.

One thing to note is that there isn't always a selected thread, so you
might want to handle that appropriately.

> And I think having it without any arguments is a bit easier to use from python.
> I changed it according to your prior suggestions and made it a tuple.
> But I guess this also depends on our per-inferior discussion above.

Indeed.  I had not noticed previously, but what I said earlier is true
for get_filter as well.  I would much rather see get_filter take a
thread as argument, than relying on the selected thread.

> 
>>> +
>>> +    # Create a new filter for new threads.
>>> +    if key not in _ptwrite_filter.keys():
>>> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>>
>> Why is the deepcopy needed.  Because users could register a complex
>> object with a __call__ method, in which case a simple copy would not do
>> the same as deepcopy?  Just wondering if that's what users would expect.
> 
> We wanted to maintain internal state per thread in an easy fashion.
> As you already saw in Patch 10.

I understand that you want to have one copy of the filter per thread.

But doing a copy seems like a strange instantiation strategy.  The
"global" filter isn't ever used, right?  It only serves to make copies?
I think a more common pattern would be for the user to register a
factory function (really, ny callable that fits the bill) which would be
called whenever we need to create an instance.

>>> +{
>>> +  std::string result;
>>> +
>>> +  if ((PyObject *) ptw_filter == nullptr)
>>> +    error (_("No valid ptwrite filter."));
>>
>> I think this can only happen if get_ptwrite_filter has failed, in
>> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
>> written as:
>>
>>     btinfo->ptw_context = get_ptwrite_filter ();
>>     if (btinfo->ptw_context != nullptr)
>>       btinfo->ptw_callback_fun = &recpy_call_filter;
>>
>> ... such that recpy_call_filter is only installed when there is
>> something in ptw_context.  recpy_call_filter could then do:
>>
>>   gdb_assert (ptw_filter != nullptr);
> 
> I guess I could do that. Yet we check ptw_callback_fun before calling it
> in btrace.c. See patch 10. So that alone would actually hide problems to the user.
> And just not print any ptwrite string silently. Even if the user gave a custom
> listener function. So we would also need to remove that check.
> 
> I am also wondering what the advantage you see is.
> That we have an assert instead of an error? I guess I could do that even
> without the change in gdbpy_load_ptwrite_filter.

Just trying to choose the right tool for the job, which in turns helps
build the right mental model.  error() calls are for errors that aren't
GDB's fault, errors that are not the result of a bug in GDB.  gdb_assert
is to catch things programming errors in GDB.

If I see:

  gdb_assert (foo == nullptr);

I conclude that there should be not situation where foo is nullptr.  If
I see:

  if (foo -- nullptr)
    error (...)

Then I conclude there are some valid situations where foo is nullptr
(and then we have to worry about how they are handled).

>>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
>>> index 258f5c42537..c21232c07dc 100644
>>> --- a/gdb/python/python-internal.h
>>> +++ b/gdb/python/python-internal.h
>>> @@ -376,6 +376,9 @@ extern enum ext_lang_rc
>> gdbpy_apply_val_pretty_printer
>>>     struct ui_file *stream, int recurse,
>>>     const struct value_print_options *options,
>>>     const struct language_defn *language);
>>> +extern void gdbpy_load_ptwrite_filter
>>> +  (const struct extension_language_defn *extlang,
>>> +   struct btrace_thread_info *btinfo);
>>
>> Can this declaration be in py-record-btrace.h?  I really don't see why
>> all the declarations are stuffed into python.h.  Also, please move the
>> comment to the declaration, and put the appropriate /* See ... */
>> comment on the definition.
> 
> I don't think so. I modelled this after the other extension language interfaces.
> These are also all in here, without any comment. This function is being called
> via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
> As far as I understand it, that is how you interact with the python layer.
> To also allow the same things to be done in guile, if someone feels like it.
> Please tell me if I misunderstood the architecture.

I think you're right that everything goes through python/python.h.  I
don't know why though, we we can't have functions declared in various .h
files.  Perhaps it's easier to manage for when building with Python
disabled, I don't know.  In any case, not really important right now,
sorry for bringing that up.

Simon
  

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Montag, 3. April 2023 22:45
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> >>> +def ptwrite_exit_handler(event):
> >>> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> >>> +    for key in list(_ptwrite_filter.keys()):
> >>
> >> I don't think the list() is needed.
> >
> > It actually is needed, as I re-size the array:
> >
> >>>> mydict = {1: "one", 2: "two"}
> >>>> for k in mydict.keys():
> > ...     if k == 2:
> > ...         del mydict[k]
> > ...
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> > RuntimeError: dictionary changed size during iteration
> Ah ok, it's to take a "snapshot" of the keys.
> 
> >> I guess that answers my question on the previous patch about the use
> >> case of that clear method.  If that's the only goal of the clear method,
> >> and if there are no use case for the user to call it, I think it would
> >> be better to have an internal function instead.
> >>
> >> Can you explain why its needed to switch threads and call clear on the
> >> same recording object, but once with each thread selected?  I thought
> >> the recording would be per-inferior, and one call to clear would clear
> >> the data for all threads of that inferior.
> >
> > The recordings are per thread not per Inferior. Though I don't think you
> > can actually record only one thread of an inferior, as GDB will always
> > start a recording for all threads, or start recording a new thread if the
> > existing thread is already recording.
> >
> > We want to allow internal state of a filter function to be maintained
> > per thread. Hence the copy and clearing for all threads of the inferior.
> > I implemented the clear() patch based on feedback from Markus.
> > Note that there is already "maintenance btrace clear" in CLI.
> >
> > That said, you are right, I shouldn't clear the same object. In the first
> > version of this series I still had it right, but changed it for V2.
> > I can't exactly remember why.
> >
> > This still leaves the problem of multiple inferiors, that you also
> > commented on below.
> > I think our actual idea was to have this globally, for all inferiors.
> > But that means I would need to clear all traces for all inferiors above.
> > Not just all threads of the current inferior.
> > That can easily be done, unless our other discussions go into a different
> > direction, and we make this per-inferior.
> >
> > So I would change this to something like this:
> >
> > def _clear_traces():
> >     """Helper function to clear the trace of all threads."""
> >     current_thread = gdb.selected_thread()
> >
> >     for inferior in gdb.inferiors():
> >         for thread in inferior.threads():
> >             thread.switch()
> >             recording = gdb.current_recording()
> >             if recording is not None:
> >                 recording.clear()
> >
> >     current_thread.switch()
> >
> > Not sure if it is worth exiting early per inferior. I couldn't find
> > a scenario where you can selectively record (or not record)
> > one thread of a multi-threaded inferior.
> 
> For our internal Python helper code, it's fine to do things based on
> some internal knowledge of how GDB is implemented.  So, if we know what
> doing "clear" on one thread clears the recording for the whole inferior,
> then we can do just one thread per inferior.  If GDB changes, we can
> adjust that code at the same time.
> 
> But seeing this, I think that from a Python user point of view, the
> clear method you are proposing is a bit confusing.  It is called on a
> Record object, which is per-thread, but it clears the recording for all
> threads of the inferior.  This would be unexpected, unless documented as
> such in the doc (but even then, it feels a bit awkward).
> 
> The doc talks about "the current recording" here and there, but it's
> unclear what the scope of the recording is.

I agree on the doc part. It would indeed be nice to write this down.

Though, I think you misunderstood some things (or maybe I did).
Gdb.current_recording().clear(), as added in patch 5, does clear on a 
per-thread basis. Not on a per inferior basis as you write.
But maybe you meant _clear_traces() here? This is supposed to be
an internal function, as the leading underscore suggests. It indeed
does clear all recordings for all threads. As that is needed for ptwrite,
which is the only intended use case of this internal function.

So I don't quite see what would be unexpected.

> >>> +
> >>> +        current_thread.switch()
> >>> +
> >>> +
> >>> +def register_filter(filter_):
> >>> +    """Register the ptwrite filter function."""
> >>> +    if filter_ is not None and not callable(filter_):
> >>> +        raise TypeError("The filter must be callable or 'None'.")
> >>> +
> >>> +    # Clear the traces of all threads to force re-decoding with
> >>> +    # the new filter.
> >>> +    thread_list = gdb.selected_inferior().threads()
> >>> +    _clear_traces(thread_list)
> >>
> >> Could we avoid relying on the selected inferior?  For instance, it could
> >> be a method in Inferior:
> >>
> >>   inf.register_ptwrite_filter(f)
> >>
> >> or pass an inferior to this function:
> >>
> >>   ptwrite.register_filter(inf, f)
> >>
> >> I don't like having to switch inferior all the time in GDB to do
> >> per-inferior stuff, I don't want to impose that to our Python users :).
> >
> > I get where you are coming from. Though one could argue that this is
> already
> > the case today. Starting recordings is already per inferior.
> > If we have two inferiors and start recording one we don't automatically
> > record the second one. And if we have one inferior that is being recorded,
> > and add a second, the second also won't be recorded. You also don't have
> > any option, apart from switching inferiors and starting the recording
> manually.
> 
> I think it's a flaw in the current API that you need to switch current
> inferior to start recording on a specific inferior.  And also, it's a
> flaw in the documentation that it doesn't state that the recording is
> started for the current inferior.  The user is left to guess (or must
> experiment, or read the GDB source code, or have bad surprises) about
> what happens when you have multiple inferiors.

I agree.

> But if I understood you clearly, register_filter is really meant to
> install a global filter (valid for all inferiors, current and future).
> And the call to _clear_traces is meant to clear all traces of all
> inferiors.  In that case, it's fine for register_ptwrite_filter to be
> global.

That is right.

> > For threads that is different. Spawning threads will also be recorded
> > automatically. And if you have two threads and start recording, both
> > will be recorded.
> >
> > What you suggest is definitely possible, years ago I had a version that
> > would allow registering different filters per thread, but we decided against
> > that.
> 
> I don't think that is what I suggested.  If your experience suggests
> that having just one global filter is enough, I can't argue with that.
> Plus, it would probably be possible to implement per-inferior or
> per-thread filters in the future if needed, that would take precedence
> on the global filter.
> 
> My point is just that an API that requires the user to switch context,
> then make an API call, then restore the original context, is bad.  It's
> unclear and error prone.  I know we have some of that already, but I
> would like to avoid adding more.

I again see your points. Above you wrote:

Simon> In that case, it's fine for register_ptwrite_filter to be
Simon> global.

So I won't change anything here now. Extending it in the future should be
simple, I agree.

> >>> +
> >>> +
> >>> +def get_filter():
> >>> +    """Returns the filters of the current thread."""
> >>> +    # The key is of this format to enable an per-inferior cleanup when an
> >>> +    # inferior exits.
> >>> +    key =
> f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
> >>
> >> I don't think we can use f-strings, we claim to support back to Python
> >> 3.4.
> >
> > Did we write down what we support clearly? The gdb/README actually
> says 3.2.
> 
> Sorry, I said 3.4 because I remembered sending a patch that raised the
> minimum level to 3.4.  But that patch was rejected, so it's plausible we
> still support 3.2:
> 
>   https://inbox.sourceware.org/gdb-patches/20220107152921.2858909-3-
> simon.marchi@polymtl.ca/#t
> 
> > And I think it might be good to mention this here:
> > https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-
> Standards
> 
> Feel free to edit that page.  However, the risk of having it documented
> at multiple places is that we can easily forget to update one spot.
> Maybe put a link to the README, and say "make sure your changes are
> compatible with the minimum Python version mentioned in the README"?

Good point, yes a single source of truth is much better.
I am no longer thinking it is a must and I don't think I can edit it anyway.

> > It seems like there are already some f strings in the master branch:
> >
> > ./lib/gdb/dap/io.py:63:            header = f"Content-Length:
> {len(body_bytes)}\r\n\r\n"
> > ./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
> > ./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")
> >
> > I just did a quick grep and didn't look at all occasions.
> >
> > That said, I will remove f strings.
> 
> Probably erroneous then.  Since this is specifically in DAP code, would
> someone using Python 3.4 hit an error when _not_ using DAP?  I would
> guess not, probably why this wasn't noticed.  Or, nobody tests with
> older Python versions...
> 
> >> You could perhaps also pass the key or the pid + lwp from the C++ code
> >> (taken from inferior_ptid).  Calling selected_inferior twice is just
> >> more churn to get the same info.
> >
> > This isn't supposed to be limited to be called from C++ code.
> 
> Ok, I didn't consider that.
> 
> One thing to note is that there isn't always a selected thread, so you
> might want to handle that appropriately.
> 
> > And I think having it without any arguments is a bit easier to use from
> python.
> > I changed it according to your prior suggestions and made it a tuple.
> > But I guess this also depends on our per-inferior discussion above.
> 
> Indeed.  I had not noticed previously, but what I said earlier is true
> for get_filter as well.  I would much rather see get_filter take a
> thread as argument, than relying on the selected thread.

Okay, I will add this. Probably as an optional argument, as I think
that makes it much easier for single threaded users.

> >
> >>> +
> >>> +    # Create a new filter for new threads.
> >>> +    if key not in _ptwrite_filter.keys():
> >>> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> >>
> >> Why is the deepcopy needed.  Because users could register a complex
> >> object with a __call__ method, in which case a simple copy would not do
> >> the same as deepcopy?  Just wondering if that's what users would expect.
> >
> > We wanted to maintain internal state per thread in an easy fashion.
> > As you already saw in Patch 10.
> 
> I understand that you want to have one copy of the filter per thread.
> 
> But doing a copy seems like a strange instantiation strategy.  The
> "global" filter isn't ever used, right?  It only serves to make copies?
> I think a more common pattern would be for the user to register a
> factory function (really, ny callable that fits the bill) which would be
> called whenever we need to create an instance.

Mhm, I think that would make this a bit too complex for users.
I don't think they want to care about providing a generator as well
that returns a callable. I don't see any new feature that
is being unlocked with that, just more tasks for the user.

And if it does at some point, the code should be straightforward to
extend in the future.

> >>> +{
> >>> +  std::string result;
> >>> +
> >>> +  if ((PyObject *) ptw_filter == nullptr)
> >>> +    error (_("No valid ptwrite filter."));
> >>
> >> I think this can only happen if get_ptwrite_filter has failed, in
> >> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
> >> written as:
> >>
> >>     btinfo->ptw_context = get_ptwrite_filter ();
> >>     if (btinfo->ptw_context != nullptr)
> >>       btinfo->ptw_callback_fun = &recpy_call_filter;
> >>
> >> ... such that recpy_call_filter is only installed when there is
> >> something in ptw_context.  recpy_call_filter could then do:
> >>
> >>   gdb_assert (ptw_filter != nullptr);
> >
> > I guess I could do that. Yet we check ptw_callback_fun before calling it
> > in btrace.c. See patch 10. So that alone would actually hide problems to the
> user.
> > And just not print any ptwrite string silently. Even if the user gave a custom
> > listener function. So we would also need to remove that check.
> >
> > I am also wondering what the advantage you see is.
> > That we have an assert instead of an error? I guess I could do that even
> > without the change in gdbpy_load_ptwrite_filter.
> 
> Just trying to choose the right tool for the job, which in turns helps
> build the right mental model.  error() calls are for errors that aren't
> GDB's fault, errors that are not the result of a bug in GDB.  gdb_assert
> is to catch things programming errors in GDB.
> 
> If I see:
> 
>   gdb_assert (foo == nullptr);
> 
> I conclude that there should be not situation where foo is nullptr.  If
> I see:
> 
>   if (foo -- nullptr)
>     error (...)
> 
> Then I conclude there are some valid situations where foo is nullptr
> (and then we have to worry about how they are handled).

I will change the error in recpy_call_filter to an assert.
I still don't see the point of not registering ptw_callback_fun().
I fear that will just hide things.
Anyway, I will look at this again when implementing the printing in C++
that you suggested for builds without Python. I think the picture
might be clearer once I have that.

> >>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-
> internal.h
> >>> index 258f5c42537..c21232c07dc 100644
> >>> --- a/gdb/python/python-internal.h
> >>> +++ b/gdb/python/python-internal.h
> >>> @@ -376,6 +376,9 @@ extern enum ext_lang_rc
> >> gdbpy_apply_val_pretty_printer
> >>>     struct ui_file *stream, int recurse,
> >>>     const struct value_print_options *options,
> >>>     const struct language_defn *language);
> >>> +extern void gdbpy_load_ptwrite_filter
> >>> +  (const struct extension_language_defn *extlang,
> >>> +   struct btrace_thread_info *btinfo);
> >>
> >> Can this declaration be in py-record-btrace.h?  I really don't see why
> >> all the declarations are stuffed into python.h.  Also, please move the
> >> comment to the declaration, and put the appropriate /* See ... */
> >> comment on the definition.
> >
> > I don't think so. I modelled this after the other extension language
> interfaces.
> > These are also all in here, without any comment. This function is being
> called
> > via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
> > As far as I understand it, that is how you interact with the python layer.
> > To also allow the same things to be done in guile, if someone feels like it.
> > Please tell me if I misunderstood the architecture.
> 
> I think you're right that everything goes through python/python.h.  I
> don't know why though, we we can't have functions declared in various .h
> files.  Perhaps it's easier to manage for when building with Python
> disabled, I don't know.  In any case, not really important right now,
> sorry for bringing that up.

No worries, I appreciate the review!

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

> I agree on the doc part. It would indeed be nice to write this down.
> 
> Though, I think you misunderstood some things (or maybe I did).
> Gdb.current_recording().clear(), as added in patch 5, does clear on a 
> per-thread basis. Not on a per inferior basis as you write.
> But maybe you meant _clear_traces() here? This is supposed to be
> an internal function, as the leading underscore suggests. It indeed
> does clear all recordings for all threads. As that is needed for ptwrite,
> which is the only intended use case of this internal function.
> 
> So I don't quite see what would be unexpected.
My misunderstanding comes from where you said:


> Not sure if it is worth exiting early per inferior. I couldn't find
> a scenario where you can selectively record (or not record)
> one thread of a multi-threaded inferior.

By "exiting early", I thought you meant that it would be enough to clear
the data for one thread in each inferior.

>> I understand that you want to have one copy of the filter per thread.
>>
>> But doing a copy seems like a strange instantiation strategy.  The
>> "global" filter isn't ever used, right?  It only serves to make copies?
>> I think a more common pattern would be for the user to register a
>> factory function (really, ny callable that fits the bill) which would be
>> called whenever we need to create an instance.
> 
> Mhm, I think that would make this a bit too complex for users.
> I don't think they want to care about providing a generator as well
> that returns a callable. I don't see any new feature that
> is being unlocked with that, just more tasks for the user.

I don't think it adds significant complexity, this is pretty typical
Python.  It is similar to what we have for the pretty printers.  You
register a function that gets called to instantiate the right pretty
printer.

At the simplest, it would be:

  class MyFilter:
    ...

  def make_one_filter():
    return MyFilter()

  register_filter(make_one_filter)

or even:

  class MyFilter:
    ...

  register_filter(lambda: MyFilter())

An advantage is that it makes it easy to return a different filter per
thread, if needed.  Since filters are on a per-thread basis, I think it
would even make sense to pass the thread to the factory function, in
case the user wants to select a filter based on that.

Not that the fact that filters are callable themselves is just a design
choice you made, you could very well decide to call something else than
__call__ on those objects.  If you want to make things more explicit,
you can make it so filters have to implement a "get_aux_data" method,
instead of __call__.  The advantage of using __call__ is just that it
allows freely passing functions as well as objects implementing
__call__, so that gives the user a bit more flexibility.

Simon
  

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Dienstag, 4. April 2023 17:06
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> > I agree on the doc part. It would indeed be nice to write this down.
> >
> > Though, I think you misunderstood some things (or maybe I did).
> > Gdb.current_recording().clear(), as added in patch 5, does clear on a
> > per-thread basis. Not on a per inferior basis as you write.
> > But maybe you meant _clear_traces() here? This is supposed to be
> > an internal function, as the leading underscore suggests. It indeed
> > does clear all recordings for all threads. As that is needed for ptwrite,
> > which is the only intended use case of this internal function.
> >
> > So I don't quite see what would be unexpected.
> My misunderstanding comes from where you said:
> 
> 
> > Not sure if it is worth exiting early per inferior. I couldn't find
> > a scenario where you can selectively record (or not record)
> > one thread of a multi-threaded inferior.
> 
> By "exiting early", I thought you meant that it would be enough to clear
> the data for one thread in each inferior.
> 
> >> I understand that you want to have one copy of the filter per thread.
> >>
> >> But doing a copy seems like a strange instantiation strategy.  The
> >> "global" filter isn't ever used, right?  It only serves to make copies?
> >> I think a more common pattern would be for the user to register a
> >> factory function (really, ny callable that fits the bill) which would be
> >> called whenever we need to create an instance.
> >
> > Mhm, I think that would make this a bit too complex for users.
> > I don't think they want to care about providing a generator as well
> > that returns a callable. I don't see any new feature that
> > is being unlocked with that, just more tasks for the user.
> 
> I don't think it adds significant complexity, this is pretty typical
> Python.  It is similar to what we have for the pretty printers.  You
> register a function that gets called to instantiate the right pretty
> printer.
> 
> At the simplest, it would be:
> 
>   class MyFilter:
>     ...
> 
>   def make_one_filter():
>     return MyFilter()
> 
>   register_filter(make_one_filter)
> 
> or even:
> 
>   class MyFilter:
>     ...
> 
>   register_filter(lambda: MyFilter())
> 
> An advantage is that it makes it easy to return a different filter per
> thread, if needed.  Since filters are on a per-thread basis, I think it
> would even make sense to pass the thread to the factory function, in
> case the user wants to select a filter based on that.
> 
> Not that the fact that filters are callable themselves is just a design
> choice you made, you could very well decide to call something else than
> __call__ on those objects.  If you want to make things more explicit,
> you can make it so filters have to implement a "get_aux_data" method,
> instead of __call__.  The advantage of using __call__ is just that it
> allows freely passing functions as well as objects implementing
> __call__, so that gives the user a bit more flexibility.
> 
> Simon

Sorry, I still don't understand this or see the advantage of the
factory pattern here.
And I only want to use a pattern if there is a clear advantage to it.

I assume you want to move the "housekeeping" to the user side, right?
Why do we need a factory pattern for that?
Can't you achieve the same if we just have one callable filter object
instead of one per thread? And have the user do it in that object, if
he needs to? (Per thread filters would still allow the same, though not
as clean, as they would have to call the singular object.)
Passing the thread can also be done with the filter function directly.
And is just a convenience, as the GDB API is available to the user.
(I am not saying, it is a bad convenience feature.)

AFAIK, you use a factory pattern when you want to abstract what
type of exact object is returned. I don't see that we need that.
And we already have a bit of freedom anyway, with allowing
callables.

If you don't like the per thread copy, we could still just not do that.
And move the per-thread or per-inferior housekeeping to the user.
I had this at some point. The documentation I wrote still gives a short
example for how to do something for one thread but not for another.

As far as I remember, Markus wanted per-thread state in an easier way
though.

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
  

On 4/5/23 06:20, Willgerodt, Felix wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Dienstag, 4. April 2023 17:06
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
>> registration.
>>
>>> I agree on the doc part. It would indeed be nice to write this down.
>>>
>>> Though, I think you misunderstood some things (or maybe I did).
>>> Gdb.current_recording().clear(), as added in patch 5, does clear on a
>>> per-thread basis. Not on a per inferior basis as you write.
>>> But maybe you meant _clear_traces() here? This is supposed to be
>>> an internal function, as the leading underscore suggests. It indeed
>>> does clear all recordings for all threads. As that is needed for ptwrite,
>>> which is the only intended use case of this internal function.
>>>
>>> So I don't quite see what would be unexpected.
>> My misunderstanding comes from where you said:
>>
>>
>>> Not sure if it is worth exiting early per inferior. I couldn't find
>>> a scenario where you can selectively record (or not record)
>>> one thread of a multi-threaded inferior.
>>
>> By "exiting early", I thought you meant that it would be enough to clear
>> the data for one thread in each inferior.
>>
>>>> I understand that you want to have one copy of the filter per thread.
>>>>
>>>> But doing a copy seems like a strange instantiation strategy.  The
>>>> "global" filter isn't ever used, right?  It only serves to make copies?
>>>> I think a more common pattern would be for the user to register a
>>>> factory function (really, ny callable that fits the bill) which would be
>>>> called whenever we need to create an instance.
>>>
>>> Mhm, I think that would make this a bit too complex for users.
>>> I don't think they want to care about providing a generator as well
>>> that returns a callable. I don't see any new feature that
>>> is being unlocked with that, just more tasks for the user.
>>
>> I don't think it adds significant complexity, this is pretty typical
>> Python.  It is similar to what we have for the pretty printers.  You
>> register a function that gets called to instantiate the right pretty
>> printer.
>>
>> At the simplest, it would be:
>>
>>   class MyFilter:
>>     ...
>>
>>   def make_one_filter():
>>     return MyFilter()
>>
>>   register_filter(make_one_filter)
>>
>> or even:
>>
>>   class MyFilter:
>>     ...
>>
>>   register_filter(lambda: MyFilter())
>>
>> An advantage is that it makes it easy to return a different filter per
>> thread, if needed.  Since filters are on a per-thread basis, I think it
>> would even make sense to pass the thread to the factory function, in
>> case the user wants to select a filter based on that.
>>
>> Not that the fact that filters are callable themselves is just a design
>> choice you made, you could very well decide to call something else than
>> __call__ on those objects.  If you want to make things more explicit,
>> you can make it so filters have to implement a "get_aux_data" method,
>> instead of __call__.  The advantage of using __call__ is just that it
>> allows freely passing functions as well as objects implementing
>> __call__, so that gives the user a bit more flexibility.
>>
>> Simon
> 
> Sorry, I still don't understand this or see the advantage of the
> factory pattern here.
> And I only want to use a pattern if there is a clear advantage to it.
> 
> I assume you want to move the "housekeeping" to the user side, right?

Not sure what you mean by "housekeeping".  In the end, with what I
propose, we still end up with one filter object per thread, it just
changes how they are instantiated.  Instead of copying a "template"
object, we call the "get me a new instance" function.

> Why do we need a factory pattern for that?
> Can't you achieve the same if we just have one callable filter object
> instead of one per thread? And have the user do it in that object, if
> he needs to? (Per thread filters would still allow the same, though not
> as clean, as they would have to call the singular object.)
> Passing the thread can also be done with the filter function directly.
> And is just a convenience, as the GDB API is available to the user.
> (I am not saying, it is a bad convenience feature.)
> 
> AFAIK, you use a factory pattern when you want to abstract what
> type of exact object is returned. I don't see that we need that.
> And we already have a bit of freedom anyway, with allowing
> callables.
>
> If you don't like the per thread copy, we could still just not do that.

With what I propose, there would still be one copy (instance) of the
filter per thread.  So maybe we're not on the same page.

> And move the per-thread or per-inferior housekeeping to the user.
> I had this at some point. The documentation I wrote still gives a short
> example for how to do something for one thread but not for another.
> 
> As far as I remember, Markus wanted per-thread state in an easier way
> though.

Simon
  

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Mittwoch, 5. April 2023 22:27
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> 
> 
> On 4/5/23 06:20, Willgerodt, Felix wrote:
> >
> >
> >> -----Original Message-----
> >> From: Simon Marchi <simark@simark.ca>
> >> Sent: Dienstag, 4. April 2023 17:06
> >> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> >> registration.
> >>
> >>> I agree on the doc part. It would indeed be nice to write this down.
> >>>
> >>> Though, I think you misunderstood some things (or maybe I did).
> >>> Gdb.current_recording().clear(), as added in patch 5, does clear on a
> >>> per-thread basis. Not on a per inferior basis as you write.
> >>> But maybe you meant _clear_traces() here? This is supposed to be
> >>> an internal function, as the leading underscore suggests. It indeed
> >>> does clear all recordings for all threads. As that is needed for ptwrite,
> >>> which is the only intended use case of this internal function.
> >>>
> >>> So I don't quite see what would be unexpected.
> >> My misunderstanding comes from where you said:
> >>
> >>
> >>> Not sure if it is worth exiting early per inferior. I couldn't find
> >>> a scenario where you can selectively record (or not record)
> >>> one thread of a multi-threaded inferior.
> >>
> >> By "exiting early", I thought you meant that it would be enough to clear
> >> the data for one thread in each inferior.
> >>
> >>>> I understand that you want to have one copy of the filter per thread.
> >>>>
> >>>> But doing a copy seems like a strange instantiation strategy.  The
> >>>> "global" filter isn't ever used, right?  It only serves to make copies?
> >>>> I think a more common pattern would be for the user to register a
> >>>> factory function (really, ny callable that fits the bill) which would be
> >>>> called whenever we need to create an instance.
> >>>
> >>> Mhm, I think that would make this a bit too complex for users.
> >>> I don't think they want to care about providing a generator as well
> >>> that returns a callable. I don't see any new feature that
> >>> is being unlocked with that, just more tasks for the user.
> >>
> >> I don't think it adds significant complexity, this is pretty typical
> >> Python.  It is similar to what we have for the pretty printers.  You
> >> register a function that gets called to instantiate the right pretty
> >> printer.
> >>
> >> At the simplest, it would be:
> >>
> >>   class MyFilter:
> >>     ...
> >>
> >>   def make_one_filter():
> >>     return MyFilter()
> >>
> >>   register_filter(make_one_filter)
> >>
> >> or even:
> >>
> >>   class MyFilter:
> >>     ...
> >>
> >>   register_filter(lambda: MyFilter())
> >>
> >> An advantage is that it makes it easy to return a different filter per
> >> thread, if needed.  Since filters are on a per-thread basis, I think it
> >> would even make sense to pass the thread to the factory function, in
> >> case the user wants to select a filter based on that.
> >>
> >> Not that the fact that filters are callable themselves is just a design
> >> choice you made, you could very well decide to call something else than
> >> __call__ on those objects.  If you want to make things more explicit,
> >> you can make it so filters have to implement a "get_aux_data" method,
> >> instead of __call__.  The advantage of using __call__ is just that it
> >> allows freely passing functions as well as objects implementing
> >> __call__, so that gives the user a bit more flexibility.
> >>
> >> Simon
> >
> > Sorry, I still don't understand this or see the advantage of the
> > factory pattern here.
> > And I only want to use a pattern if there is a clear advantage to it.
> >
> > I assume you want to move the "housekeeping" to the user side, right?
> 
> Not sure what you mean by "housekeeping".  In the end, with what I
> propose, we still end up with one filter object per thread, it just
> changes how they are instantiated.  Instead of copying a "template"
> object, we call the "get me a new instance" function.
> 
> > Why do we need a factory pattern for that?
> > Can't you achieve the same if we just have one callable filter object
> > instead of one per thread? And have the user do it in that object, if
> > he needs to? (Per thread filters would still allow the same, though not
> > as clean, as they would have to call the singular object.)
> > Passing the thread can also be done with the filter function directly.
> > And is just a convenience, as the GDB API is available to the user.
> > (I am not saying, it is a bad convenience feature.)
> >
> > AFAIK, you use a factory pattern when you want to abstract what
> > type of exact object is returned. I don't see that we need that.
> > And we already have a bit of freedom anyway, with allowing
> > callables.
> >
> > If you don't like the per thread copy, we could still just not do that.
> 
> With what I propose, there would still be one copy (instance) of the
> filter per thread.  So maybe we're not on the same page.
> 
> > And move the per-thread or per-inferior housekeeping to the user.
> > I had this at some point. The documentation I wrote still gives a short
> > example for how to do something for one thread but not for another.
> >
> > As far as I remember, Markus wanted per-thread state in an easier way
> > though.
> 
> Simon

Ah okay, we were indeed not on the same page. Sorry for that.
So it is just about getting rid of the deepcopy mechanism and
the extra copy.

Okay, I will look into this a bit more for the next version.

Thanks a lot!
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