Re: [PATCH v2] Fix alignment of disassemble /r

[Leonardo Boquillon <leonardo dot boquillon at tallertechnologies dot com>]

Hi Markus,

On Tue, Apr 5, 2016 at 6:20 AM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:

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

Yes, you're right.

> >  {
> >    /* 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.
>
>

Next version will be fixed.
> > +    {
> > +      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?
>
>

This is a mistake. Next version will be fixed.

> >    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 '('.
>
Ok

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

I'm not sure what you meant with this, that snippet is a backtrace
code. If you mean I should use a iterator, I dont know if there is
something similar to btrace_insn_iterator that I could use for
disassembly.

Thanks


-- 



Leonardo BoquillÃn

Software Engineer


San Lorenzo 47, 3rd Floor, Office 5

CÃrdoba, Argentina


Phone: +54 351 6288940

Skype: lboquillon