From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 28F8F38708AB for ; Fri, 24 Mar 2023 15:23:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28F8F38708AB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.42.115] (modemcable092.73-163-184.mc.videotron.ca [184.163.73.92]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D3C821E0D3; Fri, 24 Mar 2023 11:23:14 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1679671394; bh=2dcupXK+A05OiQ37QZYLaBkuK/MmpKn89jC9vXrhu3Y=; h=Date:Subject:To:References:From:In-Reply-To:From; b=G5EwRa7+FPxfWHlm15Obs5Sj50sRODznXGJG3qVpZ4L2L0LXNF7H6MjcAQ4T4YQTH RAVMa1IYqL/w0TOEvtgS7w8h2DTj0QHSmsngllqyllyf8o6iYIBl7jQLR0aD9lraHx O9c2EzB+Rlg5h9v4eg+mU/uUN+f/D5xS+wPeuE30= Message-ID: Date: Fri, 24 Mar 2023 11:23:14 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration. Content-Language: fr To: Felix Willgerodt , gdb-patches@sourceware.org References: <20230321154626.448816-1-felix.willgerodt@intel.com> <20230321154626.448816-10-felix.willgerodt@intel.com> From: Simon Marchi In-Reply-To: <20230321154626.448816-10-felix.willgerodt@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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 . > + > +"""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