RE: [PATCH v2] 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: Friday, April 1, 2016 8:18 PM
> To: gdb-patches@sourceware.org; dje@google.com;
> daniel.gutson@tallertechnologies.com
> Subject: [PATCH v2] Fix alignment of disassemble /r

Hi Leonardo,


> 2016-04-01  Leonrado Boquillon
> <leonardo.boquillon@tallertechnologies.com>
> 
> 	* gdb/disasm.c (gdb_pretty_print_insn): Made it static and
> refactored.
> 	(dump_insns): Add calls to calculate longest opcode, then pass it to
> the
> 	print function.
> 	(get_insn_set_longest_opcode): New function.
> 	(gdb_print_clean): New function.
> 	(gdb_pretty_print_insn_tab): New function.
> 	(gdb_pretty_print_insn_padding): New function.
> 	* gdb/disasm.h (gdb_pretty_print_insn_tab): Declare.
> 	(gdb_pretty_print_insn_padding): Declare.
> 	* gdb/record-btrace.c (btrace_insn_history): Use
> 	gdb_pretty_print_insn_tab for printing.

Is there a corresponding test for the changes?


> -int
> +static int
>  gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
> -		       struct disassemble_info * di,
> -		       const struct disasm_insn *insn, int flags,
> -		       struct ui_file *stb)
> +                       struct disassemble_info * di,
> +                       const struct disasm_insn *insn, int flags,
> +                       struct ui_file *stb, struct cleanup **ui_out_chain)

Looks like the indentation is off.  In several places.


>  {
>    /* parts of the symbolic representation of the address */
>    int unmapped;
>    int offset;
>    int line;
>    int size;
> -  struct cleanup *ui_out_chain;

Why can't we leave the cleanup in gdb_pretty_print_insn?


>    char *filename = NULL;
>    char *name = NULL;
>    CORE_ADDR pc;
> 
> -  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +  *ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
>    pc = insn->addr;
> 
>    if (insn->number != 0)
> @@ -240,6 +237,7 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch,
> struct ui_out *uiout,
>      xfree (name);
> 
>    ui_file_rewind (stb);
> +  size = gdbarch_print_insn (gdbarch, pc, di);
>    if (flags & DISASSEMBLY_RAW_INSN)
>      {
>        CORE_ADDR end_pc;
> @@ -253,7 +251,6 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch,
> struct ui_out *uiout,
>        struct cleanup *cleanups =
>  	make_cleanup_ui_file_delete (opcode_stream);
> 
> -      size = gdbarch_print_insn (gdbarch, pc, di);
>        end_pc = pc + size;
> 
>        for (;pc < end_pc; ++pc)
> @@ -267,18 +264,72 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch,
> struct ui_out *uiout,
>  	}
> 
>        ui_out_field_stream (uiout, "opcodes", opcode_stream);
> -      ui_out_text (uiout, "\t");
> +    }
> +  return size;
> +}
> +
> +static size_t
> +get_insn_set_longest_opcode(struct gdbarch *gdbarch, struct
> disassemble_info *di,
> +                            CORE_ADDR addr, CORE_ADDR high, int how_many)
> +{
> +  size_t longest_opcode = 0;
> +  size_t num_displayed = 0;
> 
> -      do_cleanups (cleanups);
> +  while (addr < high && (how_many < 0 || num_displayed < how_many))

This compares int (signed) with size_t (unsigned).  May be safer to stay with int.


> +    {
> +      const int current_length = gdbarch_print_insn(gdbarch, addr, di);
> +      longest_opcode =
> +        (current_length > longest_opcode) ? current_length : longest_opcode;

Same here.


> +      addr += current_length;
> +      ++num_displayed;
>      }
> -  else
> -    size = gdbarch_print_insn (gdbarch, pc, di);
> 
> +  return longest_opcode;
> +}
> +
> +static void
> +gdb_print_clean (struct ui_out *uiout, struct ui_file *stb,
> +                 struct cleanup *ui_out_chain)
> +{
> +  do_cleanups (ui_out_chain);
>    ui_out_field_stream (uiout, "inst", stb);
>    ui_file_rewind (stb);
>    do_cleanups (ui_out_chain);

Why do_cleanups twice?


>    ui_out_text (uiout, "\n");
> +}
> 
> +/* See disasm.h.  */
> +
> +int gdb_pretty_print_insn_tab (struct gdbarch *gdbarch, struct ui_out
> *uiout,
> +                                  struct disassemble_info *di,
> +                                  const struct disasm_insn *insn, int flags,
> +                                  struct ui_file *stb)
> +{
> +  struct cleanup *ui_out_chain = NULL;
> +  const int size = gdb_pretty_print_insn(gdbarch, uiout, di, insn, flags, stb,
> +                                         &ui_out_chain);
> +  ui_out_text (uiout, "\t");
> +  gdb_print_clean(uiout, stb, ui_out_chain);
> +  return size;
> +}
> +
> +int gdb_pretty_print_insn_padding (struct gdbarch *gdbarch,
> +                                   struct ui_out *uiout,
> +                                   struct disassemble_info *di,
> +                                   const struct disasm_insn *insn, int flags,
> +                                   struct ui_file *stb, size_t longest_opcode)
> +{
> +  struct cleanup *ui_out_chain = NULL;
> +  const int size = gdb_pretty_print_insn(gdbarch, uiout, di, insn, flags, stb,
> +                                         &ui_out_chain);
> +  size_t i;
> +  size_t max_print_space;
> +  gdb_assert(longest_opcode >= size);
> +  max_print_space = 3u * (1 + longest_opcode - size);
> +  for (i = 0; i < max_print_space; ++i)
> +    ui_out_text (uiout, " ");
> +
> +  gdb_print_clean(uiout, stb, ui_out_chain);
>    return size;
>  }
> 
> @@ -291,15 +342,18 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out
> *uiout,
>  {
>    struct disasm_insn insn;
>    int num_displayed = 0;
> +  size_t longest_opcode;
> 
>    memset (&insn, 0, sizeof (insn));
>    insn.addr = low;
> 
> +  longest_opcode = get_insn_set_longest_opcode(gdbarch, di, low, high,

Space between function name and '('.


> how_many);
>    while (insn.addr < high && (how_many < 0 || num_displayed <
> how_many))
>      {
>        int size;
> 
> -      size = gdb_pretty_print_insn (gdbarch, uiout, di, &insn, flags, stb);
> +      size = gdb_pretty_print_insn_padding (gdbarch, uiout, di, &insn, flags,
> +                                            stb, longest_opcode);
>        if (size <= 0)
>  	break;
> 
> diff --git a/gdb/disasm.h b/gdb/disasm.h
> index a2b72b9..370036a 100644
> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -50,10 +50,22 @@ struct disasm_insn
>  /* Prints the instruction INSN into UIOUT and returns the length of the
>     printed instruction in bytes.  */
> 
> -extern int gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out
> *uiout,
> -				  struct disassemble_info * di,
> -				  const struct disasm_insn *insn, int flags,
> -				  struct ui_file *stb);
> +extern int gdb_pretty_print_insn_tab (struct gdbarch *gdbarch,
> +                                      struct ui_out *uiout,
> +				      struct disassemble_info *di,
> +				      const struct disasm_insn *insn, int flags,
> +                                      struct ui_file *stb);
> +
> +/* Prints the instruction INSN aligned according opcode length into UIOUT
> and
> +   returns the length of the printed instruction in bytes.  */
> +
> +
> +extern int gdb_pretty_print_insn_padding (struct gdbarch *gdbarch,
> +                                          struct ui_out *uiout,
> +                                          struct disassemble_info *di,
> +                                          const struct disasm_insn *insn,
> +                                          int flags, struct ui_file *stb,
> +                                          size_t longest_opcode);
> 
>  /* Return a filled in disassemble_info object for use by gdb.  */
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 77b5180..72a0b77 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -745,7 +745,7 @@ 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_tab (gdbarch, uiout, &di, &dinsn, flags, stb);

You can compute the longest opcode just like in the low, high case by iterating
over the to-be-printed instructions like this:

  for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
    {
      const struct btrace_insn *insn;

      insn = btrace_insn_get (&it);
      if (insn == NULL)
        continue;

      <compute longest opcode>

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