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

[Doug Evans <xdje42 at gmail dot com>]

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,