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

  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

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

Commit Message

Comments

Patch