RE: [PATCH 2/2] btrace: set/show record btrace cpu

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

Hello Eli,

Thanks for your review.

> > The general format is "<vendor>: <identifier>" but we also allow two
> > special values "auto" and "none".
> >
> > The default is "auto", which is the current behaviour of having GDB
> > determine the processor on which the trace was recorded.
> >
> > If that cpu is not known to the trace decoder, e.g. when using an old
> > decoder on a new system, decode may fail with "unknown cpu".  In most
> > cases it should suffice to 'downgrade' decode to assume an older cpu.
> > Unfortunately, we can't do this automatically.
> 
> This is useful information that is entirely missing from the patch to the manual.
> As a general rule, if you find something you need to say in the patch log message
> in order to describe it to this list, it's almost certain the same text should be in the
> manual, as the manual will be read by folks who are not unlike the readers of this
> list.

This is somewhat implicit in the example...

> > +@smallexample
> > +(gdb) info record
> > +Active record target: record-btrace
> > +Recording format: Intel Processor Trace.
> > +Buffer size: 16kB.
> > +Failed to configure the Intel Processor Trace decoder: unknown cpu.
> > +(gdb) set record btrace cpu intel:6/158
> > +(gdb) info record
> > +Active record target: record-btrace
> > +Recording format: Intel Processor Trace.
> > +Buffer size: 16kB.
> > +Recorded 84872 instructions in 3189 functions (0 gaps) for thread 1 (...).
> > +@end smallexample

I will spell it out more clearly as introduction to the example.

 
> > +@item set record btrace cpu @var{identifier} Set the processor to be
> > +used for enabling trace decode errata workarounds.
> 
> I think we need to say something about just what those "errata workarounds"
> are, and what are they used for.

I rephrased this to "... for enabling workarounds for processor errata when
decoding the trace".


> >                The general @var{identifier} format is a vendor
> > +identifier followed by a vendor-specific processor identifier.
> 
> This fails to mention the colon delimiter, and in general it is better to just show
> the form, rather than describe it.  Like this:
> 
>   The argument @var{identifier} identifies the @sc{cpu}, and is of the
>   form
> @code{@var{vendor}:@var{family}/@var{model}@r{[}/@var{stepping}@r{]}.

The general form is @code{@var{vendor}:@var{processor identifier}}, where the
format of @var{processor identifier} depends on @{vendor} and is defined in the
table below.  Will update as you suggested.


> > +The following vendor identifiers and corresponding processor
> > +identifiers are currently supported:
> > +
> > +@multitable @columnfractions .1 .9
> > +
> > +@item @code{intel}
> > +@tab @var{family}/@var{model}[/@var{stepping}]
> 
> I think we need to tell a bit more about @var{family} and @var{model} here,
> and/or maybe tell the readers how to obtain that information.

I added:

On GNU/Linux systems, the processor @var{family}, @var{model}, and
@var{stepping} can be obtained from @code{/proc/cpuinfo}.

Should this be a @footnote{}?


> > +If @var{identifier} is @code{auto}, enable errata workarounds for the
> > +processor on which the trace was recorded.  If @var{identifier} is
> > +@code{none}, errata workarounds are disabled.
> 
> This should mention what you described in the log message above, and also tell
> what does "disabled" mean in practice (or maybe this will become clear when you
> explain what are the errata workarounds about).

I added:

For example, when using an old @value{GDBN} on a new system, decode
may fail because @value{GDBN} does not support the new processor.  It
often suffices to specify an older processor that @value{GDBN}
supports.

 
> > +  add_prefix_cmd ("cpu", class_support, cmd_set_record_btrace_cpu,
> > +		  _("\
> > +Set the cpu to be used for trace decode.\n\n\ The format is
> > +\"<vendor>: <identifier>\" or \"none\" or \"auto\" (default).
>                            ^^
> So should there be a blank after the colon, or shouldn't there be?
> The example in the manual says no blank.

White space is ignored.  Do we write this explicitly?


> > +The default is AUTO, which uses the cpu on which the trace was
> > +recorded.\n\
>                   ^^^^
> Above you used "auto", quoted and in lower case.

Changed it to \"auto\" and \"none\".


> > +When GDB does not support that cpu, this option can be used to
> > +enable\n\ workarounds for a similar cpu that GDB supports.\n\n\
> 
> This trick is not in the manual; it should be, IMO.

See above.

Below is the revised diff for the documentation.

Thanks,
Markus.

---
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ee7adc8..01a2e58 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6952,10 +6952,59 @@ and to read-write memory.  Beware that the accessed memory corresponds
 to the live target and not necessarily to the current replay
 position.
 
+@item set record btrace cpu @var{identifier}
+Set the processor to be used for enabling workarounds for processor
+errata when decoding the trace.  The argument @var{identifier}
+identifies the @sc{cpu} and is of the form:
+@code{@var{vendor}:@var{procesor identifier}}.
+
+In addition, there are two special identifiers, @code{none} and
+@code{auto} (default).
+
+The following vendor identifiers and corresponding processor
+identifiers are currently supported:
+
+@multitable @columnfractions .1 .9
+
+@item @code{intel}
+@tab @var{family}/@var{model}[/@var{stepping}]
+
+@end multitable
+
+On GNU/Linux systems, the processor @var{family}, @var{model}, and
+@var{stepping} can be obtained from @code{/proc/cpuinfo}.
+
+If @var{identifier} is @code{auto}, enable errata workarounds for the
+processor on which the trace was recorded.  If @var{identifier} is
+@code{none}, errata workarounds are disabled.
+
+For example, when using an old @value{GDBN} on a new system, decode
+may fail because @value{GDBN} does not support the new processor.  It
+often suffices to specify an older processor that @value{GDBN}
+supports.
+
+@smallexample
+(gdb) info record
+Active record target: record-btrace
+Recording format: Intel Processor Trace.
+Buffer size: 16kB.
+Failed to configure the Intel Processor Trace decoder: unknown cpu.
+(gdb) set record btrace cpu intel:6/158
+(gdb) info record
+Active record target: record-btrace
+Recording format: Intel Processor Trace.
+Buffer size: 16kB.
+Recorded 84872 instructions in 3189 functions (0 gaps) for thread 1 (...).
+@end smallexample
+
 @kindex show record btrace
 @item show record btrace replay-memory-access
 Show the current setting of @code{replay-memory-access}.
 
+@item show record btrace cpu
+Show the processor to be used for enabling trace decode errata
+workarounds.
+
 @kindex set record btrace bts
 @item set record btrace bts buffer-size @var{size}
 @itemx set record btrace bts buffer-size unlimited


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928