[PATCH] TUI disassembly window improvemenmt

[PATCH] TUI disassembly window improvemenmt

[Vasili Burdo]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi,

I implemented a small update to TUI disassembly window - see attachment.
This patch removes function name from disassembly listing leaving only
offset from function start.
The  reason for it - disassembly TUI is almost unusable in case of C++
functions - their names (especially templated ones) are very long and
thus litter disassembly view.

Please, look at sample screenshots here: https://imgur.com/a/GlkVXGi

Thanks,
Vasili

[-- Attachment #2: vburdo-tui-asm.patch --]
[-- Type: text/x-patch, Size: 3638 bytes --]

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 163552aede4..e6ddfcd90cc 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -27,6 +27,7 @@
 #include "value.h"
 #include "source.h"
 #include "disasm.h"
+#include "valprint.h"
 #include "tui/tui.h"
 #include "tui/tui-command.h"
 #include "tui/tui-data.h"
@@ -48,6 +49,7 @@ struct tui_asm_line
 {
   CORE_ADDR addr;
   std::string addr_string;
+  std::string func_string;
   size_t addr_size;
   std::string insn;
 };
@@ -101,7 +103,8 @@ static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
 		 std::vector<tui_asm_line> &asm_lines,
 		 CORE_ADDR pc, int count,
-		 size_t *addr_size = nullptr)
+		 size_t *addr_size = nullptr,
+		 bool for_ui = false)
 {
   bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
   string_file gdb_dis_out (term_out);
@@ -110,7 +113,7 @@ tui_disassemble (struct gdbarch *gdbarch,
   asm_lines.clear ();
 
   /* Now construct each line.  */
-  for (int i = 0; i < count; ++i)
+  while (asm_lines.size() < count)
     {
       tui_asm_line tal;
       CORE_ADDR orig_pc = pc;
@@ -135,8 +138,45 @@ 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);
-      tal.addr_string = std::move (gdb_dis_out.string ());
+      if(for_ui) 
+        {
+          std::string name, filename;
+          int unmapped = 0, offset = 0, line = 0;
+  
+          if (0 == build_address_symbolic (gdbarch, orig_pc, asm_demangle, false,
+                  &name, &offset, &filename, &line, &unmapped))
+            {
+              if (0 == offset)
+                {
+                  tui_asm_line tal2 = tal;
+                  fputs_styled (name.c_str (), function_name_style.style (), &gdb_dis_out);
+                  fputs_filtered (":", &gdb_dis_out);
+                  tal2.func_string = name;
+                  tal2.addr_string = std::move (gdb_dis_out.string ());
+                  tal2.addr_size = 0;
+                  tal2.insn.clear ();
+                  gdb_dis_out.clear ();
+  
+                  asm_lines.push_back (std::move (tal2));
+                }
+  
+              fputs_styled (paddress (gdbarch, orig_pc), address_style.style (), &gdb_dis_out);
+              fputs_filtered (" <", &gdb_dis_out);
+              if (unmapped)
+                fputs_filtered ("*", &gdb_dis_out);
+              fprintf_styled (&gdb_dis_out, address_style.style(), "%+d", offset);
+              if (unmapped)
+                fputs_filtered ("*", &gdb_dis_out);
+              fputs_filtered (">", &gdb_dis_out);
+              tal.addr_string = std::move (gdb_dis_out.string ());
+              tal.func_string = std::move (name);
+            }
+        }
+      else
+        {
+          print_address (gdbarch, orig_pc, &gdb_dis_out);
+          tal.addr_string = std::move (gdb_dis_out.string ());
+        }
       gdb_dis_out.clear ();
 
       if (addr_size != nullptr)
@@ -342,7 +382,12 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
   /* Get temporary table that will hold all strings (addr & insn).  */
   std::vector<tui_asm_line> asm_lines;
   size_t addr_size = 0;
-  tui_disassemble (m_gdbarch, asm_lines, pc, max_lines, &addr_size);
+  tui_disassemble (m_gdbarch, asm_lines, pc, max_lines, &addr_size, true);
+
+  /* Set title to current function name */
+  title.clear();
+  if (asm_lines.size())
+    title = asm_lines.front().func_string;
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;

Re: [PATCH] TUI disassembly window improvemenmt

[Simon Marchi]

On 2021-06-08 12:16 p.m., Vasili Burdo via Gdb-patches wrote:
> Hi,
> 
> I implemented a small update to TUI disassembly window - see attachment.
> This patch removes function name from disassembly listing leaving only
> offset from function start.
> The  reason for it - disassembly TUI is almost unusable in case of C++
> functions - their names (especially templated ones) are very long and
> thus litter disassembly view.
> 
> Please, look at sample screenshots here: https://imgur.com/a/GlkVXGi
> 
> Thanks,
> Vasili

I don't really have time to look at the implementation, but looking at
the screeshots, I think this is a good idea.

Simon

Re: [PATCH] TUI disassembly window improvemenmt

[Eli Zaretskii]

> Date: Tue, 8 Jun 2021 19:16:20 +0300
> From: Vasili Burdo via Gdb-patches <gdb-patches@sourceware.org>
> 
> I implemented a small update to TUI disassembly window - see attachment.
> This patch removes function name from disassembly listing leaving only
> offset from function start.
> The  reason for it - disassembly TUI is almost unusable in case of C++
> functions - their names (especially templated ones) are very long and
> thus litter disassembly view.

Shouldn't this be optional behavior?  Not everyone disassembles only
C++ code.

Re: [PATCH] TUI disassembly window improvemenmt

[Vasili Burdo]

Hi, Eli

ср, 9 июн. 2021 г. в 15:33, Eli Zaretskii <eliz@gnu.org>:
>
> > Date: Tue, 8 Jun 2021 19:16:20 +0300
> > From: Vasili Burdo via Gdb-patches <gdb-patches@sourceware.org>
> >
> > I implemented a small update to TUI disassembly window - see attachment.
> > This patch removes function name from disassembly listing leaving only
> > offset from function start.
> > The  reason for it - disassembly TUI is almost unusable in case of C++
> > functions - their names (especially templated ones) are very long and
> > thus litter disassembly view.
>
> Shouldn't this be optional behavior?  Not everyone disassembles only
> C++ code.
This patch does not limit disassembly view in any way.
- The current  function name moved from disassembly line to window header
- Offset from function start still present in disassembly line
- Function start is marked by function name label.

Hence, I don't see any reason to keep previous view style.
Long function names may exist in any programming language, C++ is just
a language where this problem w/ disassembly view is most annying.

Re: [PATCH] TUI disassembly window improvemenmt

[Eli Zaretskii]

> From: Vasili Burdo <vasili.burdo@gmail.com>
> Date: Wed, 9 Jun 2021 15:56:38 +0300
> Cc: gdb-patches@sourceware.org
> 
> > Shouldn't this be optional behavior?  Not everyone disassembles only
> > C++ code.
> This patch does not limit disassembly view in any way.
> - The current  function name moved from disassembly line to window header
> - Offset from function start still present in disassembly line
> - Function start is marked by function name label.

But AFAIU only the current function's name is visible; if some other
function is referenced in the disassembly, it will now be invisible,
right?

IOW, you seem to assume that any function names mentioned in the
disassembly are always the name of the current function, but is that
an assumption that is always correct?

Re: [PATCH] TUI disassembly window improvemenmt

[Vasili Burdo]

Eli, please take look at screenshots I posted: https://imgur.com/a/GlkVXGi
They show disassembly starting from the same address.

>if some other function is referenced in the disassembly, it will now be invisible, right?
No. Only "address" part for disassembly line is changed. "insn" part
which contains references is intact.

>IOW, you seem to assume that any function names mentioned in the disassembly are always the name of the current function
I didn't make any assumptions of this kind.

ср, 9 июн. 2021 г. в 16:05, Eli Zaretskii <eliz@gnu.org>:
>
> > From: Vasili Burdo <vasili.burdo@gmail.com>
> > Date: Wed, 9 Jun 2021 15:56:38 +0300
> > Cc: gdb-patches@sourceware.org
> >
> > > Shouldn't this be optional behavior?  Not everyone disassembles only
> > > C++ code.
> > This patch does not limit disassembly view in any way.
> > - The current  function name moved from disassembly line to window header
> > - Offset from function start still present in disassembly line
> > - Function start is marked by function name label.
>
> But AFAIU only the current function's name is visible; if some other
> function is referenced in the disassembly, it will now be invisible,
> right?
>
> IOW, you seem to assume that any function names mentioned in the
> disassembly are always the name of the current function, but is that
> an assumption that is always correct?

Re: [PATCH] TUI disassembly window improvemenmt

[Eli Zaretskii]

> From: Vasili Burdo <vasili.burdo@gmail.com>
> Date: Wed, 9 Jun 2021 16:16:08 +0300
> Cc: gdb-patches@sourceware.org
> 
> Eli, please take look at screenshots I posted: https://imgur.com/a/GlkVXGi
> They show disassembly starting from the same address.

My browser shows nothing there.  Can't you attach the screenshots to
you email messages?

> >if some other function is referenced in the disassembly, it will now be invisible, right?
> No. Only "address" part for disassembly line is changed. "insn" part
> which contains references is intact.

Sorry if I misunderstood.

Fwd: [PATCH] TUI disassembly window improvemenmt

[Vasili Burdo]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

Hi, Eli.

Here are screen shots:

ср, 9 июн. 2021 г. в 16:43, Eli Zaretskii <eliz@gnu.org>:
>
> > From: Vasili Burdo <vasili.burdo@gmail.com>
> > Date: Wed, 9 Jun 2021 16:16:08 +0300
> > Cc: gdb-patches@sourceware.org
> >
> > Eli, please take look at screenshots I posted: https://imgur.com/a/GlkVXGi
> > They show disassembly starting from the same address.
>
> My browser shows nothing there.  Can't you attach the screenshots to
> you email messages?
>
> > >if some other function is referenced in the disassembly, it will now be invisible, right?
> > No. Only "address" part for disassembly line is changed. "insn" part
> > which contains references is intact.
>
> Sorry if I misunderstood.

[-- Attachment #2: gdb-asm-new.png --]
[-- Type: image/png, Size: 34411 bytes --]

[-- Attachment #3: gdb-asm-old.png --]
[-- Type: image/png, Size: 55190 bytes --]

Re: [PATCH] TUI disassembly window improvemenmt

[Eli Zaretskii]

> From: Vasili Burdo <vasili.burdo@gmail.com>
> Date: Wed, 9 Jun 2021 16:16:08 +0300
> Cc: gdb-patches@sourceware.org
> 
> Eli, please take look at screenshots I posted: https://imgur.com/a/GlkVXGi
> They show disassembly starting from the same address.

Thanks, now that I've seen the display, I realize that I misunderstood
your original description.  Sorry about that.  I have no issues with
this change.