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

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

On 8/3/25 19:21, Tom de Vries wrote:
> 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.
> 

I've fixed this in a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2025-August/219701.html ).

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
>