[v2] Fix alignment of disassemble /r
Commit Message
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
If this is ok for commit I have a company-wide copyright access, and a coworker
of mine has write access.
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.
---
gdb/disasm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++---------
gdb/disasm.h | 20 ++++++++++---
gdb/record-btrace.c | 2 +-
3 files changed, 85 insertions(+), 19 deletions(-)
Comments
> -----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
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
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Leonardo Boquillon
> Sent: Tuesday, April 5, 2016 8:11 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2] Fix alignment of disassemble /r
Hi Leonardo,
> > > 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.
I was just referring to the record-btrace changes. The patch uses
gdb_pretty_print_insn_tab here. We may instead compute the longest
opcode and use gdb_pretty_print_insn_padding here, as well. Just as the
patch does for disasm.
Record-btrace uses an iterator instead of a low-high address pair. I sketched the
loop to compute the longest opcode in this case.
We wouldn't need gdb_pretty_print_insn_tab anymore as all users now use
gdb_pretty_print_insn_padding.
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
Hi Markus,
I sketched a function to compute the opcode length on record-btrace [1] but
I'm not sure how to test it. Could you please give me a hint about how to?
Can be it tested over x86?
[1] =
static size_t
get_insn_set_longest_opcode(struct gdbarch *gdbarch, struct
disassemble_info* di,
const struct btrace_insn_iterator *begin,
const struct btrace_insn_iterator *end)
{
size_t 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 int current_length = gdbarch_print_insn(gdbarch, insn->pc, di);
longest_opcode =
(current_length > longest_opcode) ? current_length : longest_opcode;
}
}
return longest_opcode;
}
Best Regards
On Wed, Apr 6, 2016 at 3:56 AM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Leonardo Boquillon
>> Sent: Tuesday, April 5, 2016 8:11 PM
>> To: Metzger, Markus T <markus.t.metzger@intel.com>
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH v2] Fix alignment of disassemble /r
>
> Hi Leonardo,
>
>> > > 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.
>
> I was just referring to the record-btrace changes. The patch uses
> gdb_pretty_print_insn_tab here. We may instead compute the longest
> opcode and use gdb_pretty_print_insn_padding here, as well. Just as the
> patch does for disasm.
>
> Record-btrace uses an iterator instead of a low-high address pair. I sketched the
> loop to compute the longest opcode in this case.
>
> We wouldn't need gdb_pretty_print_insn_tab anymore as all users now use
> gdb_pretty_print_insn_padding.
>
> 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
> -----Original Message-----
> From: Leonardo Boquillon
> [mailto:leonardo.boquillon@tallertechnologies.com]
> Sent: Wednesday, April 6, 2016 7:47 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2] Fix alignment of disassemble /r
>
> Hi Markus,
> I sketched a function to compute the opcode length on record-btrace [1] but
> I'm not sure how to test it. Could you please give me a hint about how to?
> Can be it tested over x86?
Hi Leonardo,
Thanks for adding this.
For testing, you'd need a 4th Generation Core (Haswell) or later or any Atom
processor. The code is used by the "record instruction-history" command
which requires the "btrace" recording method; use "record btrace" to start
recording. There are tests in gdb/testsuite/gdb.btrace that might serve as
template.
If you don't have the necessary hardware for testing I can do this for you.
> static size_t
> get_insn_set_longest_opcode(struct gdbarch *gdbarch, struct
> disassemble_info* di,
> const struct btrace_insn_iterator *begin,
> const struct btrace_insn_iterator *end)
Indentation is off.
> {
> size_t longest_opcode = 0;
How about int instead of size_t to avoid conversions below?
> 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 int current_length = gdbarch_print_insn(gdbarch, insn->pc, di);
I'd use gdb_insn_length here.
> longest_opcode =
> (current_length > longest_opcode) ? current_length : longest_opcode;
> }
> }
>
> return longest_opcode;
> }
Looks good. This should also simplify your patch as we won't need two
versions of gdb_pretty_print_insn anymore.
Looking forward to the next version of your patch.
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
@@ -169,25 +169,22 @@ compare_lines (const void *mle1p, const void *mle2p)
return val;
}
-/* See disasm.h. */
-
-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)
{
/* 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;
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))
+ {
+ const int current_length = gdbarch_print_insn(gdbarch, addr, di);
+ longest_opcode =
+ (current_length > longest_opcode) ? current_length : longest_opcode;
+ 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);
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, 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;
@@ -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. */
@@ -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);
}
}