RE: [PATCH v3] Fix alignment of disassemble /r

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Leonardo Boquillon
> Sent: Thursday, April 7, 2016 11:59 PM
> To: gdb-patches@sourceware.org; daniel.gutson@tallertechnologies.com;
> dje@google.com; Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH v3] Fix alignment of disassemble /r
> 
> When disassembling in raw mode (/r) in a variable-length insn architecture (i.e.
> x86),
> the output can be messed since no alignment takes place.
> 
> The first version of this was sent on this mail thread
> https://sourceware.org/ml/gdb-patches/2014-04/msg00226.html and this patch
> is
> the same placed previously here
> https://sourceware.org/bugzilla/show_bug.cgi?id=19768
> 
> This patch performs the two passes: the first is at get_insn_set_longest_opcode
> at line 136 in the patch, and the second is the while loop that follows like was
> agreed here https://sourceware.org/ml/gdb-patches/2014-04/msg00427.html
> 
> In this version have been added the capability to compute longest opcode for
> btrace.

Thanks.  I tested the btrace part with this:

diff --git a/gdb/testsuite/gdb.btrace/instruction_history.exp b/gdb/testsuite/gdb.btrace/instruction_h
index 58ae770..b361e32 100644
--- a/gdb/testsuite/gdb.btrace/instruction_history.exp
+++ b/gdb/testsuite/gdb.btrace/instruction_history.exp
@@ -97,6 +97,18 @@ gdb_test "record instruction-history /pf 3,7" [multi_line \
   "7\t   0x\[0-9a-f\]+ <\\+\[0-9\]+>:\tje     0x\[0-9a-f\]+ <loop\\+\[0-9\]+>\r" \
   ]
 
+# We don't know the exact encoding that is used but we know that the raw
+# opcode bytes should be padded so they should all be the same length.
+#
+# To simplify things let's assume that the longest opcode is 3B.
+gdb_test "record instruction-history /r 3,7" [multi_line \
+  "3\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\t\[0-9a-f \]{8}   je     0x\[0-9a-f\]+ <loop\\+\[0-9\]+>" \
+  "4\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\t\[0-9a-f \]{8}   dec    %eax" \
+  "5\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\t\[0-9a-f \]{8}   jmp    0x\[0-9a-f\]+ <loop\\+\[0-9\]+>" \
+  "6\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\t\[0-9a-f \]{8}   cmp    \\\$0x0,%eax" \
+  "7\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\t\[0-9a-f \]{8}   je     0x\[0-9a-f\]+ <loop\\+\[0-9\]+>\r"
+  ]
+
 gdb_test "record instruction-history 3,3" "3\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tje     0x\[0-9a-f\
 
 # the following tests are checking the iterators

You might want to add such a test to your patch, as well.


> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 77b5180..84d9d57 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -660,6 +660,29 @@ btrace_print_lines (struct btrace_line_range lines, struct
> ui_out *uiout,
>      }
>  }
> 
> +static unsigned int
> +btrace_get_longest_opcode(struct gdbarch *gdbarch,
> +			  const struct btrace_insn_iterator *begin,
> +			  const struct btrace_insn_iterator *end)

Please add a short comment for the function.


> +{
> +  unsigned int longest_opcode = 0;
> +  struct btrace_insn_iterator it;
> +
> +  for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
> +    {
> +      const struct btrace_insn *insn = btrace_insn_get (&it);
> +
> +      if (insn != NULL)
> +	{
> +	  const unsigned int current_length =
> +	    gdb_insn_length(gdbarch, insn->pc);

Note that gdb_insn_length returns (signed) int.  I'd use int throughout for the
opcode size.

There's a space between function name and '('.


> +	  longest_opcode =
> +	    (current_length > longest_opcode) ? current_length : longest_opcode;
> +	}
> +    }
> +
> +  return longest_opcode;
> +}
>  /* Disassemble a section of the recorded instruction trace.  */
> 
>  static void
> @@ -674,6 +697,7 @@ btrace_insn_history (struct ui_out *uiout,
>    struct gdbarch *gdbarch;
>    struct btrace_insn_iterator it;
>    struct btrace_line_range last_lines;
> +  unsigned int longest_opcode;
> 
>    DEBUG ("itrace (0x%x): [%u; %u)", flags, btrace_insn_number (begin),
>  	 btrace_insn_number (end));
> @@ -692,6 +716,7 @@ btrace_insn_history (struct ui_out *uiout,
>       instructions corresponding to that line.  */
>    ui_item_chain = NULL;
> 
> +  longest_opcode = btrace_get_longest_opcode(gdbarch, begin, end);

There's a space between function name and '('.


>    for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
>      {
>        const struct btrace_insn *insn;
> @@ -745,7 +770,8 @@ btrace_insn_history (struct ui_out *uiout,
>  	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
>  	    dinsn.is_speculative = 1;
> 
> -	  gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb);
> +	  gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb,
> +				 longest_opcode);
>  	}
>      }

The btrace hunks are OK otherwise.

The signed vs. unsigned int and the formatting comments apply to the other
changes, as well.

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