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

[Simon Marchi <simon.marchi@ericsson.com>]

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