Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] [gdb/tui] Improve asm window
@ 2025-08-03 15:00 Tom de Vries
  2025-08-03 17:21 ` Tom de Vries
  0 siblings, 1 reply; 3+ messages in thread
From: Tom de Vries @ 2025-08-03 15:00 UTC (permalink / raw)
  To: gdb-patches

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.

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
-- 
2.43.0


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-04 13:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-03 15:00 [PATCH] [gdb/tui] Improve asm window Tom de Vries
2025-08-03 17:21 ` Tom de Vries
2025-08-04 13:20   ` Tom de Vries

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox