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

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

> -----Original Message-----
> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Freitag, 16. September 2022 14:02
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v6 10/10] btrace: Extend ptwrite event decoding.
> 
> > Date: Fri, 16 Sep 2022 13:36:46 +0200
> > From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> >
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -3,6 +3,12 @@
> >
> >  *** Changes since GDB 12
> >
> > +* GDB now supports printing of ptwrite payloads from the Intel Processor
> > +  Trace during 'record instruction-history', 'record function-call-history',
> > +  all stepping commands and in  Python.  Printing is customizable via a
> 
> Can we rephrase the "and in Python" part to be less confusing and
> ambiguous?  (Also, there's an extraneous blank character there.)

The NEWS file very often says "in Python".
But it is a fair point. How about "and in the Python API?"
I will fix the extra space.


> > +If an inferior uses the instruction, @value{GDBN} by default inserts the
> > +raw payload value as auxiliary information into the execution history.
> > +Auxiliary information is by default printed during
> > +@code{record instruction-history}, @code{record function-call-history},
> > +and all stepping commands and is accessible in Python as a
>                             ^
> Comma missing there.

Thanks, fixed locally.

> Btw, it sounds like this functionality is only available if GDB was
> built with Python?  If so, I think other places where you mention this
> in the manual should indicate that the feature is only available
> through Python.

That is true. Yet most of the documentation is in python.texi for that reason.
Do you think that is not enough?

The other documentation changes (outside of this patch) were for:
1) the btrace config parameter in RSP:
The new RSP parameter will be available regardless of Python.
I don't see a need or a good place to add anything here.
See patch 7.
2) about auxiliary instructions:
These are also python independent. That is by design, to be able
to use the infrastructure for other things in the future.
See patches 1-3.
3) New auxiliary instruction in Python. This is also in python.texi and clear
enough imo. See patch 5.
4) Document record.clear(). This is also only in python.texi and clear enough imo.
See patch 6.

If you think I should mention it in any of these, please let me know.
I don't see where it would make sense.

> > +This function will be called with the ptwrite payload and PC as arguments
> > +during trace decoding.                ^^^^^^^
> 
> @code{ptwrite}.
> 
> Also, please decide whether you want to use "ptwrite" in lower-case or
> PTWRITE in upper-case, and please use the same convention everywhere
> in the manual.

My intention was to use PTWRITE for the instruction. And lower case for
every ptwrite related "thing". E.g. the ptwrite packet, the ptwrite payload,
the ptwrite filter etc.
I will change it to @code{PTWRITE} everywhere.

> I also think talking about a "payload" obfuscates the description for
> no good reason.  It's just a value of some data type supported by the
> 'ptwrite' instruction, right?  If so, I think "value" is much clearer
> and less mysterious.

The Intel Software Developer Manual calls this value the "payload".
Mainly the chapter "Software Trace Instrumentation with PTWRITE".
I would rather have us stick to that name. Talking about just a "value"
or a "PTWRITE value" is more confusing in my eyes, as it isn't specific
and not the official name.

> > +@findex gdb.ptwrite.register_filter
> > +@defun register_filter (@var{filter})
> > +Used to register the ptwrite filter.  The filter can be any callable
>                         ^^^^^^^
> @code{ptwrite}

See my answer above.

> > +@findex gdb.ptwrite.get_filter
> > +@defun get_filter ()
> > +Returns the currently active ptwrite filter function.
> 
> I think our style is to say "Return", not "Returns".

Thanks, will change it.

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