Re: [PATCH 4/8] Disassemble blocks with non-contiguous ranges

Simon Marchi
2018-08-01T02:08:52Z

On 2018-06-26 02:49 AM, Kevin Buettner wrote:
> This patch adds support for disassembly of blocks with non-contiguous
> ranges. These blocks are printed as follows:
>
> (gdb) disassemble foo
> Dump of assembler code for function foo:
> Address range 0x401136 to 0x401151:
> 0x0000000000401136 <+0>: push %rbp
> 0x0000000000401137 <+1>: mov %rsp,%rbp
> 0x000000000040113a <+4>: callq 0x401134 <bar>
> 0x000000000040113f <+9>: mov 0x2eef(%rip),%eax # 0x404034 <e>
> 0x0000000000401145 <+15>: test %eax,%eax
> 0x0000000000401147 <+17>: je 0x40114e <foo+24>
> 0x0000000000401149 <+19>: callq 0x401128 <foo+4294967282>
> 0x000000000040114e <+24>: nop
> 0x000000000040114f <+25>: pop %rbp
> 0x0000000000401150 <+26>: retq
> Address range 0x401128 to 0x401134:
> 0x0000000000401128 <+-14>: push %rbp
> 0x0000000000401129 <+-13>: mov %rsp,%rbp
> 0x000000000040112c <+-10>: callq 0x401126 <baz>
> 0x0000000000401131 <+-5>: nop
> 0x0000000000401132 <+-4>: pop %rbp
> 0x0000000000401133 <+-3>: retq
> End of assembler dump.
>
> This is an actual dump from the test case that I constructed for
> this work. The ranges are printed in the order encountered in the
> debug info. For the above example, note that the second range occupies
> lower addresses than the first range.
>
> Functions with contiguous ranges are still printed as follows:
>
> (gdb) disassemble main
> Dump of assembler code for function main:
> 0x0000000000401151 <+0>: push %rbp
> 0x0000000000401152 <+1>: mov %rsp,%rbp
> 0x0000000000401155 <+4>: callq 0x401136 <foo>
> 0x000000000040115a <+9>: mov $0x0,%eax
> 0x000000000040115f <+14>: pop %rbp
> 0x0000000000401160 <+15>: retq
> End of assembler dump.
>
> gdb/ChangeLog:
>
> * cli/cli-cmds.c (block.h): Include.
> (print_disassembly): Handle printing of non-contiguous blocks.
> (disassemble_current_function): Likewise.
> (disassemble_command): Likewise.
> ---
> gdb/cli/cli-cmds.c | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 5c5d6dc..171936c 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -38,6 +38,7 @@
> #include "tracepoint.h"
> #include "filestuff.h"
> #include "location.h"
> +#include "block.h"
>
> #include "ui-out.h"
>
> @@ -1096,6 +1097,7 @@ list_command (const char *arg, int from_tty)
> static void
> print_disassembly (struct gdbarch *gdbarch, const char *name,
> CORE_ADDR low, CORE_ADDR high,
> + const struct block *b,
> gdb_disassembly_flags flags)

Could you document the new parameter?

> {
> #if defined(TUI)
> @@ -1104,14 +1106,28 @@ print_disassembly (struct gdbarch *gdbarch, const char *name,
> {
> printf_filtered ("Dump of assembler code ");
> if (name != NULL)
> - printf_filtered ("for function %s:\n", name);
> - else
> - printf_filtered ("from %s to %s:\n",
> - paddress (gdbarch, low), paddress (gdbarch, high));
> -
> - /* Dump the specified range. */
> - gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
> + printf_filtered ("for function %s:\n", name);
> + if (!b || BLOCK_CONTIGUOUS_P (b))

b == nullptr

> + {
> + if (name == NULL)
> + printf_filtered ("from %s to %s:\n",
> + paddress (gdbarch, low), paddress (gdbarch, high));
>
> + /* Dump the specified range. */
> + gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
> + }
> + else
> + {
> + for (int i = 0; i < BLOCK_NRANGES (b); i++)
> + {
> + CORE_ADDR low = BLOCK_RANGE_START (b, i);
> + CORE_ADDR high = BLOCK_RANGE_END (b, i);
> + printf_filtered ("Address range %s to %s:\n",
> + paddress (gdbarch, low),
> + paddress (gdbarch, high));

Use _() for user-visible strings. You can add it to pre-existing printfs that
your patch touches, if you want.

Otherwise, LGTM.

Simon