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

[Eli Zaretskii <eliz at gnu dot org>]

> From: Markus Metzger <markus.t.metzger@intel.com>
> Date: Fri, 23 Feb 2018 10:52:50 +0100
> 
> Add new set/show commands to set the processor that is used for enabling errata
> workarounds when decoding branch trace.

Thanks.

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

> +* New options
> +
> +set record btrace cpu
> +show record btrace cpu
> +  Controls the processor to be used for enabling errata workarounds for branch
> +  trace decode.

This part is OK.

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

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

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

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

This is a long example, and it contains several parts.  If we care
about possible division of the example between pages, we should use
@group..@end group around the parts that we want to be on the same
page.

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

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

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