[PATCH v2 4/4] btrace: add instruction-history /s and fix documentation

[PATCH v2 4/4] btrace: add instruction-history /s and fix documentation

[Markus Metzger]

Add support for the /s modifier of the "record instruction-history" command.  It
behaves exactly like /m and prints disassembled instructions in the order in
which they were recorded with interleaved sources.  We accept /s in addition
to /m to align with the "disassemble" command.

The "record instruction-history" modifiers were not documented.  Document
all of them.

2015-10-19  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* record.c (get_insn_history_modifiers): Set DISASSEMBLY_SOURCE
	instead of DISASSEMBLY_SOURCE_DEPRECATED.  Also accept /s.
	(_initialize_record): Document the /s modifier.
	* NEWS: Announce record instruction-history's new /s modifier.

doc/
	* gdb.texinfo (Process Record and Replay): Document "record
	instruction-history" modifiers.

2015-10-19  Markus Metzger <markus.t.metzger@intel.com>
---
 gdb/NEWS            |  3 +++
 gdb/doc/gdb.texinfo | 14 ++++++++++++++
 gdb/record.c        |  5 +++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index b2b1e99..78d1328 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -51,6 +51,9 @@ show remote multiprocess-extensions-packet
   The "/m" option is now considered deprecated: its "source-centric"
   output hasn't proved useful in practice.
 
+* The "record instruction-history" command accepts a new modifier: /s.
+  It behaves exactly like /m and prints mixed source+disassembly.
+
 * The "set scheduler-locking" command supports a new mode "replay".
   It behaves like "off" in record mode and like "on" in replay mode.
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 3c1f785..3e49fc7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6741,6 +6741,20 @@ default, ten instructions are disassembled.  This can be changed using
 the @code{set record instruction-history-size} command.  Instructions
 are printed in execution order.
 
+It can also print mixed source+disassembly by specifying the @code{/m}
+or @code{/s} modifier and print the raw instructions in hex as well as
+in symbolic form by specifying the @code{/r} modifier.
+
+The current position marker is printed for the instruction at the
+current program counter value.  This instruction can appear multiple
+times in the trace and the current position marker will be printed
+every time.  To omit the current position marker, specify the
+@code{/p} modifier.
+
+To better align the printed instructions when the trace contains
+instructions from more than one function, the function name may be
+omitted by specifying the @code{/f} modifier.
+
 Speculatively executed instructions are prefixed with @samp{?}.  This
 feature is not available for all recording formats.
 
diff --git a/gdb/record.c b/gdb/record.c
index 71ef973..b06bec7 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -458,7 +458,8 @@ get_insn_history_modifiers (char **arg)
 	  switch (*args)
 	    {
 	    case 'm':
-	      modifiers |= DISASSEMBLY_SOURCE_DEPRECATED;
+	    case 's':
+	      modifiers |= DISASSEMBLY_SOURCE;
 	      modifiers |= DISASSEMBLY_FILENAME;
 	      break;
 	    case 'r':
@@ -817,7 +818,7 @@ Argument is instruction number, as shown by 'info record'."),
 
   add_cmd ("instruction-history", class_obscure, cmd_record_insn_history, _("\
 Print disassembled instructions stored in the execution log.\n\
-With a /m modifier, source lines are included (if available).\n\
+With a /m or /s modifier, source lines are included (if available).\n\
 With a /r modifier, raw instructions in hex are included.\n\
 With a /f modifier, function names are omitted.\n\
 With a /p modifier, current position markers are omitted.\n\
-- 
1.8.3.1

[PATCH v2 3/4] btrace: change record instruction-history /m

[Markus Metzger]

The /m modifier interleaves source lines with the disassembly of recorded
instructions.  This calls disasm.c's gdb_disassembly once for each recorded
instruction to be printed.

This doesn't really work because gdb_disassembly may choose not to print
anything in some situations.  And if it does print something, the output
interferes with btrace_insn_history's output around it.

It further results in a separate asm_insns list for each instruction in MI.
Even though there is no MI support for target record, yet, we fix this obvious
issue.

Change record instruction-history /m to use the new gdb_print_insn_tuple
function for printing a single instruction and interleave source lines as
appropriate.

We cannot reuse the new disasm.c do_mixed_source_and_assembly function without
significant changes to it.

2015-10-19  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* record-btrace.c (struct btrace_line_range): New.
	(btrace_mk_line_range, btrace_line_range_add)
	(btrace_line_range_is_empty, btrace_line_range_contains_range)
	(btrace_find_line_range, btrace_print_lines): New.
	(btrace_insn_history): Add source interleaving algorithm.
---
 gdb/record-btrace.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 169 insertions(+), 22 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3f068e2..1bf24a8 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -526,6 +526,133 @@ ui_out_field_uint (struct ui_out *uiout, const char *fld, unsigned int val)
   ui_out_field_fmt (uiout, fld, "%u", val);
 }
 
+/* A range of source lines.  */
+
+struct btrace_line_range
+{
+  /* The symtab this line is from.  */
+  struct symtab *symtab;
+
+  /* The first line (inclusive).  */
+  int begin;
+
+  /* The last line (exclusive).  */
+  int end;
+};
+
+/* Construct a line range.  */
+
+static struct btrace_line_range
+btrace_mk_line_range (struct symtab *symtab, int begin, int end)
+{
+  struct btrace_line_range range;
+
+  range.symtab = symtab;
+  range.begin = begin;
+  range.end = end;
+
+  return range;
+}
+
+/* Add a line to a line range.  */
+
+static struct btrace_line_range
+btrace_line_range_add (struct btrace_line_range range, int line)
+{
+  if (range.end <= range.begin)
+    {
+      /* This is the first entry.  */
+      range.begin = line;
+      range.end = line + 1;
+    }
+  else if (line < range.begin)
+    range.begin = line;
+  else if (range.end < line)
+    range.end = line;
+
+  return range;
+}
+
+/* Return non-zero if RANGE is empty, zero otherwise.  */
+
+static int
+btrace_line_range_is_empty (struct btrace_line_range range)
+{
+  return (range.end <= range.begin);
+}
+
+/* Return non-zero if LHS contains RHS, zero otherwise.  */
+
+static int
+btrace_line_range_contains_range (struct btrace_line_range lhs,
+				  struct btrace_line_range rhs)
+{
+  return ((lhs.symtab == rhs.symtab)
+	  && (lhs.begin <= rhs.begin)
+	  && (rhs.end <= lhs.end));
+}
+
+/* Find the line range associated with PC.  */
+
+static struct btrace_line_range
+btrace_find_line_range (CORE_ADDR pc)
+{
+  struct btrace_line_range range;
+  struct linetable_entry *lines;
+  struct linetable *ltable;
+  struct symtab *symtab;
+  int nlines, i;
+
+  symtab = find_pc_line_symtab (pc);
+  if (symtab == NULL)
+    return btrace_mk_line_range (NULL, 0, 0);
+
+  ltable = SYMTAB_LINETABLE (symtab);
+  if (ltable == NULL)
+    return btrace_mk_line_range (symtab, 0, 0);
+
+  nlines = ltable->nitems;
+  lines = ltable->item;
+  if (nlines <= 0)
+    return btrace_mk_line_range (symtab, 0, 0);
+
+  range = btrace_mk_line_range (symtab, 0, 0);
+  for (i = 0; i < nlines - 1; i++)
+    {
+      if ((lines[i].pc == pc) && (lines[i].line != 0))
+	range = btrace_line_range_add (range, lines[i].line);
+    }
+
+  return range;
+}
+
+/* Print source lines in LINES.  */
+
+static void
+btrace_print_lines (struct btrace_line_range lines, struct ui_out *uiout,
+		    struct cleanup **ui_item, int flags)
+{
+  enum print_source_lines_flags psl_flags;
+  int line;
+
+  psl_flags = 0;
+  if (flags & DISASSEMBLY_FILENAME)
+    psl_flags |= PRINT_SOURCE_LINES_FILENAME;
+
+  for (line = lines.begin; line < lines.end; ++line)
+    {
+      if (*ui_item != NULL)
+	do_cleanups (*ui_item);
+
+      *ui_item
+	= make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line");
+
+      print_source_lines (lines.symtab, line, line + 1, psl_flags);
+
+      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
+    }
+}
+
 /* Disassemble a section of the recorded instruction trace.  */
 
 static void
@@ -534,13 +661,26 @@ btrace_insn_history (struct ui_out *uiout,
 		     const struct btrace_insn_iterator *begin,
 		     const struct btrace_insn_iterator *end, int flags)
 {
+  struct ui_file *stb;
+  struct cleanup *cleanups, *ui_item;
+  struct disassemble_info di;
   struct gdbarch *gdbarch;
   struct btrace_insn_iterator it;
+  struct btrace_line_range last_lines;
 
   DEBUG ("itrace (0x%x): [%u; %u)", flags, btrace_insn_number (begin),
 	 btrace_insn_number (end));
 
+  flags |= DISASSEMBLY_SPECULATIVE;
+
   gdbarch = target_gdbarch ();
+  stb = mem_fileopen ();
+  cleanups = make_cleanup_ui_file_delete (stb);
+  di = gdb_disassemble_info (gdbarch, stb);
+  last_lines = btrace_mk_line_range (NULL, 0, 0);
+
+  make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
+  ui_item = NULL;
 
   for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
     {
@@ -563,37 +703,44 @@ btrace_insn_history (struct ui_out *uiout,
 	}
       else
 	{
-	  char prefix[4];
+	  struct disasm_insn dinsn;
 
-	  /* We may add a speculation prefix later.  We use the same space
-	     that is used for the pc prefix.  */
-	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
-	    strncpy (prefix, pc_prefix (insn->pc), 3);
-	  else
+	  if ((flags & DISASSEMBLY_SOURCE) != 0)
 	    {
-	      prefix[0] = ' ';
-	      prefix[1] = ' ';
-	      prefix[2] = ' ';
+	      struct btrace_line_range lines;
+
+	      lines = btrace_find_line_range (insn->pc);
+	      if (!btrace_line_range_is_empty (lines)
+		  && !btrace_line_range_contains_range (last_lines, lines))
+		{
+		  btrace_print_lines (lines, uiout, &ui_item, flags);
+		  last_lines = lines;
+		}
+	      else if (ui_item == NULL)
+		{
+		  ui_item =
+		    make_cleanup_ui_out_tuple_begin_end (uiout,
+							 "src_and_asm_line");
+		  /* No source information.  */
+		  make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
+		}
+
+	      gdb_assert (ui_item != NULL);
 	    }
-	  prefix[3] = 0;
 
-	  /* Print the instruction index.  */
-	  ui_out_field_uint (uiout, "index", btrace_insn_number (&it));
-	  ui_out_text (uiout, "\t");
+	  memset (&dinsn, 0, sizeof (dinsn));
+	  dinsn.number = btrace_insn_number (&it);
+	  dinsn.addr = insn->pc;
 
-	  /* Indicate speculative execution by a leading '?'.  */
 	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
-	    prefix[0] = '?';
+	    dinsn.is_speculative = 1;
 
-	  /* Print the prefix; we tell gdb_disassembly below to omit it.  */
-	  ui_out_field_fmt (uiout, "prefix", "%s", prefix);
-
-	  /* Disassembly with '/m' flag may not produce the expected result.
-	     See PR gdb/11833.  */
-	  gdb_disassembly (gdbarch, uiout, NULL, flags | DISASSEMBLY_OMIT_PC,
-			   1, insn->pc, insn->pc + 1);
+	  gdb_print_insn_tuple (gdbarch, uiout, &di, &dinsn, flags, stb);
 	}
     }
+
+  do_cleanups (cleanups);
+  gdb_flush (gdb_stdout);
 }
 
 /* The to_insn_history method of target record-btrace.  */
-- 
1.8.3.1

[PATCH v2 1/4] disasm: split dump_insns

[Markus Metzger]

Split disasm.c's dump_insn into two parts:

  - print a single instruction
  - loop over the specified address range

The first part will be refined in subsequent patches so it can be reused.

2015-10-19  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.c (dump_insns):  Split into this and ...
	(gdb_print_insn_tuple): ... this.
---
 gdb/disasm.c | 172 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 95 insertions(+), 77 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 6e3d6c1..4c80c5f 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -170,100 +170,118 @@ compare_lines (const void *mle1p, const void *mle2p)
   return val;
 }
 
+/* Prints the instruction at PC into UIOUT and returns the length of the
+   printed instruction in bytes.  */
+
 static int
-dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
-	    struct disassemble_info * di,
-	    CORE_ADDR low, CORE_ADDR high,
-	    int how_many, int flags, struct ui_file *stb,
-	    CORE_ADDR *end_pc)
+gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
+		      struct disassemble_info * di, CORE_ADDR pc, int flags,
+		      struct ui_file *stb)
 {
-  int num_displayed = 0;
-  CORE_ADDR pc;
-
   /* parts of the symbolic representation of the address */
   int unmapped;
   int offset;
   int line;
+  int size;
   struct cleanup *ui_out_chain;
+  char *filename = NULL;
+  char *name = NULL;
+
+  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
+  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+    ui_out_text (uiout, pc_prefix (pc));
+  ui_out_field_core_addr (uiout, "address", gdbarch, pc);
+
+  if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
+			       &line, &unmapped))
+    {
+      /* We don't care now about line, filename and unmapped.  But we might in
+	 the future.  */
+      ui_out_text (uiout, " <");
+      if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
+	ui_out_field_string (uiout, "func-name", name);
+      ui_out_text (uiout, "+");
+      ui_out_field_int (uiout, "offset", offset);
+      ui_out_text (uiout, ">:\t");
+    }
+  else
+    ui_out_text (uiout, ":\t");
 
-  for (pc = low; pc < high;)
+  if (filename != NULL)
+    xfree (filename);
+  if (name != NULL)
+    xfree (name);
+
+  ui_file_rewind (stb);
+  if (flags & DISASSEMBLY_RAW_INSN)
     {
-      char *filename = NULL;
-      char *name = NULL;
+      CORE_ADDR end_pc;
+      bfd_byte data;
+      int status;
+      const char *spacer = "";
 
-      QUIT;
-      if (how_many >= 0)
-	{
-	  if (num_displayed >= how_many)
-	    break;
-	  else
-	    num_displayed++;
-	}
-      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+      /* Build the opcodes using a temporary stream so we can
+	 write them out in a single go for the MI.  */
+      struct ui_file *opcode_stream = mem_fileopen ();
+      struct cleanup *cleanups =
+	make_cleanup_ui_file_delete (opcode_stream);
 
-      if ((flags & DISASSEMBLY_OMIT_PC) == 0)
-	ui_out_text (uiout, pc_prefix (pc));
-      ui_out_field_core_addr (uiout, "address", gdbarch, pc);
+      size = gdbarch_print_insn (gdbarch, pc, di);
+      end_pc = pc + size;
 
-      if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
-				   &line, &unmapped))
+      for (;pc < end_pc; ++pc)
 	{
-	  /* We don't care now about line, filename and
-	     unmapped. But we might in the future.  */
-	  ui_out_text (uiout, " <");
-	  if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
-	    ui_out_field_string (uiout, "func-name", name);
-	  ui_out_text (uiout, "+");
-	  ui_out_field_int (uiout, "offset", offset);
-	  ui_out_text (uiout, ">:\t");
+	  status = (*di->read_memory_func) (pc, &data, 1, di);
+	  if (status != 0)
+	    (*di->memory_error_func) (status, pc, di);
+	  fprintf_filtered (opcode_stream, "%s%02x",
+			    spacer, (unsigned) data);
+	  spacer = " ";
 	}
-      else
-	ui_out_text (uiout, ":\t");
-
-      if (filename != NULL)
-	xfree (filename);
-      if (name != NULL)
-	xfree (name);
-
-      ui_file_rewind (stb);
-      if (flags & DISASSEMBLY_RAW_INSN)
-        {
-          CORE_ADDR old_pc = pc;
-          bfd_byte data;
-          int status;
-          const char *spacer = "";
-
-          /* Build the opcodes using a temporary stream so we can
-             write them out in a single go for the MI.  */
-          struct ui_file *opcode_stream = mem_fileopen ();
-          struct cleanup *cleanups =
-            make_cleanup_ui_file_delete (opcode_stream);
-
-          pc += gdbarch_print_insn (gdbarch, pc, di);
-          for (;old_pc < pc; old_pc++)
-            {
-              status = (*di->read_memory_func) (old_pc, &data, 1, di);
-              if (status != 0)
-                (*di->memory_error_func) (status, old_pc, di);
-              fprintf_filtered (opcode_stream, "%s%02x",
-                                spacer, (unsigned) data);
-              spacer = " ";
-            }
-          ui_out_field_stream (uiout, "opcodes", opcode_stream);
-          ui_out_text (uiout, "\t");
-
-          do_cleanups (cleanups);
-        }
-      else
-        pc += gdbarch_print_insn (gdbarch, pc, di);
-      ui_out_field_stream (uiout, "inst", stb);
-      ui_file_rewind (stb);
-      do_cleanups (ui_out_chain);
-      ui_out_text (uiout, "\n");
+      ui_out_field_stream (uiout, "opcodes", opcode_stream);
+      ui_out_text (uiout, "\t");
+
+      do_cleanups (cleanups);
+    }
+  else
+    size = gdbarch_print_insn (gdbarch, pc, di);
+
+  ui_out_field_stream (uiout, "inst", stb);
+  ui_file_rewind (stb);
+  do_cleanups (ui_out_chain);
+  ui_out_text (uiout, "\n");
+
+  return size;
+}
+
+static int
+dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
+	    struct disassemble_info * di,
+	    CORE_ADDR low, CORE_ADDR high,
+	    int how_many, int flags, struct ui_file *stb,
+	    CORE_ADDR *end_pc)
+{
+  int num_displayed = 0;
+
+  while (low < high && (how_many < 0 || num_displayed < how_many))
+    {
+      int size;
+
+      size = gdb_print_insn_tuple (gdbarch, uiout, di, low, flags, stb);
+      if (size <= 0)
+	break;
+
+      num_displayed += 1;
+      low += size;
+
+      /* Allow user to bail out with ^C.  */
+      QUIT;
     }
 
   if (end_pc != NULL)
-    *end_pc = pc;
+    *end_pc = low;
+
   return num_displayed;
 }
 
-- 
1.8.3.1

[PATCH v2 0/4] disasm, record: fix "record instruction-history /m"

[Markus Metzger]

The /m modifier interleaves source lines with the disassembly of recorded
instructions.  This calls disasm.c's do_mixed_source_and_assembly once for
each recorded instruction to be printed.

The latter really does a source print with intermixed disassembly.  To that
purpose, it may reorder instructions to better match the source.  This doesn't
work for printing a single instruction.  The output also interferes with
"record instruction-history" specific extensions.

Change record instruction-history /m to use its own simple source interleaving
algorithm.  The most important part is that instructions are printed in
the order in which they were executed.  Before each instruction, we print
a range of source lines that are attributed to the instruction's PC.  If the
source line range has already been printed (or skipped) for the previous
instruction, we skip it.

This is a mixture of an earlier RFC that added source interleaving to the
"record instruction-history" command and version 1 of this patch series that
tried to modify Doug Evans new "disassembly" source interleaving algorithm and
use it for "record instruction-history", as well.

We extend disasm.c's insn tuple printing and use it for printing a single
instruction.  To also share the source interleaving algorithm, we'd need to
change it significantly.  When building a hash table of source lines of the
memory range to disassemble and when inserting lines without source code, the
algorithm assumes that instructions are printed in the order in which they
appear in memory.  The "record instruction-history" command needs to print
instructions in the order in which they retired.

I propose to leave the two source interleaving algorithms separate.


Markus Metzger (4):
  disasm: split dump_insns
  disasm: add struct disasm_insn to describe to-be-disassembled
    instruction
  btrace: change record instruction-history /m
  btrace: add instruction-history /s and fix documentation

 gdb/NEWS            |   3 +
 gdb/disasm.c        | 199 ++++++++++++++++++++++++++++++++--------------------
 gdb/disasm.h        |  23 ++++++
 gdb/doc/gdb.texinfo |  14 ++++
 gdb/record-btrace.c | 191 +++++++++++++++++++++++++++++++++++++++++++------
 gdb/record.c        |   5 +-
 6 files changed, 336 insertions(+), 99 deletions(-)

-- 
1.8.3.1

[PATCH v2 2/4] disasm: add struct disasm_insn to describe to-be-disassembled instruction

[Markus Metzger]

The "record instruction-history" command prints for each instruction in
addition to the instruction's disassembly:

  - the instruction number in the recorded execution trace
  - a '?' before the instruction if it was executed speculatively

To allow the "record instruction-history" command to use GDB's disassembly
infrastructure, we extend gdb_print_insn_tuple to optionally print those
additional fields and export the function.

Add a new struct disasm_insn to add additional fields describing the
to-be-disassembled instruction.  The additional fields are:

  number            an optional instruction number, zero if omitted.
  is_speculative    a predicate saying whether the instruction was
                    executed speculatively.

If non-zero, the instruction number is printed first.  It will also appear
as a new optional field "insn-number" in MI.  The field will be present if
insn_num is non-zero.

If is_speculative is set, speculative execution will be indicated by a "?"
following the new instruction number field.  Unless the PC is omitted, it
will overwrite the first byte of the PC prefix.  It will appear as a new
optional field "is-speculative" in MI.  The field will contain "?" and will
be present if is_speculative is set.

The speculative execution indication is guarded by a new flag
DISASSEMBLY_SPECULATION.

Replace the PC parameter of gdb_print_insn_tuple with a pointer to the above
struct.  GDB's "disassemble" command does not use the new fields.

2015-10-19  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.h (DISASSEMBLY_SPECULATION): New.
	(struct disasm_insn): New.
	(gdb_print_insn_tuple): New.
	* disasm.c (gdb_print_insn_tuple): Replace parameter PC with INSN.
	Update users.  Print instruction number and indicate speculative
	execution, if requested.
---
 gdb/disasm.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
 gdb/disasm.h | 23 +++++++++++++++++++++++
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 4c80c5f..9b3f156 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -170,12 +170,12 @@ compare_lines (const void *mle1p, const void *mle2p)
   return val;
 }
 
-/* Prints the instruction at PC into UIOUT and returns the length of the
-   printed instruction in bytes.  */
+/* See disasm.h.  */
 
-static int
+int
 gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
-		      struct disassemble_info * di, CORE_ADDR pc, int flags,
+		      struct disassemble_info * di,
+		      const struct disasm_insn *insn, int flags,
 		      struct ui_file *stb)
 {
   /* parts of the symbolic representation of the address */
@@ -186,10 +186,37 @@ gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct cleanup *ui_out_chain;
   char *filename = NULL;
   char *name = NULL;
+  CORE_ADDR pc;
 
   ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+  pc = insn->addr;
+
+  if (insn->number != 0)
+    {
+      ui_out_field_fmt (uiout, "insn-number", "%u", insn->number);
+      ui_out_text (uiout, "\t");
+    }
 
-  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+  if ((flags & DISASSEMBLY_SPECULATIVE) != 0)
+    {
+      if (insn->is_speculative)
+	{
+	  ui_out_field_string (uiout, "is-speculative", "?");
+
+	  /* The speculative execution indication overwrites the first
+	     character of the PC prefix.
+	     We assume a PC prefix length of 3 characters.  */
+	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	    ui_out_text (uiout, pc_prefix (pc) + 1);
+	  else
+	    ui_out_text (uiout, "  ");
+	}
+      else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	ui_out_text (uiout, pc_prefix (pc));
+      else
+	ui_out_text (uiout, "   ");
+    }
+  else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
     ui_out_text (uiout, pc_prefix (pc));
   ui_out_field_core_addr (uiout, "address", gdbarch, pc);
 
@@ -262,25 +289,29 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
 	    int how_many, int flags, struct ui_file *stb,
 	    CORE_ADDR *end_pc)
 {
+  struct disasm_insn insn;
   int num_displayed = 0;
 
-  while (low < high && (how_many < 0 || num_displayed < how_many))
+  memset (&insn, 0, sizeof (insn));
+  insn.addr = low;
+
+  while (insn.addr < high && (how_many < 0 || num_displayed < how_many))
     {
       int size;
 
-      size = gdb_print_insn_tuple (gdbarch, uiout, di, low, flags, stb);
+      size = gdb_print_insn_tuple (gdbarch, uiout, di, &insn, flags, stb);
       if (size <= 0)
 	break;
 
       num_displayed += 1;
-      low += size;
+      insn.addr += size;
 
       /* Allow user to bail out with ^C.  */
       QUIT;
     }
 
   if (end_pc != NULL)
-    *end_pc = low;
+    *end_pc = insn.addr;
 
   return num_displayed;
 }
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 7e6f1a2..3b7d01a 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -27,11 +27,34 @@
 #define DISASSEMBLY_FILENAME	(0x1 << 3)
 #define DISASSEMBLY_OMIT_PC	(0x1 << 4)
 #define DISASSEMBLY_SOURCE	(0x1 << 5)
+#define DISASSEMBLY_SPECULATIVE	(0x1 << 6)
 
 struct gdbarch;
 struct ui_out;
 struct ui_file;
 
+/* An instruction to be disassembled.  */
+
+struct disasm_insn
+{
+  /* The address of the memory containing the instruction.  */
+  CORE_ADDR addr;
+
+  /* An optional instruction number.  If non-zero, it is printed first.  */
+  unsigned int number;
+
+  /* A bit-field saying whether the instruction was executed speculatively.  */
+  unsigned int is_speculative:1;
+};
+
+/* Prints the instruction INSN into UIOUT and returns the length of the
+   printed instruction in bytes.  */
+
+extern int gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
+				 struct disassemble_info * di,
+				 const struct disasm_insn *insn, int flags,
+				 struct ui_file *stb);
+
 /* Return a filled in disassemble_info object for use by gdb.  */
 
 extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,
-- 
1.8.3.1

Re: [PATCH v2 4/4] btrace: add instruction-history /s and fix documentation

[Eli Zaretskii]

> From: Markus Metzger <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org, palves@redhat.com
> Date: Mon, 19 Oct 2015 11:23:30 +0200
> 
> +It can also print mixed source+disassembly by specifying the @code{/m}
> +or @code{/s} modifier and print the raw instructions in hex as well as
> +in symbolic form by specifying the @code{/r} modifier.

"By specifying" makes it sound like you are talking about the command,
because of the "It" that starts the sentence.  Suggest to reword:

  It can also print mixed source+disassembly if you specify the @code{/m}
  or @code{/s} modifier, and print the raw instructions in hex as well as
  in symbolic form by specifying the @code{/r} modifier.

Note that I also added a missing comma before "and".

The documentation parts are okay with this fixed.

Thanks.

RE: [PATCH v2 4/4] btrace: add instruction-history /s and fix documentation

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Monday, October 19, 2015 11:37 AM
> To: Metzger, Markus T
> Cc: dje@google.com; gdb-patches@sourceware.org; palves@redhat.com
> Subject: Re: [PATCH v2 4/4] btrace: add instruction-history /s and fix
> documentation

Thanks for the quick response.

> > +It can also print mixed source+disassembly by specifying the @code{/m}
> > +or @code{/s} modifier and print the raw instructions in hex as well as
> > +in symbolic form by specifying the @code{/r} modifier.
> 
> "By specifying" makes it sound like you are talking about the command,
> because of the "It" that starts the sentence.  Suggest to reword:
> 
>   It can also print mixed source+disassembly if you specify the @code{/m}
>   or @code{/s} modifier, and print the raw instructions in hex as well as
>   in symbolic form by specifying the @code{/r} modifier.
> 
> Note that I also added a missing comma before "and".

Fixed.  Thanks.

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, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 0/4] disasm, record: fix "record instruction-history /m"

[Doug Evans]

Markus Metzger <markus.t.metzger@intel.com> writes:
> The /m modifier interleaves source lines with the disassembly of recorded
> instructions.  This calls disasm.c's do_mixed_source_and_assembly once for
> each recorded instruction to be printed.
>
> The latter really does a source print with intermixed disassembly.  To that
> purpose, it may reorder instructions to better match the source.  This doesn't
> work for printing a single instruction.  The output also interferes with
> "record instruction-history" specific extensions.
>
> Change record instruction-history /m to use its own simple source interleaving
> algorithm.  The most important part is that instructions are printed in
> the order in which they were executed.  Before each instruction, we print
> a range of source lines that are attributed to the instruction's PC.  If the
> source line range has already been printed (or skipped) for the previous
> instruction, we skip it.
>
> This is a mixture of an earlier RFC that added source interleaving to the
> "record instruction-history" command and version 1 of this patch series that
> tried to modify Doug Evans new "disassembly" source interleaving algorithm and
> use it for "record instruction-history", as well.
>
> We extend disasm.c's insn tuple printing and use it for printing a single
> instruction.  To also share the source interleaving algorithm, we'd need to
> change it significantly.  When building a hash table of source lines of the
> memory range to disassemble and when inserting lines without source code, the
> algorithm assumes that instructions are printed in the order in which they
> appear in memory.  The "record instruction-history" command needs to print
> instructions in the order in which they retired.
>
> I propose to leave the two source interleaving algorithms separate.

Hi.

This proposal is fine by me.

Re: [PATCH v2 4/4] btrace: add instruction-history /s and fix documentation

[Doug Evans]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:
>> -----Original Message-----
>> From: Eli Zaretskii [mailto:eliz@gnu.org]
>> Sent: Monday, October 19, 2015 11:37 AM
>> To: Metzger, Markus T
>> Cc: dje@google.com; gdb-patches@sourceware.org; palves@redhat.com
>> Subject: Re: [PATCH v2 4/4] btrace: add instruction-history /s and fix
>> documentation
>
> Thanks for the quick response.
>
>> > +It can also print mixed source+disassembly by specifying the @code{/m}
>> > +or @code{/s} modifier and print the raw instructions in hex as well as
>> > +in symbolic form by specifying the @code{/r} modifier.
>> 
>> "By specifying" makes it sound like you are talking about the command,
>> because of the "It" that starts the sentence.  Suggest to reword:
>> 
>>   It can also print mixed source+disassembly if you specify the @code{/m}
>>   or @code{/s} modifier, and print the raw instructions in hex as well as
>>   in symbolic form by specifying the @code{/r} modifier.
>> 
>> Note that I also added a missing comma before "and".
>
> Fixed.  Thanks.

4/4 LGTM otherwise.

Re: [PATCH v2 3/4] btrace: change record instruction-history /m

[Doug Evans]

Markus Metzger <markus.t.metzger@intel.com> writes:
> The /m modifier interleaves source lines with the disassembly of recorded
> instructions.  This calls disasm.c's gdb_disassembly once for each recorded
> instruction to be printed.
>
> This doesn't really work because gdb_disassembly may choose not to print
> anything in some situations.  And if it does print something, the output
> interferes with btrace_insn_history's output around it.
>
> It further results in a separate asm_insns list for each instruction in MI.
> Even though there is no MI support for target record, yet, we fix this obvious
> issue.
>
> Change record instruction-history /m to use the new gdb_print_insn_tuple
> function for printing a single instruction and interleave source lines as
> appropriate.
>
> We cannot reuse the new disasm.c do_mixed_source_and_assembly function without
> significant changes to it.
>
> 2015-10-19  Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> 	* record-btrace.c (struct btrace_line_range): New.
> 	(btrace_mk_line_range, btrace_line_range_add)
> 	(btrace_line_range_is_empty, btrace_line_range_contains_range)
> 	(btrace_find_line_range, btrace_print_lines): New.
> 	(btrace_insn_history): Add source interleaving algorithm.
> ---
>  gdb/record-btrace.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 169 insertions(+), 22 deletions(-)
>
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 3f068e2..1bf24a8 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -526,6 +526,133 @@ ui_out_field_uint (struct ui_out *uiout, const char *fld, unsigned int val)
>    ui_out_field_fmt (uiout, fld, "%u", val);
>  }
>  
> +/* A range of source lines.  */
> +
> +struct btrace_line_range
> +{
> +  /* The symtab this line is from.  */
> +  struct symtab *symtab;
> +
> +  /* The first line (inclusive).  */
> +  int begin;
> +
> +  /* The last line (exclusive).  */
> +  int end;
> +};
> +
> +/* Construct a line range.  */
> +
> +static struct btrace_line_range
> +btrace_mk_line_range (struct symtab *symtab, int begin, int end)
> +{
> +  struct btrace_line_range range;
> +
> +  range.symtab = symtab;
> +  range.begin = begin;
> +  range.end = end;
> +
> +  return range;
> +}
> +
> +/* Add a line to a line range.  */
> +
> +static struct btrace_line_range
> +btrace_line_range_add (struct btrace_line_range range, int line)
> +{
> +  if (range.end <= range.begin)
> +    {
> +      /* This is the first entry.  */
> +      range.begin = line;
> +      range.end = line + 1;
> +    }
> +  else if (line < range.begin)
> +    range.begin = line;
> +  else if (range.end < line)
> +    range.end = line;
> +
> +  return range;
> +}
> +
> +/* Return non-zero if RANGE is empty, zero otherwise.  */
> +
> +static int
> +btrace_line_range_is_empty (struct btrace_line_range range)
> +{
> +  return (range.end <= range.begin);
> +}
> +
> +/* Return non-zero if LHS contains RHS, zero otherwise.  */
> +
> +static int
> +btrace_line_range_contains_range (struct btrace_line_range lhs,
> +				  struct btrace_line_range rhs)
> +{
> +  return ((lhs.symtab == rhs.symtab)
> +	  && (lhs.begin <= rhs.begin)
> +	  && (rhs.end <= lhs.end));
> +}
> +
> +/* Find the line range associated with PC.  */
> +
> +static struct btrace_line_range
> +btrace_find_line_range (CORE_ADDR pc)
> +{
> +  struct btrace_line_range range;
> +  struct linetable_entry *lines;
> +  struct linetable *ltable;
> +  struct symtab *symtab;
> +  int nlines, i;
> +
> +  symtab = find_pc_line_symtab (pc);
> +  if (symtab == NULL)
> +    return btrace_mk_line_range (NULL, 0, 0);
> +
> +  ltable = SYMTAB_LINETABLE (symtab);
> +  if (ltable == NULL)
> +    return btrace_mk_line_range (symtab, 0, 0);
> +
> +  nlines = ltable->nitems;
> +  lines = ltable->item;
> +  if (nlines <= 0)
> +    return btrace_mk_line_range (symtab, 0, 0);
> +
> +  range = btrace_mk_line_range (symtab, 0, 0);
> +  for (i = 0; i < nlines - 1; i++)
> +    {
> +      if ((lines[i].pc == pc) && (lines[i].line != 0))
> +	range = btrace_line_range_add (range, lines[i].line);
> +    }
> +
> +  return range;
> +}
> +
> +/* Print source lines in LINES.  */

UI_ITEM needs to be documented, use of cleanups like this
is certainly atypical.
Plus the name is confusing (sorry!). I read "ui_item" and the last thing
I think of is it being a cleanup. ui_item_cleanup?
Or ui_item_chain which would be more consistent with the use in disasm.c.

> +
> +static void
> +btrace_print_lines (struct btrace_line_range lines, struct ui_out *uiout,
> +		    struct cleanup **ui_item, int flags)
> +{
> +  enum print_source_lines_flags psl_flags;
> +  int line;
> +
> +  psl_flags = 0;
> +  if (flags & DISASSEMBLY_FILENAME)
> +    psl_flags |= PRINT_SOURCE_LINES_FILENAME;
> +
> +  for (line = lines.begin; line < lines.end; ++line)
> +    {
> +      if (*ui_item != NULL)
> +	do_cleanups (*ui_item);
> +
> +      *ui_item
> +	= make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line");
> +
> +      print_source_lines (lines.symtab, line, line + 1, psl_flags);
> +
> +      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");

I think I understand what's going on here.
If we're printing multiple source lines that don't have insns
associated with them we still have to include in the output
line_asm_insn lists, even if they're empty.
It's a bit subtle though: the second,third,... times through the
loop we'll immediately run the cleanup set up by
make_cleanup_ui_out_list_begin_end, and I was left scratching my
head for a moment. The last time through the loop
we'll leave the line_asm_insn list open, so that we can add
disassembled insns to the list. IWBN to explain this to the reader.

> +    }
> +}
> +
>  /* Disassemble a section of the recorded instruction trace.  */
>  
>  static void
> @@ -534,13 +661,26 @@ btrace_insn_history (struct ui_out *uiout,
>  		     const struct btrace_insn_iterator *begin,
>  		     const struct btrace_insn_iterator *end, int flags)
>  {
> +  struct ui_file *stb;
> +  struct cleanup *cleanups, *ui_item;
> +  struct disassemble_info di;
>    struct gdbarch *gdbarch;
>    struct btrace_insn_iterator it;
> +  struct btrace_line_range last_lines;
>  
>    DEBUG ("itrace (0x%x): [%u; %u)", flags, btrace_insn_number (begin),
>  	 btrace_insn_number (end));
>  
> +  flags |= DISASSEMBLY_SPECULATIVE;
> +
>    gdbarch = target_gdbarch ();
> +  stb = mem_fileopen ();
> +  cleanups = make_cleanup_ui_file_delete (stb);
> +  di = gdb_disassemble_info (gdbarch, stb);
> +  last_lines = btrace_mk_line_range (NULL, 0, 0);
> +
> +  make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
> +  ui_item = NULL;
>  
>    for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
>      {
> @@ -563,37 +703,44 @@ btrace_insn_history (struct ui_out *uiout,
>  	}
>        else
>  	{
> -	  char prefix[4];
> +	  struct disasm_insn dinsn;
>  
> -	  /* We may add a speculation prefix later.  We use the same space
> -	     that is used for the pc prefix.  */
> -	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> -	    strncpy (prefix, pc_prefix (insn->pc), 3);
> -	  else
> +	  if ((flags & DISASSEMBLY_SOURCE) != 0)
>  	    {
> -	      prefix[0] = ' ';
> -	      prefix[1] = ' ';
> -	      prefix[2] = ' ';
> +	      struct btrace_line_range lines;
> +
> +	      lines = btrace_find_line_range (insn->pc);
> +	      if (!btrace_line_range_is_empty (lines)
> +		  && !btrace_line_range_contains_range (last_lines, lines))
> +		{
> +		  btrace_print_lines (lines, uiout, &ui_item, flags);
> +		  last_lines = lines;
> +		}
> +	      else if (ui_item == NULL)
> +		{
> +		  ui_item =

Community rules say that if you're going to break on an '=', the '='
goes on the next line.

> +		    make_cleanup_ui_out_tuple_begin_end (uiout,
> +							 "src_and_asm_line");
> +		  /* No source information.  */
> +		  make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
> +		}
> +
> +	      gdb_assert (ui_item != NULL);
>  	    }
> -	  prefix[3] = 0;
>  
> -	  /* Print the instruction index.  */
> -	  ui_out_field_uint (uiout, "index", btrace_insn_number (&it));
> -	  ui_out_text (uiout, "\t");
> +	  memset (&dinsn, 0, sizeof (dinsn));
> +	  dinsn.number = btrace_insn_number (&it);
> +	  dinsn.addr = insn->pc;
>  
> -	  /* Indicate speculative execution by a leading '?'.  */
>  	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
> -	    prefix[0] = '?';
> +	    dinsn.is_speculative = 1;
>  
> -	  /* Print the prefix; we tell gdb_disassembly below to omit it.  */
> -	  ui_out_field_fmt (uiout, "prefix", "%s", prefix);
> -
> -	  /* Disassembly with '/m' flag may not produce the expected result.
> -	     See PR gdb/11833.  */
> -	  gdb_disassembly (gdbarch, uiout, NULL, flags | DISASSEMBLY_OMIT_PC,
> -			   1, insn->pc, insn->pc + 1);
> +	  gdb_print_insn_tuple (gdbarch, uiout, &di, &dinsn, flags, stb);
>  	}
>      }
> +
> +  do_cleanups (cleanups);
> +  gdb_flush (gdb_stdout);

I know gdb_disassembly calls gdb_flush (gdb_stdout) too,
but I'm not sure why that is. Do you know?
I'd say delete it otherwise.

>  }
>  
>  /* The to_insn_history method of target record-btrace.  */

Re: [PATCH v2 1/4] disasm: split dump_insns

[Doug Evans]

[sorry for the dupe]

Markus Metzger <markus.t.metzger@intel.com> writes:
> Split disasm.c's dump_insn into two parts:
>
>   - print a single instruction
>   - loop over the specified address range
>
> The first part will be refined in subsequent patches so it can be reused.
>
> 2015-10-19  Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> 	* disasm.c (dump_insns):  Split into this and ...
> 	(gdb_print_insn_tuple): ... this.
> ---
>  gdb/disasm.c | 172 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 95 insertions(+), 77 deletions(-)
>
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 6e3d6c1..4c80c5f 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -170,100 +170,118 @@ compare_lines (const void *mle1p, const void *mle2p)
>    return val;
>  }
>  
> +/* Prints the instruction at PC into UIOUT and returns the length of the
> +   printed instruction in bytes.  */
> +
>  static int
> -dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> -	    struct disassemble_info * di,
> -	    CORE_ADDR low, CORE_ADDR high,
> -	    int how_many, int flags, struct ui_file *stb,
> -	    CORE_ADDR *end_pc)
> +gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,

The word "tuple" in the name here is odd.
Intuitively, since there is still "dump_insns" I'd expect this function
to be named "dump_insn".
Maybe it'll make sense once I read the entire patch set.
[I could do that first, but I often forget things I want to say
from the first time through, so sending them now is just easier.]

> +		      struct disassemble_info * di, CORE_ADDR pc, int flags,
> +		      struct ui_file *stb)
>  {
> -  int num_displayed = 0;
> -  CORE_ADDR pc;
> -
>    /* parts of the symbolic representation of the address */
>    int unmapped;
>    int offset;
>    int line;
> +  int size;
>    struct cleanup *ui_out_chain;
> +  char *filename = NULL;
> +  char *name = NULL;
> +
> +  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +
> +  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> +    ui_out_text (uiout, pc_prefix (pc));
> +  ui_out_field_core_addr (uiout, "address", gdbarch, pc);
> +
> +  if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
> +			       &line, &unmapped))
> +    {
> +      /* We don't care now about line, filename and unmapped.  But we might in
> +	 the future.  */
> +      ui_out_text (uiout, " <");
> +      if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
> +	ui_out_field_string (uiout, "func-name", name);
> +      ui_out_text (uiout, "+");
> +      ui_out_field_int (uiout, "offset", offset);
> +      ui_out_text (uiout, ">:\t");
> +    }
> +  else
> +    ui_out_text (uiout, ":\t");
>  
> -  for (pc = low; pc < high;)
> +  if (filename != NULL)
> +    xfree (filename);
> +  if (name != NULL)
> +    xfree (name);
> +
> +  ui_file_rewind (stb);
> +  if (flags & DISASSEMBLY_RAW_INSN)
>      {
> -      char *filename = NULL;
> -      char *name = NULL;
> +      CORE_ADDR end_pc;
> +      bfd_byte data;
> +      int status;
> +      const char *spacer = "";
>  
> -      QUIT;
> -      if (how_many >= 0)
> -	{
> -	  if (num_displayed >= how_many)
> -	    break;
> -	  else
> -	    num_displayed++;
> -	}
> -      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +      /* Build the opcodes using a temporary stream so we can
> +	 write them out in a single go for the MI.  */
> +      struct ui_file *opcode_stream = mem_fileopen ();
> +      struct cleanup *cleanups =
> +	make_cleanup_ui_file_delete (opcode_stream);
>  
> -      if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> -	ui_out_text (uiout, pc_prefix (pc));
> -      ui_out_field_core_addr (uiout, "address", gdbarch, pc);
> +      size = gdbarch_print_insn (gdbarch, pc, di);
> +      end_pc = pc + size;
>  
> -      if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
> -				   &line, &unmapped))
> +      for (;pc < end_pc; ++pc)
>  	{
> -	  /* We don't care now about line, filename and
> -	     unmapped. But we might in the future.  */
> -	  ui_out_text (uiout, " <");
> -	  if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
> -	    ui_out_field_string (uiout, "func-name", name);
> -	  ui_out_text (uiout, "+");
> -	  ui_out_field_int (uiout, "offset", offset);
> -	  ui_out_text (uiout, ">:\t");
> +	  status = (*di->read_memory_func) (pc, &data, 1, di);
> +	  if (status != 0)
> +	    (*di->memory_error_func) (status, pc, di);
> +	  fprintf_filtered (opcode_stream, "%s%02x",
> +			    spacer, (unsigned) data);
> +	  spacer = " ";
>  	}
> -      else
> -	ui_out_text (uiout, ":\t");
> -
> -      if (filename != NULL)
> -	xfree (filename);
> -      if (name != NULL)
> -	xfree (name);
> -
> -      ui_file_rewind (stb);
> -      if (flags & DISASSEMBLY_RAW_INSN)
> -        {
> -          CORE_ADDR old_pc = pc;
> -          bfd_byte data;
> -          int status;
> -          const char *spacer = "";
> -
> -          /* Build the opcodes using a temporary stream so we can
> -             write them out in a single go for the MI.  */
> -          struct ui_file *opcode_stream = mem_fileopen ();
> -          struct cleanup *cleanups =
> -            make_cleanup_ui_file_delete (opcode_stream);
> -
> -          pc += gdbarch_print_insn (gdbarch, pc, di);
> -          for (;old_pc < pc; old_pc++)
> -            {
> -              status = (*di->read_memory_func) (old_pc, &data, 1, di);
> -              if (status != 0)
> -                (*di->memory_error_func) (status, old_pc, di);
> -              fprintf_filtered (opcode_stream, "%s%02x",
> -                                spacer, (unsigned) data);
> -              spacer = " ";
> -            }
> -          ui_out_field_stream (uiout, "opcodes", opcode_stream);
> -          ui_out_text (uiout, "\t");
> -
> -          do_cleanups (cleanups);
> -        }
> -      else
> -        pc += gdbarch_print_insn (gdbarch, pc, di);
> -      ui_out_field_stream (uiout, "inst", stb);
> -      ui_file_rewind (stb);
> -      do_cleanups (ui_out_chain);
> -      ui_out_text (uiout, "\n");
> +      ui_out_field_stream (uiout, "opcodes", opcode_stream);
> +      ui_out_text (uiout, "\t");
> +
> +      do_cleanups (cleanups);
> +    }
> +  else
> +    size = gdbarch_print_insn (gdbarch, pc, di);
> +
> +  ui_out_field_stream (uiout, "inst", stb);
> +  ui_file_rewind (stb);
> +  do_cleanups (ui_out_chain);
> +  ui_out_text (uiout, "\n");
> +
> +  return size;
> +}
> +
> +static int
> +dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> +	    struct disassemble_info * di,
> +	    CORE_ADDR low, CORE_ADDR high,
> +	    int how_many, int flags, struct ui_file *stb,
> +	    CORE_ADDR *end_pc)
> +{
> +  int num_displayed = 0;
> +
> +  while (low < high && (how_many < 0 || num_displayed < how_many))
> +    {
> +      int size;
> +
> +      size = gdb_print_insn_tuple (gdbarch, uiout, di, low, flags, stb);
> +      if (size <= 0)
> +	break;
> +
> +      num_displayed += 1;

I'm guessing the "+= 1" is used to be consistent with the next line.
My impression is that the community prefers ++num_displayed,
but in this particular case I could go with either.

> +      low += size;
> +
> +      /* Allow user to bail out with ^C.  */
> +      QUIT;
>      }
>  
>    if (end_pc != NULL)
> -    *end_pc = pc;
> +    *end_pc = low;
> +
>    return num_displayed;
>  }

Re: [PATCH v2 2/4] disasm: add struct disasm_insn to describe to-be-disassembled instruction

[Doug Evans]

Markus Metzger <markus.t.metzger@intel.com> writes:
> The "record instruction-history" command prints for each instruction in
> addition to the instruction's disassembly:
>
>   - the instruction number in the recorded execution trace
>   - a '?' before the instruction if it was executed speculatively
>
> To allow the "record instruction-history" command to use GDB's disassembly
> infrastructure, we extend gdb_print_insn_tuple to optionally print those
> additional fields and export the function.
>
> Add a new struct disasm_insn to add additional fields describing the
> to-be-disassembled instruction.  The additional fields are:
>
>   number            an optional instruction number, zero if omitted.
>   is_speculative    a predicate saying whether the instruction was
>                     executed speculatively.
>
> If non-zero, the instruction number is printed first.  It will also appear
> as a new optional field "insn-number" in MI.  The field will be present if
> insn_num is non-zero.
>
> If is_speculative is set, speculative execution will be indicated by a "?"
> following the new instruction number field.  Unless the PC is omitted, it
> will overwrite the first byte of the PC prefix.  It will appear as a new
> optional field "is-speculative" in MI.  The field will contain "?" and will
> be present if is_speculative is set.
>
> The speculative execution indication is guarded by a new flag
> DISASSEMBLY_SPECULATION.
>
> Replace the PC parameter of gdb_print_insn_tuple with a pointer to the above
> struct.  GDB's "disassemble" command does not use the new fields.
>
> 2015-10-19  Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> 	* disasm.h (DISASSEMBLY_SPECULATION): New.
> 	(struct disasm_insn): New.
> 	(gdb_print_insn_tuple): New.
> 	* disasm.c (gdb_print_insn_tuple): Replace parameter PC with INSN.
> 	Update users.  Print instruction number and indicate speculative
> 	execution, if requested.
> ---
>  gdb/disasm.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
>  gdb/disasm.h | 23 +++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 4c80c5f..9b3f156 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -170,12 +170,12 @@ compare_lines (const void *mle1p, const void *mle2p)
>    return val;
>  }
>  
> -/* Prints the instruction at PC into UIOUT and returns the length of the
> -   printed instruction in bytes.  */
> +/* See disasm.h.  */
>  
> -static int
> +int
>  gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
> -		      struct disassemble_info * di, CORE_ADDR pc, int flags,
> +		      struct disassemble_info * di,
> +		      const struct disasm_insn *insn, int flags,
>  		      struct ui_file *stb)
>  {
>    /* parts of the symbolic representation of the address */
> @@ -186,10 +186,37 @@ gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,
>    struct cleanup *ui_out_chain;
>    char *filename = NULL;
>    char *name = NULL;
> +  CORE_ADDR pc;
>  
>    ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +  pc = insn->addr;
> +
> +  if (insn->number != 0)
> +    {
> +      ui_out_field_fmt (uiout, "insn-number", "%u", insn->number);
> +      ui_out_text (uiout, "\t");
> +    }
>  
> -  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> +  if ((flags & DISASSEMBLY_SPECULATIVE) != 0)
> +    {
> +      if (insn->is_speculative)
> +	{
> +	  ui_out_field_string (uiout, "is-speculative", "?");
> +
> +	  /* The speculative execution indication overwrites the first
> +	     character of the PC prefix.
> +	     We assume a PC prefix length of 3 characters.  */
> +	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> +	    ui_out_text (uiout, pc_prefix (pc) + 1);
> +	  else
> +	    ui_out_text (uiout, "  ");
> +	}
> +      else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> +	ui_out_text (uiout, pc_prefix (pc));
> +      else
> +	ui_out_text (uiout, "   ");
> +    }
> +  else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
>      ui_out_text (uiout, pc_prefix (pc));
>    ui_out_field_core_addr (uiout, "address", gdbarch, pc);
>  
> @@ -262,25 +289,29 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>  	    int how_many, int flags, struct ui_file *stb,
>  	    CORE_ADDR *end_pc)
>  {
> +  struct disasm_insn insn;
>    int num_displayed = 0;
>  
> -  while (low < high && (how_many < 0 || num_displayed < how_many))
> +  memset (&insn, 0, sizeof (insn));
> +  insn.addr = low;
> +
> +  while (insn.addr < high && (how_many < 0 || num_displayed < how_many))
>      {
>        int size;
>  
> -      size = gdb_print_insn_tuple (gdbarch, uiout, di, low, flags, stb);
> +      size = gdb_print_insn_tuple (gdbarch, uiout, di, &insn, flags, stb);
>        if (size <= 0)
>  	break;
>  
>        num_displayed += 1;
> -      low += size;
> +      insn.addr += size;
>  
>        /* Allow user to bail out with ^C.  */
>        QUIT;
>      }
>  
>    if (end_pc != NULL)
> -    *end_pc = low;
> +    *end_pc = insn.addr;
>  
>    return num_displayed;
>  }
> diff --git a/gdb/disasm.h b/gdb/disasm.h
> index 7e6f1a2..3b7d01a 100644
> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -27,11 +27,34 @@
>  #define DISASSEMBLY_FILENAME	(0x1 << 3)
>  #define DISASSEMBLY_OMIT_PC	(0x1 << 4)
>  #define DISASSEMBLY_SOURCE	(0x1 << 5)
> +#define DISASSEMBLY_SPECULATIVE	(0x1 << 6)
>  
>  struct gdbarch;
>  struct ui_out;
>  struct ui_file;
>  
> +/* An instruction to be disassembled.  */
> +
> +struct disasm_insn
> +{
> +  /* The address of the memory containing the instruction.  */
> +  CORE_ADDR addr;
> +
> +  /* An optional instruction number.  If non-zero, it is printed first.  */
> +  unsigned int number;
> +
> +  /* A bit-field saying whether the instruction was executed speculatively.  */

How about: "True if the instruction was executed speculatively."

> +  unsigned int is_speculative:1;
> +};
> +
> +/* Prints the instruction INSN into UIOUT and returns the length of the
> +   printed instruction in bytes.  */
> +
> +extern int gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,

Still would rather not have "tuple" in the name here.
I know gdb_print_insn is taken.
If one reads their function comments the reader is left thinking
they print essentially the same thing (which is obviously not true).
We need to pick names that distinguish them.
I'm as bad at picking names as anyone, but how about
using gdb_pretty_print_insn here (leaving gdb_print_insn as is) ?

> +				 struct disassemble_info * di,
> +				 const struct disasm_insn *insn, int flags,
> +				 struct ui_file *stb);
> +
>  /* Return a filled in disassemble_info object for use by gdb.  */
>  
>  extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,

RE: [PATCH v2 3/4] btrace: change record instruction-history /m

[Metzger, Markus T]

> -----Original Message-----
> From: Doug Evans [mailto:xdje42@gmail.com]
> Sent: Monday, October 26, 2015 12:10 AM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; palves@redhat.com
> Subject: Re: [PATCH v2 3/4] btrace: change record instruction-history /m

Hi Doug,

Thanks for your review.


> > +/* Print source lines in LINES.  */
> 
> UI_ITEM needs to be documented, use of cleanups like this
> is certainly atypical.
> Plus the name is confusing (sorry!). I read "ui_item" and the last thing
> I think of is it being a cleanup. ui_item_cleanup?
> Or ui_item_chain which would be more consistent with the use in disasm.c.

I changed it to ui_item_chain and documented the way it is used.


> > +  for (line = lines.begin; line < lines.end; ++line)
> > +    {
> > +      if (*ui_item != NULL)
> > +	do_cleanups (*ui_item);
> > +
> > +      *ui_item
> > +	= make_cleanup_ui_out_tuple_begin_end (uiout,
> "src_and_asm_line");
> > +
> > +      print_source_lines (lines.symtab, line, line + 1, psl_flags);
> > +
> > +      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
> 
> I think I understand what's going on here.
> If we're printing multiple source lines that don't have insns
> associated with them we still have to include in the output
> line_asm_insn lists, even if they're empty.
> It's a bit subtle though: the second,third,... times through the
> loop we'll immediately run the cleanup set up by
> make_cleanup_ui_out_list_begin_end, and I was left scratching my
> head for a moment. The last time through the loop
> we'll leave the line_asm_insn list open, so that we can add
> disassembled insns to the list. IWBN to explain this to the reader.

That's exactly how it works.  I added a detailed comment to
btrace_print_lines and a brief comment to btrace_insn_history.

I'm waiting for your feedback on a better name for gdb_print_insn_tuple
before sending out v3.

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, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 2/4] disasm: add struct disasm_insn to describe to-be-disassembled instruction

[Metzger, Markus T]

> -----Original Message-----
> From: Doug Evans [mailto:xdje42@gmail.com]
> Sent: Sunday, October 25, 2015 11:24 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; palves@redhat.com
> Subject: Re: [PATCH v2 2/4] disasm: add struct disasm_insn to describe to-
> be-disassembled instruction

Hi Doug,

Thanks for your review.


> > +/* Prints the instruction INSN into UIOUT and returns the length of the
> > +   printed instruction in bytes.  */
> > +
> > +extern int gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out
> *uiout,
> 
> Still would rather not have "tuple" in the name here.
> I know gdb_print_insn is taken.
> If one reads their function comments the reader is left thinking
> they print essentially the same thing (which is obviously not true).
> We need to pick names that distinguish them.
> I'm as bad at picking names as anyone, but how about
> using gdb_pretty_print_insn here (leaving gdb_print_insn as is) ?

The "tuple" in the name refers to ui_out/MI.  The function prints the ui_out
tuple for one instruction.

I'm also fine to call it gdb_pretty_print_insn or just dump_insn.

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, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 2/4] disasm: add struct disasm_insn to describe to-be-disassembled instruction

[Doug Evans]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:
>> -----Original Message-----
>> From: Doug Evans [mailto:xdje42@gmail.com]
>> Sent: Sunday, October 25, 2015 11:24 PM
>> To: Metzger, Markus T
>> Cc: gdb-patches@sourceware.org; palves@redhat.com
>> Subject: Re: [PATCH v2 2/4] disasm: add struct disasm_insn to describe to-
>> be-disassembled instruction
>
> Hi Doug,
>
> Thanks for your review.
>
>
>> > +/* Prints the instruction INSN into UIOUT and returns the length of the
>> > +   printed instruction in bytes.  */
>> > +
>> > +extern int gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out
>> *uiout,
>> 
>> Still would rather not have "tuple" in the name here.
>> I know gdb_print_insn is taken.
>> If one reads their function comments the reader is left thinking
>> they print essentially the same thing (which is obviously not true).
>> We need to pick names that distinguish them.
>> I'm as bad at picking names as anyone, but how about
>> using gdb_pretty_print_insn here (leaving gdb_print_insn as is) ?
>
> The "tuple" in the name refers to ui_out/MI.  The function prints the ui_out
> tuple for one instruction.

Yeah, but it's also used for the non-MI case,
and when I read "insn_tuple" MI isn't what I think about.

> I'm also fine to call it gdb_pretty_print_insn or just dump_insn.

The convention in disasm.c is to prefix exported routines with gdb_,
which is fine by me, so let's go with gdb_pretty_print_insn.
[Maybe that's just happenstance, but I like it so let's stick with it.]

Re: [PATCH v2 3/4] btrace: change record instruction-history /m

[Doug Evans]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:
> That's exactly how it works.  I added a detailed comment to
> btrace_print_lines and a brief comment to btrace_insn_history.

Thanks.

> I'm waiting for your feedback on a better name for gdb_print_insn_tuple
> before sending out v3.

Done.