diff mbox

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

Message ID A78C989F6D9628469189715575E55B236964BF9A@IRSMSX104.ger.corp.intel.com
State New
Headers show

Commit Message

Metzger, Markus T Feb. 26, 2018, 3:45 p.m. UTC
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.

---


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

Comments

Eli Zaretskii Feb. 26, 2018, 7:13 p.m. UTC | #1
> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Mon, 26 Feb 2018 15:45:36 +0000
> > > +@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".

It's better, but still not clear enough.  What kind of "errata" are we
talking about?  The kind described in
https://community.amd.com/thread/186609, for example?  And what do the
workarounds do?

If you can explain that to me or give an example, I will try to
propose some text to describe that in the manual.

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

Not necessarily.  But I'd prefer us to consistently use one of the
forms.

The rest of the patch LGTM, thanks.
Metzger, Markus T Feb. 27, 2018, 11:41 a.m. UTC | #2
Hello Eli,

> > > 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".
> 
> It's better, but still not clear enough.  What kind of "errata" are we talking about?
> The kind described in https://community.amd.com/thread/186609, for example?
> And what do the workarounds do?
> 
> If you can explain that to me or give an example, I will try to propose some text to
> describe that in the manual.

Processor errata are bugs that, in our case, may cause the trace to not match the spec.
This typically causes unaware decoders to fail with some error.

An erratum workaround will try to detect an erroneous trace packet sequence and
correct it.

In our case, each workaround needs to be enabled separately.  The decoder determines
the workarounds to be enabled based on the processor on which the trace was recorded.


> > > > +  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?
> 
> Not necessarily.  But I'd prefer us to consistently use one of the forms.

I removed the optional space here and in the commit-message.

Regards,
Markus.
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
Eli Zaretskii Feb. 27, 2018, 6:23 p.m. UTC | #3
> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Tue, 27 Feb 2018 11:41:43 +0000
> > > I rephrased this to "... for enabling workarounds for processor errata
> > > when decoding the trace".
> > 
> > It's better, but still not clear enough.  What kind of "errata" are we talking about?
> > The kind described in https://community.amd.com/thread/186609, for example?
> > And what do the workarounds do?
> > 
> > If you can explain that to me or give an example, I will try to propose some text to
> > describe that in the manual.
> 
> Processor errata are bugs that, in our case, may cause the trace to not match the spec.
> This typically causes unaware decoders to fail with some error.
> 
> An erratum workaround will try to detect an erroneous trace packet sequence and
> correct it.
> 
> In our case, each workaround needs to be enabled separately.  The decoder determines
> the workarounds to be enabled based on the processor on which the trace was recorded.

Thanks.  Then I suggest to have this text in the manual:

  @item set record btrace cpu @var{identifier}
  Set the processor to be used for enabling workarounds for processor
  errata when decoding the trace.

  @cindex processor errata
  @dfn{Processor errata} are bugs in processor firmware that can cause
  a trace not to match the specification.  Trace decoders that are
  unaware of these errata might fail to decode such a trace.
  @value{GDBN} can detect erroneous trace packets and correct them,
  thus avoiding the decoding failures.  These corrections are known as
  @dfn{errata workarounds}, and are enabled based on the processor on
  which the trace was recorded.

  By default, @value{GDBN} attempts to detect the processor
  automatically, and apply the necessary workarounds for it.  However,
  you may need to specify the processor if @value{GDBN} does not yet
  support it.  This command allows you to do that, and also allows to
  disable the workarounds.

  The argument @var{identifier} identifies the @sc{cpu} and is of the
  form:
  [...]

How does that sound?
diff mbox

Patch

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