Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.

[Simon Marchi <simark@simark.ca>]

On 3/31/23 06:58, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Freitag, 24. März 2023 16:41
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.
>>
>> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
>>> Call the ptwrite filter function whenever a ptwrite event is decoded.
>>> The returned string is written to the aux_data string table and a
>>> corresponding auxiliary instruction is appended to the function segment.
>>> ---
>>>  gdb/NEWS                                  |   7 +
>>>  gdb/btrace.c                              |  54 +++
>>>  gdb/config.in                             |   3 +
>>>  gdb/configure                             |  11 +
>>>  gdb/doc/python.texi                       | 150 ++++++
>>>  gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 550
>> ++++++++++++++++++++++
>>>  gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
>>>  gdb/testsuite/gdb.btrace/ptwrite.exp      | 200 ++++++++
>>>  gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 544
>> +++++++++++++++++++++
>>>  gdb/testsuite/lib/gdb.exp                 |  72 +++
>>>  gdbsupport/common.m4                      |   2 +
>>>  gdbsupport/config.in                      |   3 +
>>>  gdbsupport/configure                      |  11 +
>>>  13 files changed, 1646 insertions(+)
>>>  create mode 100644 gdb/testsuite/gdb.btrace/i386-ptwrite.S
>>>  create mode 100644 gdb/testsuite/gdb.btrace/ptwrite.c
>>>  create mode 100644 gdb/testsuite/gdb.btrace/ptwrite.exp
>>>  create mode 100644 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
>>>
>>> diff --git a/gdb/NEWS b/gdb/NEWS
>>> index cc262f1f8a6..5dd05867f2a 100644
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -106,6 +106,13 @@ show always-read-ctf
>>>
>>>  *** Changes in GDB 13
>>>
>>> +* GDB now supports printing of ptwrite payloads from the Intel Processor
>>> +  Trace during 'record instruction-history', 'record function-call-history'
>>> +  and all stepping commands.  The payload is also accessible in Python as a
>>> +  RecordAuxiliary object.  Printing is customizable via a ptwrite filter
>>> +  function in Python.  By default, the raw ptwrite payload is printed for
>>> +  each ptwrite that is encountered.
>>> +
>>>  * MI version 1 is deprecated, and will be removed in GDB 14.
>>>
>>>  * GDB now supports dumping memory tag data for AArch64 MTE.  It also
>> supports
>>> diff --git a/gdb/btrace.c b/gdb/btrace.c
>>> index 37dd0b666d8..db0d0e291d9 100644
>>> --- a/gdb/btrace.c
>>> +++ b/gdb/btrace.c
>>> @@ -1253,6 +1253,54 @@ handle_pt_insn_events (struct
>> btrace_thread_info *btinfo,
>>>  		   bfun->insn_offset - 1, offset);
>>>
>>>  	  break;
>>> +#if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
>>> +	case ptev_ptwrite:
>>> +	  {
>>> +	    uint64_t ip = 0;
>>> +	    std::string ptw_string;
>>> +	    btrace_insn_flags flags = 0;
>>> +
>>> +	    /* Lookup the ip if available.  */
>>> +	    if (event.ip_suppressed == 0)
>>> +	      ip = event.variant.ptwrite.ip;
>>> +
>>> +	    if (btinfo->ptw_callback_fun != nullptr)
>>> +	      ptw_string
>>> +		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
>>> +					    ip, btinfo->ptw_context);
>>
>> If ptw_callback_fun is nullptr, the string will always be empty, and the
>> data not shown?  This means that in a build without Python, the data
>> will never be shown?
>>
>> I think a better approach would be to handle the default case (print as
>> hex) here, instead of in a Python function.  I think that
>> ptw_callback_fun should return a gdb::optional<std::string>.  If the
>> optional is instantiated (including containing an empty string), use
>> that value.  If ptw_callback_fun is nullptr (perhaps because GDB was
>> built with no Python support), or if the returned optional is not
>> instantiated (perhaps because the user did not register any filter),
>> generate the default (print as hex) here.
>>
> 
> We wanted the user to be able to register None as a listener. But I think
> that might still be from a time where we didn't implement
> the /a modifiers for the instruction/function-history CLI commands to
> enable not printing the strings.

Passing None could be used to clear the registered filter, to go back to
the default state.

> I think your suggestion makes sense. I guess in that case we should try
> to get rid of the current default filter in Python. To not have two places
> in the code that do the same thing.

Agreed.

>>> +@end defun
>>> +
>>> +@findex gdb.ptwrite.get_filter
>>> +@defun get_filter ()
>>> +Return the currently active @code{PTWRITE} filter function.
>>> +@end defun
>>> +
>>> +@findex gdb.ptwrite.default_filter
>>> +@defun default_filter (@var{payload}, @var{ip})
>>> +The filter function active by default.  It prints the payload in hexadecimal
>>> +format.
>>> +@end defun
>>> +
>>> +@value{GDBN} creates a new copy of the filter function for each thread
>> to
>>> +allow for independent internal states.  There is no support for registering
>>
>> Ok, I guess that answers my question about deepcopy.  I think it would
>> be good to say that the copy is done using the deepcopy module/function.
>>
> 
> I will add that.

Note that with my suggestion of using a factory function instead, that
suggestion would be moot.

Simon