[PATCH 1/2] btrace: fix output of "set record btrace"

[PATCH 1/2] btrace: fix output of "set record btrace"

[Markus Metzger]

Instead of giving a message that "set record btrace" needs a sub-command, GDB
crashed.  Fix it.  A regression test comes with the next patch.

2018-02-23  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* record-btrace.c (cmd_set_record_btrace): Print sub-commands.


---
 gdb/record-btrace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 2464672..bc4566c 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2955,7 +2955,10 @@ cmd_record_btrace_start (const char *args, int from_tty)
 static void
 cmd_set_record_btrace (const char *args, int from_tty)
 {
-  cmd_show_list (set_record_btrace_cmdlist, from_tty, "");
+  printf_unfiltered (_("\"set record btrace\" must be followed "
+		       "by an apporpriate subcommand.\n"));
+  help_list (set_record_btrace_cmdlist, "set record btrace ", all_commands,
+	     gdb_stdout);
 }
 
 /* The "show record btrace" command.  */
-- 
1.8.3.1

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

[Markus Metzger]

Add new set/show commands to set the processor that is used for enabling errata
workarounds when decoding branch trace.

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.

The other special value, "none", disables errata workarounds.

2018-02-23  Markus Metzger  <markus.t.metzger@intel.com>

gdb/
	* NEWS (New options): announce set/show record btrace cpu.
	* btrace.c: Include record-btrace.h.
	(btrace_compute_ftrace_pt): Skip enabling errata workarounds if the
	vendor is unknown.
	(btrace_compute_ftrace_1): Add cpu parameter.  Update callers.
	Maybe overwrite the btrace configuration's cpu.
	(btrace_compute_ftrace): Add cpu parameter.  Update callers.
	(btrace_fetch): Add cpu parameter.  Update callers.
	(btrace_maint_update_pt_packets): Call record_btrace_get_cpu.  Maybe
	overwrite the btrace configuration's cpu.  Skip enabling errata
	workarounds if the vendor is unknown.
	* python/py-record-btrace.c: Include record-btrace.h.
	(recpy_bt_begin, recpy_bt_end, recpy_bt_instruction_history)
	(recpy_bt_function_call_history): Call record_btrace_get_cpu.
	* record-btrace.c (record_btrace_cpu_state_kind): New.
	(record_btrace_cpu): New.
	(set_record_btrace_cpu_cmdlist): New.
	(record_btrace_get_cpu): New.
	(require_btrace_thread, record_btrace_info)
	(record_btrace_resume_thread): Call record_btrace_get_cpu.
	(cmd_set_record_btrace_cpu_none): New.
	(cmd_set_record_btrace_cpu_auto): New.
	(cmd_set_record_btrace_cpu): New.
	(cmd_show_record_btrace_cpu): New.
	(_initialize_record_btrace): Initialize set/show record btrace cpu
	commands.
	* record-btrace.h (record_btrace_get_cpu): New.

testsuite/
	* gdb.btrace/cpu.exp: New.

doc/
	* gdb.texinfo: Document set/show record btrace cpu.
---
 gdb/NEWS                         |   7 ++
 gdb/btrace.c                     |  69 ++++++++++-----
 gdb/btrace.h                     |   5 +-
 gdb/doc/gdb.texinfo              |  41 +++++++++
 gdb/python/py-record-btrace.c    |   9 +-
 gdb/record-btrace.c              | 176 ++++++++++++++++++++++++++++++++++++++-
 gdb/record-btrace.h              |   4 +
 gdb/testsuite/gdb.btrace/cpu.exp |  74 ++++++++++++++++
 8 files changed, 352 insertions(+), 33 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/cpu.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 1767cef..548386a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -6,6 +6,13 @@
 * 'info proc' now works on running processes on FreeBSD systems and core
   files created on FreeBSD systems.
 
+* New options
+
+set record btrace cpu
+show record btrace cpu
+  Controls the processor to be used for enabling errata workarounds for branch
+  trace decode.
+
 *** Changes in GDB 8.1
 
 * GDB now supports dynamically creating arbitrary register groups specified
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 158d03c..7940bc8 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -35,6 +35,9 @@
 #include "gdbcmd.h"
 #include "cli/cli-utils.h"
 
+/* For maintenance commands.  */
+#include "record-btrace.h"
+
 #include <inttypes.h>
 #include <ctype.h>
 #include <algorithm>
@@ -1428,15 +1431,19 @@ btrace_compute_ftrace_pt (struct thread_info *tp,
   config.begin = btrace->data;
   config.end = btrace->data + btrace->size;
 
-  config.cpu.vendor = pt_translate_cpu_vendor (btrace->config.cpu.vendor);
-  config.cpu.family = btrace->config.cpu.family;
-  config.cpu.model = btrace->config.cpu.model;
-  config.cpu.stepping = btrace->config.cpu.stepping;
+  /* We treat an unknown vendor as 'no errata'.  */
+  if (btrace->config.cpu.vendor != CV_UNKNOWN)
+    {
+      config.cpu.vendor = pt_translate_cpu_vendor (btrace->config.cpu.vendor);
+      config.cpu.family = btrace->config.cpu.family;
+      config.cpu.model = btrace->config.cpu.model;
+      config.cpu.stepping = btrace->config.cpu.stepping;
 
-  errcode = pt_cpu_errata (&config.errata, &config.cpu);
-  if (errcode < 0)
-    error (_("Failed to configure the Intel Processor Trace decoder: %s."),
-	   pt_errstr (pt_errcode (errcode)));
+      errcode = pt_cpu_errata (&config.errata, &config.cpu);
+      if (errcode < 0)
+	error (_("Failed to configure the Intel Processor Trace decoder: %s."),
+	       pt_errstr (pt_errcode (errcode)));
+    }
 
   decoder = pt_insn_alloc_decoder (&config);
   if (decoder == NULL)
@@ -1484,11 +1491,13 @@ btrace_compute_ftrace_pt (struct thread_info *tp,
 
 #endif /* defined (HAVE_LIBIPT)  */
 
-/* Compute the function branch trace from a block branch trace BTRACE for
-   a thread given by BTINFO.  */
+/* Compute the function branch trace from a block branch trace BTRACE for a
+   thread given by BTINFO.  If CPU is not NULL, overwrite the cpu in the branch
+   trace configuration.  This is currently only used for the PT format.  */
 
 static void
 btrace_compute_ftrace_1 (struct thread_info *tp, struct btrace_data *btrace,
+			 const struct btrace_cpu *cpu,
 			 std::vector<unsigned int> &gaps)
 {
   DEBUG ("compute ftrace");
@@ -1503,6 +1512,10 @@ btrace_compute_ftrace_1 (struct thread_info *tp, struct btrace_data *btrace,
       return;
 
     case BTRACE_FORMAT_PT:
+      /* Overwrite the cpu we use for enabling errata workarounds.  */
+      if (cpu != nullptr)
+	btrace->variant.pt.config.cpu = *cpu;
+
       btrace_compute_ftrace_pt (tp, &btrace->variant.pt, gaps);
       return;
     }
@@ -1521,13 +1534,14 @@ btrace_finalize_ftrace (struct thread_info *tp, std::vector<unsigned int> &gaps)
 }
 
 static void
-btrace_compute_ftrace (struct thread_info *tp, struct btrace_data *btrace)
+btrace_compute_ftrace (struct thread_info *tp, struct btrace_data *btrace,
+		       const struct btrace_cpu *cpu)
 {
   std::vector<unsigned int> gaps;
 
   TRY
     {
-      btrace_compute_ftrace_1 (tp, btrace, gaps);
+      btrace_compute_ftrace_1 (tp, btrace, cpu, gaps);
     }
   CATCH (error, RETURN_MASK_ALL)
     {
@@ -1564,7 +1578,7 @@ btrace_add_pc (struct thread_info *tp)
   block->begin = pc;
   block->end = pc;
 
-  btrace_compute_ftrace (tp, &btrace);
+  btrace_compute_ftrace (tp, &btrace, NULL);
 
   do_cleanups (cleanup);
 }
@@ -1872,7 +1886,7 @@ btrace_decode_error (enum btrace_format format, int errcode)
 /* See btrace.h.  */
 
 void
-btrace_fetch (struct thread_info *tp)
+btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
 {
   struct btrace_thread_info *btinfo;
   struct btrace_target_info *tinfo;
@@ -1948,7 +1962,7 @@ btrace_fetch (struct thread_info *tp)
       btrace_maint_clear (btinfo);
 
       btrace_clear_history (btinfo);
-      btrace_compute_ftrace (tp, &btrace);
+      btrace_compute_ftrace (tp, &btrace, cpu);
     }
 
   do_cleanups (cleanup);
@@ -3028,6 +3042,7 @@ static void
 btrace_maint_update_pt_packets (struct btrace_thread_info *btinfo)
 {
   struct pt_packet_decoder *decoder;
+  const struct btrace_cpu *cpu;
   struct btrace_data_pt *pt;
   struct pt_config config;
   int errcode;
@@ -3044,15 +3059,23 @@ btrace_maint_update_pt_packets (struct btrace_thread_info *btinfo)
   config.begin = pt->data;
   config.end = pt->data + pt->size;
 
-  config.cpu.vendor = pt_translate_cpu_vendor (pt->config.cpu.vendor);
-  config.cpu.family = pt->config.cpu.family;
-  config.cpu.model = pt->config.cpu.model;
-  config.cpu.stepping = pt->config.cpu.stepping;
+  cpu = record_btrace_get_cpu ();
+  if (cpu == nullptr)
+    cpu = &pt->config.cpu;
 
-  errcode = pt_cpu_errata (&config.errata, &config.cpu);
-  if (errcode < 0)
-    error (_("Failed to configure the Intel Processor Trace decoder: %s."),
-	   pt_errstr (pt_errcode (errcode)));
+  /* We treat an unknown vendor as 'no errata'.  */
+  if (cpu->vendor != CV_UNKNOWN)
+    {
+      config.cpu.vendor = pt_translate_cpu_vendor (cpu->vendor);
+      config.cpu.family = cpu->family;
+      config.cpu.model = cpu->model;
+      config.cpu.stepping = cpu->stepping;
+
+      errcode = pt_cpu_errata (&config.errata, &config.cpu);
+      if (errcode < 0)
+	error (_("Failed to configure the Intel Processor Trace decoder: %s."),
+	       pt_errstr (pt_errcode (errcode)));
+    }
 
   decoder = pt_pkt_alloc_decoder (&config);
   if (decoder == NULL)
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 5c3f21b..e05ab3c 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -385,8 +385,9 @@ extern void btrace_teardown (struct thread_info *);
 
 extern const char *btrace_decode_error (enum btrace_format format, int errcode);
 
-/* Fetch the branch trace for a single thread.  */
-extern void btrace_fetch (struct thread_info *);
+/* Fetch the branch trace for a single thread.  If CPU is not NULL, assume CPU
+   for trace decode.  */
+extern void btrace_fetch (struct thread_info *, const struct btrace_cpu *cpu);
 
 /* Clear the branch trace for a single thread.  */
 extern void btrace_clear (struct thread_info *);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ee7adc8..8480fde 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6952,10 +6952,51 @@ 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 trace decode errata
+workarounds.  The general @var{identifier} format is a vendor
+identifier followed by a vendor-specific processor 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
+
+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:
+
+@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
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 35828a6..d78df7f 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -24,6 +24,7 @@
 #include "btrace.h"
 #include "py-record.h"
 #include "py-record-btrace.h"
+#include "record-btrace.h"
 #include "disasm.h"
 
 #if defined (IS_PY3K)
@@ -678,7 +679,7 @@ recpy_bt_begin (PyObject *self, void *closure)
   if (tinfo == NULL)
     Py_RETURN_NONE;
 
-  btrace_fetch (tinfo);
+  btrace_fetch (tinfo, record_btrace_get_cpu ());
 
   if (btrace_is_empty (tinfo))
     Py_RETURN_NONE;
@@ -700,7 +701,7 @@ recpy_bt_end (PyObject *self, void *closure)
   if (tinfo == NULL)
     Py_RETURN_NONE;
 
-  btrace_fetch (tinfo);
+  btrace_fetch (tinfo, record_btrace_get_cpu ());
 
   if (btrace_is_empty (tinfo))
     Py_RETURN_NONE;
@@ -724,7 +725,7 @@ recpy_bt_instruction_history (PyObject *self, void *closure)
    if (tinfo == NULL)
      Py_RETURN_NONE;
 
-   btrace_fetch (tinfo);
+   btrace_fetch (tinfo, record_btrace_get_cpu ());
 
    if (btrace_is_empty (tinfo))
      Py_RETURN_NONE;
@@ -753,7 +754,7 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
   if (tinfo == NULL)
     Py_RETURN_NONE;
 
-  btrace_fetch (tinfo);
+  btrace_fetch (tinfo, record_btrace_get_cpu ());
 
   if (btrace_is_empty (tinfo))
     Py_RETURN_NONE;
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index bc4566c..d4a2fc3 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -60,6 +60,20 @@ static const char *const replay_memory_access_types[] =
 /* The currently allowed replay memory access type.  */
 static const char *replay_memory_access = replay_memory_access_read_only;
 
+/* The cpu state kinds.  */
+enum record_btrace_cpu_state_kind
+{
+  CS_AUTO,
+  CS_NONE,
+  CS_CPU
+};
+
+/* The current cpu state.  */
+static enum record_btrace_cpu_state_kind record_btrace_cpu_state = CS_AUTO;
+
+/* The current cpu for trace decode.  */
+static struct btrace_cpu record_btrace_cpu;
+
 /* Command lists for "set/show record btrace".  */
 static struct cmd_list_element *set_record_btrace_cmdlist;
 static struct cmd_list_element *show_record_btrace_cmdlist;
@@ -87,6 +101,9 @@ static struct cmd_list_element *show_record_btrace_bts_cmdlist;
 static struct cmd_list_element *set_record_btrace_pt_cmdlist;
 static struct cmd_list_element *show_record_btrace_pt_cmdlist;
 
+/* Command list for "set record btrace cpu".  */
+static struct cmd_list_element *set_record_btrace_cpu_cmdlist;
+
 /* Print a record-btrace debug message.  Use do ... while (0) to avoid
    ambiguities when used in if statements.  */
 
@@ -100,6 +117,26 @@ static struct cmd_list_element *show_record_btrace_pt_cmdlist;
   while (0)
 
 
+/* Return the cpu configured by the user.  Returns NULL if the cpu was
+   configured as auto.  */
+const struct btrace_cpu *
+record_btrace_get_cpu (void)
+{
+  switch (record_btrace_cpu_state)
+    {
+    case CS_AUTO:
+      return nullptr;
+
+    case CS_NONE:
+      record_btrace_cpu.vendor = CV_UNKNOWN;
+      /* Fall through.  */
+    case CS_CPU:
+      return &record_btrace_cpu;
+    }
+
+  error (_("Internal error: bad record btrace cpu state."));
+}
+
 /* Update the branch trace for the current thread and return a pointer to its
    thread_info.
 
@@ -119,7 +156,7 @@ require_btrace_thread (void)
 
   validate_registers_access ();
 
-  btrace_fetch (tp);
+  btrace_fetch (tp, record_btrace_get_cpu ());
 
   if (btrace_is_empty (tp))
     error (_("No trace."));
@@ -427,7 +464,7 @@ record_btrace_info (struct target_ops *self)
   if (conf != NULL)
     record_btrace_print_conf (conf);
 
-  btrace_fetch (tp);
+  btrace_fetch (tp, record_btrace_get_cpu ());
 
   insns = 0;
   calls = 0;
@@ -1843,7 +1880,7 @@ record_btrace_resume_thread (struct thread_info *tp,
   btinfo = &tp->btrace;
 
   /* Fetch the latest branch trace.  */
-  btrace_fetch (tp);
+  btrace_fetch (tp, record_btrace_get_cpu ());
 
   /* A resume request overwrites a preceding resume or stop request.  */
   btinfo->flags &= ~(BTHR_MOVE | BTHR_STOP);
@@ -2979,7 +3016,113 @@ cmd_show_replay_memory_access (struct ui_file *file, int from_tty,
 		    replay_memory_access);
 }
 
-/* The "set record btrace bts" command.  */
+/* The "set record btrace cpu none" command.  */
+
+static void
+cmd_set_record_btrace_cpu_none (const char *args, int from_tty)
+{
+  if (args != nullptr && *args != 0)
+    error (_("Trailing junk: '%s'."), args);
+
+  record_btrace_cpu_state = CS_NONE;
+}
+
+/* The "set record btrace cpu auto" command.  */
+
+static void
+cmd_set_record_btrace_cpu_auto (const char *args, int from_tty)
+{
+  if (args != nullptr && *args != 0)
+    error (_("Trailing junk: '%s'."), args);
+
+  record_btrace_cpu_state = CS_AUTO;
+}
+
+/* The "set record btrace cpu" command.  */
+
+static void
+cmd_set_record_btrace_cpu (const char *args, int from_tty)
+{
+  if (args == nullptr)
+    args = "";
+
+  /* We use a hard-coded vendor string for now.  */
+  unsigned int family, model, stepping;
+  int l1, l2, matches = sscanf (args, "intel: %u/%u%n/%u%n", &family, &model,
+				&l1, &stepping, &l2);
+  if (matches == 3)
+    {
+      if (strlen (args) != l2)
+	error (_("Trailing junk: '%s'."), args + l2);
+    }
+  else if (matches == 2)
+    {
+      if (strlen (args) != l1)
+	error (_("Trailing junk: '%s'."), args + l1);
+
+      stepping = 0;
+    }
+  else
+    error (_("Bad format.  See \"help set record btrace cpu\"."));
+
+  if (USHRT_MAX < family)
+    error (_("Cpu family too big."));
+
+  if (UCHAR_MAX < model)
+    error (_("Cpu model too big."));
+
+  if (UCHAR_MAX < stepping)
+    error (_("Cpu stepping too big."));
+
+  record_btrace_cpu.vendor = CV_INTEL;
+  record_btrace_cpu.family = family;
+  record_btrace_cpu.model = model;
+  record_btrace_cpu.stepping = stepping;
+
+  record_btrace_cpu_state = CS_CPU;
+}
+
+/* The "show record btrace cpu" command.  */
+
+static void
+cmd_show_record_btrace_cpu (const char *args, int from_tty)
+{
+  const char *cpu;
+
+  if (args != nullptr && *args != 0)
+    error (_("Trailing junk: '%s'."), args);
+
+  switch (record_btrace_cpu_state)
+    {
+    case CS_AUTO:
+      printf_unfiltered (_("btrace cpu is 'auto'.\n"));
+      return;
+
+    case CS_NONE:
+      printf_unfiltered (_("btrace cpu is 'none'.\n"));
+      return;
+
+    case CS_CPU:
+      switch (record_btrace_cpu.vendor)
+	{
+	case CV_INTEL:
+	  if (record_btrace_cpu.stepping == 0)
+	    printf_unfiltered (_("btrace cpu is 'intel: %u/%u'.\n"),
+			       record_btrace_cpu.family,
+			       record_btrace_cpu.model);
+	  else
+	    printf_unfiltered (_("btrace cpu is 'intel: %u/%u/%u'.\n"),
+			       record_btrace_cpu.family,
+			       record_btrace_cpu.model,
+			       record_btrace_cpu.stepping);
+	  return;
+	}
+    }
+
+  error (_("Internal error: bad cpu state."));
+}
+
+/* The "s record btrace bts" command.  */
 
 static void
 cmd_set_record_btrace_bts (const char *args, int from_tty)
@@ -3087,6 +3230,31 @@ replay."),
 			   &set_record_btrace_cmdlist,
 			   &show_record_btrace_cmdlist);
 
+  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).\n\
+For vendor \"intel\" the format is \"<family>/<model>[/<stepping>]\".\n\n\
+When decoding branch trace, enable errata workarounds for the specified cpu.\n\
+The default is AUTO, which uses the cpu on which the trace was recorded.\n\
+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\
+When set to NONE, errata workarounds are disabled."),
+		  &set_record_btrace_cpu_cmdlist, _("set record btrace cpu "),
+		  1, &set_record_btrace_cmdlist);
+
+  add_cmd ("auto", class_support, cmd_set_record_btrace_cpu_auto, _("\
+Automatically determine the cpu to be used for trace decode."),
+	   &set_record_btrace_cpu_cmdlist);
+
+  add_cmd ("none", class_support, cmd_set_record_btrace_cpu_none, _("\
+Do not enable errata workarounds for trace decode."),
+	   &set_record_btrace_cpu_cmdlist);
+
+  add_cmd ("cpu", class_support, cmd_show_record_btrace_cpu, _("\
+Show the cpu to be used for trace decode."),
+	   &show_record_btrace_cmdlist);
+
   add_prefix_cmd ("bts", class_support, cmd_set_record_btrace_bts,
 		  _("Set record btrace bts options"),
 		  &set_record_btrace_bts_cmdlist,
diff --git a/gdb/record-btrace.h b/gdb/record-btrace.h
index ba66719..f0341ef 100644
--- a/gdb/record-btrace.h
+++ b/gdb/record-btrace.h
@@ -25,4 +25,8 @@
 /* Push the record_btrace target.  */
 extern void record_btrace_push_target (void);
 
+/* Return the cpu configured by the user via "set btrace cpu".  Returns NULL if
+   the cpu was configured as auto.  */
+extern const struct btrace_cpu *record_btrace_get_cpu (void);
+
 #endif /* RECORD_BTRACE_H */
diff --git a/gdb/testsuite/gdb.btrace/cpu.exp b/gdb/testsuite/gdb.btrace/cpu.exp
new file mode 100644
index 0000000..a5b6275
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/cpu.exp
@@ -0,0 +1,74 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2018 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+gdb_start
+
+proc test_good { arg } {
+    gdb_test_no_output "set record btrace cpu $arg" "set cpu $arg"
+    gdb_test "show record btrace cpu" "btrace cpu is '$arg'\." "show cpu $arg"
+}
+
+proc test_bad { arg current } {
+    gdb_test "set record btrace cpu $arg" \
+        "Bad format\.  See \"help set record btrace cpu\"\." \
+        "set cpu $arg"
+    gdb_test "show record btrace cpu" "btrace cpu is '$current'\." \
+        "show cpu $arg"
+}
+
+proc test_junk { arg junk current } {
+    gdb_test "set record btrace cpu $arg" \
+        "Trailing junk: '$junk'\." \
+        "set cpu $arg"
+    gdb_test "show record btrace cpu" "btrace cpu is '$current'\." \
+        "show cpu $arg"
+}
+
+gdb_test "show record btrace cpu" "btrace cpu is 'auto'\." "default cpu"
+
+gdb_test "set record" \
+    "\"set record\" must be followed by an apporpriate subcommand.*" \
+    "set record"
+gdb_test "set record btrace" \
+    "\"set record btrace\" must be followed by an apporpriate subcommand.*" \
+    "set record btrace"
+test_bad "" "auto"
+
+test_good "intel: 0/0"
+test_good "intel: 0/0/1"
+
+# We omit a zero stepping in the output.
+gdb_test_no_output "set record btrace cpu intel: 0/0/0" "set cpu intel: 0/0/0"
+gdb_test "show record btrace cpu" "btrace cpu is 'intel: 0/0'\." \
+    "show cpu intel: 0/0/0"
+
+test_good "auto"
+test_good "none"
+
+test_bad "intel: foo" "none"
+test_bad "intel: 0" "none"
+test_bad "intel: 0/" "none"
+test_bad "intel: 0/foo" "none"
+test_bad "intel: foo/bar" "none"
+test_bad "intel: foo/0" "none"
+test_bad "intel: 0x0/0" "none"
+
+test_junk "intel: 0/0 foo" " foo" "none"
+test_junk "intel: 0/0x0" "x0" "none"
+test_junk "intel: 0/0/foo" "/foo" "none"
+test_junk "intel: 0/0/0 foo" " foo" "none"
+test_junk "intel: 0/0/0x0" "x0" "none"
-- 
1.8.3.1

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

[Eli Zaretskii]

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

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

[Metzger, Markus T]

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

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

[Eli Zaretskii]

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

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

[Metzger, Markus T]

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

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

[Eli Zaretskii]

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

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

[Metzger, Markus T]

Hello Eli,

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

I'm not sure whether the term 'firmware' is correct.  I would instead phrase
it like this:

"Errata may cause the recorded trace to not match the specification.  This,
in turn, may cause trace decode to fail".

Then continue with "@value{GDBN} can detect ..." as you suggested.


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

That sounds good.  Thanks for your help.

Below is the full doc patch.

regards,
Markus.

---
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ee7adc8..2abb8d7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6952,10 +6952,72 @@ 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.
+
+Errata may cause the recorded trace to not match the specification.
+This, in turn, may cause trace decode to fail.  @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: @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

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

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Wed, 28 Feb 2018 07:38:03 +0000
> > 
> >   @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.
> 
> I'm not sure whether the term 'firmware' is correct.  I would instead phrase
> it like this:
> 
> "Errata may cause the recorded trace to not match the specification.  This,
> in turn, may cause trace decode to fail".

But that completely loses the explanation of what the errata are.  If
my explanation is not accurate, let's correct it, rather than deleting
it.

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

[Metzger, Markus T]

Hello Eli,

> > >   @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.
> >
> > I'm not sure whether the term 'firmware' is correct.  I would instead
> > phrase it like this:
> >
> > "Errata may cause the recorded trace to not match the specification.
> > This, in turn, may cause trace decode to fail".
> 
> But that completely loses the explanation of what the errata are.  If my
> explanation is not accurate, let's correct it, rather than deleting it.

I didn't mean to delete your explanation.  I only removed the 'firmware' part.
And the bit about 'unaware decoders' because it really only matters to the
reader what GDB's decoder does.  Or that's what I thought I did.  What else is
missing?

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

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

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Thu, 1 Mar 2018 07:05:42 +0000
> 
> > > >   @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.
> > >
> > But that completely loses the explanation of what the errata are.  If my
> > explanation is not accurate, let's correct it, rather than deleting it.
> 
> I didn't mean to delete your explanation.  I only removed the 'firmware' part.

The text I proposed is above.  It begins with an explanation of what
those errata are, and why they are detrimental to btrace.  The text
you proposed instead is this:

  Errata may cause the recorded trace to not match the specification.
  This, in turn, may cause trace decode to fail.  @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.

This just says that trace decode can fail, but tells nothing about the
phenomenon itself.  Thus my "completely loses" reaction.

But I don't want to argue.  If you feel that the text you wrote is
good enough, go ahead and push it, even though I'm unhappy.

Thanks.

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

[Metzger, Markus T]

Hello Eli,

> > > > >   @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.
> > > >
> > > But that completely loses the explanation of what the errata are.
> > > If my explanation is not accurate, let's correct it, rather than deleting it.
> >
> > I didn't mean to delete your explanation.  I only removed the 'firmware' part.
> 
> The text I proposed is above.  It begins with an explanation of what those errata
> are, and why they are detrimental to btrace.  The text you proposed instead is
> this:
> 
>   Errata may cause the recorded trace to not match the specification.
>   This, in turn, may cause trace decode to fail.  @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.
> 
> This just says that trace decode can fail, but tells nothing about the phenomenon
> itself.  Thus my "completely loses" reaction.
> 
> But I don't want to argue.  If you feel that the text you wrote is good enough, go
> ahead and push it, even though I'm unhappy.

I'd rather we find a wording we can all agree on.

I added that errata cause the trace to not match the spec and that this would
cause decode to fail.

I removed the bit about errata being in the firmware since that is not correct.

And I replaced "unaware decoders might fail to decode such a trace" with "This,
in turn, may cause trace decode to fail".

The rest I took as you suggested.

Where do you think we need to improve the wording?

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

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

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Thu, 1 Mar 2018 16:23:59 +0000
> 
> Where do you think we need to improve the wording?

I'd like to have an explanation of what the errata are and what causes
them.  If "bugs in firmware" is inaccurate, then how do we describe
the reason accurately?  (If you can point me to some URL where that is
described, I might be able to come up with an accurate description
myself.)

Than ks.

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

[Metzger, Markus T]

Hello Eli,

> > Where do you think we need to improve the wording?
> 
> I'd like to have an explanation of what the errata are and what causes them.  If
> "bugs in firmware" is inaccurate, then how do we describe the reason accurately?
> (If you can point me to some URL where that is described, I might be able to
> come up with an accurate description
> myself.)

The term "erratum" already means 'bug somewhere in the processor'.

From https://en.wikipedia.org/wiki/Erratum:

    "Design errors and mistakes in a CPU's hardwired logic may also be documented
     and described as errata."

Intel publishes them in documents called ....-spec-update.pdf.

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

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

[Maciej W. Rozycki]

On Thu, 1 Mar 2018, Metzger, Markus T wrote:

> The term "erratum" already means 'bug somewhere in the processor'.

 The term "erratum" is generic and usually means a mistake in a published 
text.  You need to be more specific and clarify what the term means in 
this context.

 I think that Eli's proposal sounds about right, except that I agree that 
"firmware" does not really fit here; an erratum may be present in hardware 
logic or in microcode, or it may even be a phenomenon of the manufacturing 
process, e.g. cases have been known where a malfunction of a specific CPU 
operation was caused by thermal effects in silicon in otherwise seemingly 
correct logic.  This makes a "processor erratum" a broad term and 
therefore I think this specific case does not belong to the concept index.

 So how about:

  Processor errata are known to exist 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 or workarounds are enabled based on the processor on 
  which the trace was recorded.

?

  Maciej

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

[Eli Zaretskii]

> Date: Fri, 2 Mar 2018 14:15:20 +0000
> From: "Maciej W. Rozycki" <macro@mips.com>
> CC: Eli Zaretskii <eliz@gnu.org>, "gdb-patches@sourceware.org"
> 	<gdb-patches@sourceware.org>
> 
> On Thu, 1 Mar 2018, Metzger, Markus T wrote:
> 
> > The term "erratum" already means 'bug somewhere in the processor'.
> 
>  The term "erratum" is generic and usually means a mistake in a published 
> text.  You need to be more specific and clarify what the term means in 
> this context.
> 
>  I think that Eli's proposal sounds about right, except that I agree that 
> "firmware" does not really fit here; an erratum may be present in hardware 
> logic or in microcode, or it may even be a phenomenon of the manufacturing 
> process, e.g. cases have been known where a malfunction of a specific CPU 
> operation was caused by thermal effects in silicon in otherwise seemingly 
> correct logic.  This makes a "processor erratum" a broad term and 
> therefore I think this specific case does not belong to the concept index.
> 
>  So how about:
> 
>   Processor errata are known to exist 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 or workarounds are enabled based on the processor on 
>   which the trace was recorded.

That still doesn't explain what are those errata.

How about replacing the first sentence above with these two:

  Processor errata are defects in processor operation, caused by its
  design or manufacture.  They can cause a trace not to match the
  specification.

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

[Maciej W. Rozycki]

On Fri, 2 Mar 2018, Eli Zaretskii wrote:

> >  So how about:
> > 
> >   Processor errata are known to exist 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 or workarounds are enabled based on the processor on 
> >   which the trace was recorded.
> 
> That still doesn't explain what are those errata.
> 
> How about replacing the first sentence above with these two:
> 
>   Processor errata are defects in processor operation, caused by its
>   design or manufacture.  They can cause a trace not to match the
>   specification.

 Fine with me.

  Maciej

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

[Maciej W. Rozycki]

On Fri, 2 Mar 2018, Eli Zaretskii wrote:

> >  So how about:
> > 
> >   Processor errata are known to exist 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 or workarounds are enabled based on the processor on 
> >   which the trace was recorded.
> 
> That still doesn't explain what are those errata.
> 
> How about replacing the first sentence above with these two:
> 
>   Processor errata are defects in processor operation, caused by its
>   design or manufacture.  They can cause a trace not to match the
>   specification.

 Fine with me.

  Maciej

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

[Metzger, Markus T]

Hello Eli, Maciej,

> > >  So how about:
> > >
> > >   Processor errata are known to exist 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 or workarounds are enabled based on the processor on
> > >   which the trace was recorded.
> >
> > That still doesn't explain what are those errata.
> >
> > How about replacing the first sentence above with these two:
> >
> >   Processor errata are defects in processor operation, caused by its
> >   design or manufacture.  They can cause a trace not to match the
> >   specification.
> 
>  Fine with me.

Fine with me, as well.  I will send out the full series with the updated
wording and a reduced column limit.

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