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

[Simon Marchi]

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.


> +
> +	    if (ptw_string.empty ())
> +	      break;
> +
> +	    btinfo->aux_data.emplace_back (ptw_string);

Since you don't need ptw_string below, you can std::move it in the
vector.


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

Simon