Re: [PATCH] [gdb/tui] Improve asm window

[Tom de Vries <tdevries@suse.de>]

On 8/3/25 17:00, Tom de Vries wrote:
> The TUI asm window is (ignoring borders) currently structured as follows, from
> left to right:
> - exec info string (3 chars)
> - numeric address (hex with 0x prefix)
> - space
> - symbolic address of the form <name> or <name+n> (with n decimal)
> - spaces left-aligning the following item to a multiple of tui tab-width,
>    relative to the start of the numeric address
> - disassembled instruction
> 
> For example, the main function in a hello world exec looks like this:
> ...
> +---------------------------------------------------------------+
> |   0x4005d7 <main>         push   %rbp                         |
> |   0x4005d8 <main+1>       mov    %rsp,%rbp                    |
> |   0x4005db <main+4>       mov    $0x4005fc,%edi               |
> |   0x4005e0 <main+9>       call   0x4004d0 <puts@plt>          |
> |   0x4005e5 <main+14>      mov    $0x0,%eax                    |
> |   0x4005ea <main+19>      pop    %rbp                         |
> |   0x4005eb <main+20>      ret                                 |
> |   0x4005ec <_fini>        sub    $0x8,%rsp                    |
> |   0x4005f0 <_fini+4>      add    $0x8,%rsp                    |
> +---------------------------------------------------------------+
> ...
> 
> A known problem with this approach is that the symbolic address can be very
> long, for instance when dealing with C++ symbols, with the result that the
> disassembled instruction is not visible unless the user scrolls to it.
> 
> And after stepping the cursor position may be reset, again requiring
> scrolling.
> 
> I've tried to address this in the following way.
> 
> I've divided the window into 4 parts:
> - exec info string
> - numeric address
> - symbolic address
> - disassembled instruction
> 
> First, we decide which parts will be shown, depending on the available width.
> 
> In the minimal case, we have:
> - exec info string
> - disassembled instruction
> 
> With a bit more space available, we have:
> - exec info string
> - numeric address
> - disassembled instruction
> 
> And with yet a bit more space available, we have the current layout with all 4
> parts.
> 
> [ Note that this is similar to how the status line is handled. ]
> 
> In the latter case, after reserving space for the exec info string and the
> numeric address, the remaining space is divided between the symbolic address
> and the disassembled instruction.
> 
> If the available space for the symbolic address is insufficient, the symbolic
> address is abbreviated.
> 
> If the available space for the disassembled instruction is insufficient, it's
> truncated at the end, as is the case currently.
> 
> Without this patch, with the example from PR tui/25347, we have:
> ...
> +---------------------------------------------------------------+
> |   0x401697 <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x401698 <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x40169b <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x40169f <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |B+>0x4016a3 <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x4016a7 <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x4016ab <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x4016af <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> |   0x4016b2 <_Z3fooN5boost5units8quantityINS0_4unitINS0_4listIN|
> +---------------------------------------------------------------+
> ...
> and with this patch we have instead:
> ...
> +---------------------------------------------------------------+
> |   0x401697 push   %rbp                                        |
> |   0x401698 mov    %rsp,%rbp                                   |
> |   0x40169b sub    $0x20,%rsp                                  |
> |   0x40169f mov    %rdi,-0x18(%rbp)                            |
> |B+>0x4016a3 lea    -0x8(%rbp),%rax                             |
> |   0x4016a7 mov    -0x18(%rbp),%rdx                            |
> |   0x4016ab mov    -0x18(%rbp),%rcx                            |
> |   0x4016af mov    %rcx,%rsi                                   |
> |   0x4016b2 mov    %rax,%rdi                                   |
> +---------------------------------------------------------------+
> ...
> and after resizing that to a bit more lines and columns:
> ...
> +-----------------------------------------------------------------------------+
> |   0x401697 <_Z3fooN5b...EEEvEEdEE>           push   %rbp                    |
> |   0x401698 <_Z3fooN5b...EEEvEEdEE+1>         mov    %rsp,%rbp               |
> |   0x40169b <_Z3fooN5b...EEEvEEdEE+4>         sub    $0x20,%rsp              |
> |   0x40169f <_Z3fooN5b...EEEvEEdEE+8>         mov    %rdi,-0x18(%rbp)        |
> |B+>0x4016a3 <_Z3fooN5b...EEEvEEdEE+12>        lea    -0x8(%rbp),%rax         |
> |   0x4016a7 <_Z3fooN5b...EEEvEEdEE+16>        mov    -0x18(%rbp),%rdx        |
> |   0x4016ab <_Z3fooN5b...EEEvEEdEE+20>        mov    -0x18(%rbp),%rcx        |
> |   0x4016af <_Z3fooN5b...EEEvEEdEE+24>        mov    %rcx,%rsi               |
> |   0x4016b2 <_Z3fooN5b...EEEvEEdEE+27>        mov    %rax,%rdi               |
> |   0x4016b5 <_Z3fooN5b...EEEvEEdEE+30>        call   0x403bd7 <_ZN5boost5unit|
> |   0x4016ba <_Z3fooN5b...EEEvEEdEE+35>        lea    -0x8(%rbp),%rax         |
> |   0x4016be <_Z3fooN5b...EEEvEEdEE+39>        mov    %rax,%rsi               |
> +-----------------------------------------------------------------------------+
> ...
> 
> The patch uses 2 hardcoded constants:
> - const size_t min_sym_addr_size = 30
> - const size_t min_insn_size = 30
> to choose which parts will be shown.
> 
> The patch also uses the hardcoded 50/50 ratio to divide space between the
> symbolic address and the disassembled instruction parts, in case they're both
> used.
> 

Hmm, I did:
...
$ echo $COLUMNS
94
$ echo $LINES
26
$ gdb -q  build/gdb/gdb -ex "b captured_main_1" -ex "layout asm" -ex run
...
and got:
...
│   0xb75847 <captured_main_1(captured_main_args*)+9> sub    $0x218,%rsp 
                     │
│   0xb7584e <captured_mai..._main_args*)+16>         mov 
%rdi,-0x228(%rbp)               │
...
so clearly there's some bug in the patch.  Both lines are in the same 
function, so either both should abbreviate, or both should not.

Thanks,
- Tom

> Tested on x86_64-linux.
> 
> There was one update required in the TUI testsuite due to this failure:
> ...
> FAIL: gdb.tui/new-layout.exp: \
>    layout=h {{-horizontal asm 1 src 1} 1 status 0 cmd 1} \
>    {{0 0 40 15} {39 0 41 15}} {0x[0-9A-Fa-f]+ <main>.*21.*return 0}: \
>    contents in layout h
> ...
> where using a small asm window now means that the address and the symbolic
> address are no longer there:
> ...
>      0 +--------------------------------------+
>      1 |   push   %rbp                        |
>      2 |   mov    %rsp,%rbp                   |
>      3 |   mov    $0x0,%eax                   |
>      4 |   pop    %rbp                        |
>      5 |   ret                                |
>      6 |                                      |
>      7 |                                      |
>      8 |                                      |
>      9 |                                      |
>     10 |                                      |
>     11 |                                      |
>     12 |                                      |
>     13 |                                      |
>     14 +--------------------------------------+
> ...
> ---
>   gdb/defs.h                           |  3 +
>   gdb/printcmd.c                       | 80 +++++++++++++++++++++++++
>   gdb/testsuite/gdb.tui/new-layout.exp |  2 +-
>   gdb/tui/tui-disasm.c                 | 88 ++++++++++++++++++++++------
>   4 files changed, 153 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/defs.h b/gdb/defs.h
> index bb1e11925da..569bc4f88ac 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -209,6 +209,9 @@ extern int print_address_symbolic (struct gdbarch *, CORE_ADDR,
>   				   struct ui_file *, int,
>   				   const char *);
>   
> +extern int print_address_symbolic_maxlen (struct gdbarch *, CORE_ADDR,
> +					  struct ui_file *, int, int);
> +
>   extern void print_address (struct gdbarch *, CORE_ADDR, struct ui_file *);
>   extern const char *pc_prefix (CORE_ADDR);
>   
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 19fbc20074e..2857ee02ed9 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -56,6 +56,7 @@
>   #include <optional>
>   #include "gdbsupport/gdb-safe-ctype.h"
>   #include "inferior.h"
> +#include <math.h>
>   
>   /* Chain containing all defined memory-tag subcommands.  */
>   
> @@ -591,6 +592,85 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
>     return 1;
>   }
>   
> +/* Return the number of digits required to display COUNT in decimal.  */
> +
> +static int
> +index_digits (unsigned int count)
> +{
> +  return ((int) log10 ((double) count)) + 1;
> +}
> +
> +/* Optionally print address ADDR symbolically as <SYMBOL+OFFSET> on STREAM,
> +   using not more than MAX chars, possibly abbreviating SYMBOL.
> +   Print nothing if no symbolic name is found nearby.
> +   DO_DEMANGLE controls whether to print a symbol in its native "raw" form,
> +   or to interpret it as a possible C++ name and convert it back to source
> +   form.  However note that DO_DEMANGLE can be overridden by the specific
> +   settings of the demangle and asm_demangle variables.  Returns
> +   non-zero if anything was printed; zero otherwise.  */
> +
> +int
> +print_address_symbolic_maxlen (struct gdbarch *gdbarch, CORE_ADDR addr,
> +			       struct ui_file *stream,
> +			       int do_demangle, int max)
> +{
> +  std::string name, filename;
> +  int unmapped = 0;
> +  int offset = 0;
> +  int line = 0;
> +
> +  if (build_address_symbolic (gdbarch, addr, do_demangle, false, &name,
> +			      &offset, &filename, &line, &unmapped))
> +    return 0;
> +
> +  /* Count "<**> or "<>".  */
> +  max -= unmapped ? 4 : 2;
> +  /* Count offset.  */
> +  max -= index_digits (max_symbolic_offset);
> +
> +  bool do_abbrev = false;
> +  if (strlen (name.c_str ()) > max)
> +    {
> +      /* If we don't have at least 3 chars on either size of the "...", we
> +	 consider there's not enough space.  */
> +      if (max < 9)
> +	return 0;
> +
> +      do_abbrev = true;
> +    }
> +
> +  if (unmapped)
> +    gdb_puts ("<*", stream);
> +  else
> +    gdb_puts ("<", stream);
> +
> +  if (do_abbrev)
> +    {
> +      max -= 3;
> +      int start_len = max / 2 + max % 2;
> +      int end_len = max / 2 + max % 2;
> +
> +      fputs_styled (name.substr (0, start_len).c_str (),
> +		    function_name_style.style (), stream);
> +
> +      fputs_styled ("...", function_name_style.style (), stream);
> +
> +      fputs_styled (name.c_str () + strlen (name.c_str ()) - end_len,
> +		    function_name_style.style (), stream);
> +    }
> +  else
> +    fputs_styled (name.c_str (), function_name_style.style (), stream);
> +
> +  if (offset != 0)
> +    gdb_printf (stream, "%+d", offset);
> +  if (unmapped)
> +    gdb_puts ("*>", stream);
> +  else
> +    gdb_puts (">", stream);
> +
> +  return 1;
> +}
> +
>   /* See valprint.h.  */
>   
>   int
> diff --git a/gdb/testsuite/gdb.tui/new-layout.exp b/gdb/testsuite/gdb.tui/new-layout.exp
> index f5179973e5b..355ab2bf80a 100644
> --- a/gdb/testsuite/gdb.tui/new-layout.exp
> +++ b/gdb/testsuite/gdb.tui/new-layout.exp
> @@ -70,7 +70,7 @@ set layouts \
>   	      {{0 0 80 15}} ""] \
>   	 [list h "{-horizontal asm 1 src 1} 1 status 0 cmd 1" \
>   	      {{0 0 40 15} {39 0 41 15}} \
> -	      "$hex <main>.*21.*return 0"] \
> +	      "21.*return 0"] \
>   	 [list example3 "{-horizontal src 1 cmd 1} 1 status 0 asm 1" \
>   	      {{0 0 40 11} {0 12 80 12}} \
>   	      "21.*return 0.*$hex <main>"] \
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index 627f71cb366..85aae4eedea 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -44,6 +44,8 @@ struct tui_asm_line
>     CORE_ADDR addr;
>     std::string addr_string;
>     size_t addr_size;
> +  std::string sym_addr_string;
> +  size_t sym_addr_size;
>     std::string insn;
>   };
>   
> @@ -80,6 +82,11 @@ len_without_escapes (const std::string &str)
>     return len;
>   }
>   
> +extern int
> +print_address_symbolic_maxlen (struct gdbarch *gdbarch, CORE_ADDR addr,
> +			       struct ui_file *stream,
> +			       int do_demangle, int max);
> +
>   /* Function to disassemble up to COUNT instructions starting from address
>      PC into the ASM_LINES vector (which will be emptied of any previous
>      contents).  Return the address of the COUNT'th instruction after pc.
> @@ -129,20 +136,23 @@ tui_disassemble (struct gdbarch *gdbarch,
>   
>         /* And capture the address the instruction is at.  */
>         tal.addr = orig_pc;
> -      print_address (gdbarch, orig_pc, &gdb_dis_out);
> +      fputs_styled (paddress (gdbarch, tal.addr), address_style.style (),
> +		    &gdb_dis_out);
>         tal.addr_string = gdb_dis_out.release ();
> +      tal.addr_size = (term_out
> +		       ? len_without_escapes (tal.addr_string)
> +		       : tal.addr_string.size ());
> +
> +      /* And capture the symbolic address the instruction is at.  */
> +      print_address_symbolic (gdbarch, orig_pc, &gdb_dis_out,
> +			      asm_demangle, "");
> +      tal.sym_addr_string = gdb_dis_out.release ();
> +      tal.sym_addr_size = (term_out
> +			   ? len_without_escapes (tal.sym_addr_string)
> +			   : tal.sym_addr_string.size ());
>   
>         if (addr_size != nullptr)
> -	{
> -	  size_t new_size;
> -
> -	  if (term_out)
> -	    new_size = len_without_escapes (tal.addr_string);
> -	  else
> -	    new_size = tal.addr_string.size ();
> -	  *addr_size = std::max (*addr_size, new_size);
> -	  tal.addr_size = new_size;
> -	}
> +	*addr_size = std::max (*addr_size, tal.addr_size);
>   
>         asm_lines.push_back (std::move (tal));
>       }
> @@ -321,8 +331,6 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
>     int i;
>     int max_lines;
>     CORE_ADDR cur_pc;
> -  int tab_len = tui_tab_width;
> -  int insn_pos;
>   
>     CORE_ADDR pc = sal.pc;
>     if (pc == 0)
> @@ -341,10 +349,26 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
>     size_t addr_size = 0;
>     tui_disassemble (m_gdbarch, asm_lines, pc, max_lines, &addr_size);
>   
> -  /* Align instructions to the same column.  */
> -  insn_pos = (1 + (addr_size / tab_len)) * tab_len;
> +  const size_t min_sym_addr_size = 30;
> +  const size_t min_insn_size = 30;
> +
> +  size_t sym_addr_size = 0;
> +  bool have_addr = false;
> +  bool have_sym_addr = false;
> +  int avail_width = width - box_size () - TUI_EXECINFO_SIZE;
> +  if (avail_width >= addr_size + 1 + min_sym_addr_size + 1 + min_insn_size)
> +    {
> +      have_addr = true;
> +      have_sym_addr = true;
> +      sym_addr_size = (avail_width - addr_size) / 2;
> +    }
> +  else if (avail_width >= addr_size + 1 + min_insn_size)
> +    have_addr = true;
>   
>     /* Now construct each line.  */
> +  bool term_out
> +    = disassembler_styling && gdb_stdout->can_emit_style_escape ();
> +  string_file gdb_dis_out (term_out);
>     m_content.resize (max_lines);
>     m_max_length = -1;
>     for (i = 0; i < max_lines; i++)
> @@ -356,10 +380,36 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
>   
>         if (i < asm_lines.size ())
>   	{
> -	  line
> -	    = (asm_lines[i].addr_string
> -	       + n_spaces (insn_pos - asm_lines[i].addr_size)
> -	       + asm_lines[i].insn);
> +	  if (have_sym_addr && asm_lines[i].sym_addr_size <= sym_addr_size)
> +	    line
> +	      = (asm_lines[i].addr_string
> +		 + " "
> +		 + asm_lines[i].sym_addr_string
> +		 + n_spaces (sym_addr_size - asm_lines[i].sym_addr_size)
> +		 + " "
> +		 + asm_lines[i].insn);
> +	  else if (have_sym_addr)
> +	    {
> +	      print_address_symbolic_maxlen (m_gdbarch, asm_lines[i].addr,
> +					     &gdb_dis_out, asm_demangle,
> +					     sym_addr_size);
> +	      std::string abbrev = gdb_dis_out.release ();
> +	      int abbrev_len = (term_out
> +				? len_without_escapes (abbrev)
> +				: abbrev.size ());
> +	      line
> +		= (asm_lines[i].addr_string
> +		   + " "
> +		   + abbrev
> +		   + n_spaces (sym_addr_size - abbrev_len)
> +		   + " "
> +		   + asm_lines[i].insn);
> +	    }
> +	  else if (have_addr)
> +	    line = asm_lines[i].addr_string + " " + asm_lines[i].insn;
> +	  else
> +	    line = asm_lines[i].insn;
> +
>   	  addr = asm_lines[i].addr;
>   	}
>         else
> 
> base-commit: 891d1654d7314fa520f708dbc5f1bf855d15bd40