TUI disassembly window improvemenmt

Message ID CANacp63m7cPtnWOgRduPgk0U4Wj3UnvpXR1H+x=pThh19V4iGw@mail.gmail.com
State New
Headers show
Series
  • TUI disassembly window improvemenmt
Related show

Commit Message

Mike Frysinger via Gdb-patches June 8, 2021, 4:16 p.m.
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

Comments

Mike Frysinger via Gdb-patches June 8, 2021, 4:21 p.m. | #1
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
Mike Frysinger via Gdb-patches June 9, 2021, 12:33 p.m. | #2
> 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.
Mike Frysinger via Gdb-patches June 9, 2021, 12:56 p.m. | #3
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.
Mike Frysinger via Gdb-patches June 9, 2021, 1:05 p.m. | #4
> 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?
Mike Frysinger via Gdb-patches June 9, 2021, 1:16 p.m. | #5
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?
Mike Frysinger via Gdb-patches June 9, 2021, 1:43 p.m. | #6
> 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.
Mike Frysinger via Gdb-patches June 9, 2021, 4:02 p.m. | #7
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.
Mike Frysinger via Gdb-patches June 9, 2021, 5:14 p.m. | #8
> 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.

Patch

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;