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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

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

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.

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

Right, I will do that.

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

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