[PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
Metzger, Markus T
markus.t.metzger@intel.com
Tue Jun 28 13:59:20 GMT 2022
Hello Felix,
>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 | 3 +
> gdb/btrace.h | 9 +++
> 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 | 86 +++++++++++++++++++++++++
> gdb/python/py-record-btrace.c | 111 +++++++++++++++++++++++++++++++++
> gdb/python/py-record-btrace.h | 8 +++
> gdb/python/python-internal.h | 3 +
> gdb/python/python.c | 2 +
> 12 files changed, 245 insertions(+)
> create mode 100644 gdb/python/lib/gdb/ptwrite.py
> /* For maintenance commands. */
>@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
> uint64_t offset;
> int status;
>
>+ apply_ext_lang_ptwrite_filter (btinfo);
A comment would be nice, here, to explain what this does.
>+ /* Function pointer to the ptwrite callback. Returns the string returned
>+ by the ptwrite filter function or nullptr if no string is supposed to
>+ be printed. */
>+ std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
>+ const void *ptw_filter);
The comment doesn't match the code. It cannot return nullptr.
Why to we call the void * parameter ptw_filter instead of the usual context?
We probably want to call the callback itself ptw_filter and the void * argument
context.
We also seem to mix the terms ptwrite callback and ptwrite filter.
>+
>+ /* Function pointer to the ptwrite filter function. */
>+ void *ptw_filter = nullptr;
Is this the function or some context for the function?
>diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
>index 14b191ded62..86f92a476af 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_listener, */
This should probably be called ptwrite_filter.
>+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}
Why those leading underscores?
>+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?
That leaves removing obsolete filters. Could this be done with some
thread notification?
>+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):
Let's remove the empty line to group the statements.
>+/* Helper function that calls the ptwrite filter PTW_FILTER with
>+ PAYLOAD and IP as arguments. Returns a pointer to the string that will
>+ be printed or nullptr if nothing should be printed. IP can be nullptr,
>+ PAYLOAD must point to a valid integer. */
>+std::string
>+recpy_call_filter (const uint64_t payload, const uint64_t ip,
>+ const void *ptw_filter)
The comment doesn't match the code. It cannot return nullptr. Also,
IP and PAYLOAD are integers, not pointers.
I think we can shorten "calls the ptwrite filter PTW_FILTER" to just
"calls PTW_FILTER".
>+{
>+ std::string result;
>+
>+ if ((PyObject *) ptw_filter == Py_None)
>+ return result;
>+ else if ((PyObject *) ptw_filter == nullptr)
>+ error (_("No valid ptwrite filter."));
No need for else. Should we check nullptr first?
>+/* Helper function returning the current ptwrite filter. Returns nullptr
>+ in case of errors. */
>+
>+PyObject *
>+get_ptwrite_filter ()
>+{
>+ PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
>+
>+ if (PyErr_Occurred ())
>+ {
>+ gdbpy_print_stack ();
>+ return nullptr;
>+ }
Do we want to print the stack without throwing an error?
>+/* Used for registering the default ptwrite filter to the current thread. A
What does 'default' mean, here? We're registering the callback that btrace
calls on PTW. From btrace's perspective, this is the ptwrite filter. From
python's perspective, this is maybe a proxy for the python filter.
>+ 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)
>+{
>+ if (!gdb_python_initialized || btinfo == nullptr)
>+ return;
Isn't this an error or even an internal error case?
>+/* Callback function for the ptwrite filter. */
>+extern std::string recpy_call_filter (const uint64_t payload,
>+ const uint64_t ip,
>+ const void *ptw_filter);
Should the comment say something like 'proxy for the python
ptwrite filter'?
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
More information about the Gdb-patches
mailing list