[PATCH] TUI disassembly window improvement - patch inline

[PATCH] TUI disassembly window improvement - patch inline

[Vasili Burdo]

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
-------------------
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 improvement - patch inline

[Tom Tromey]

>>>>> "Vasili" == Vasili Burdo via Gdb-patches <gdb-patches@sourceware.org> writes:

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

Hi.  Thank you for your patch.

I like the idea, I think it's a nice improvement.

Currently, patches require a ChangeLog entry.
See https://sourceware.org/gdb/wiki/ContributionChecklist

Also, if you do not have a copyright assignment, then you will probably
need one.  You can email me off-list to get started.  We can't check in
a non-trivial patch without one.

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

I appreciate this, but I think it would be better if the commit message
didn't reference a URL like this.  Perhaps a description would be
better.

This is "PR tui/25347" (see
https://sourceware.org/bugzilla/show_bug.cgi?id=25347) so the commit
message ought to include that text.  This will ensure the commit is
mentioned in the bug.

Vasili> @@ -110,7 +113,7 @@ tui_disassemble (struct gdbarch *gdbarch,
Vasili>    asm_lines.clear ();

Vasili>    /* Now construct each line.  */
Vasili> -  for (int i = 0; i < count; ++i)
Vasili> +  while (asm_lines.size() < count)

gdb style is a space before an open paren.

Vasili> +      if(for_ui)

I didn't really understand why this function needed two modes.
I think it would be good to update the introductory comment before
tui_disassemble to explain the meaning of the new parameter.

Vasili> +          if (0 == build_address_symbolic (gdbarch, orig_pc,
Vasili> asm_demangle, false,

Your mailer seems to have mangled the patch.
This will make it harder to eventually apply it.

Vasili> +  /* Set title to current function name */
Vasili> +  title.clear();
Vasili> +  if (asm_lines.size())
Vasili> +    title = asm_lines.front().func_string;

Probably you can check '!asm_lines.empty ()' instead.

I wonder if the title setting is correct here.  What if the window
contents end up like:

    fn1:
      asm
      asm
      asm
    fn2:
      asm
      PC => asm
      asm

In this scenario, would the title be "fn1" or "fn2"? 

Also, which is better?  On the one hand, if the title just shows the
function at the top of the window, then scrolling down will work nicely
in a way -- when a function name scrolls off the top, it will stick in
the title.  On the other hand, I might expect the title to display the
function holding the PC.  I'm really not sure, your input is
appreciated.

I didn't try the patch, but it would be good to report how you tested
it.  I don't recall offhand if there are TUI tests that would need to be
updated.  When modifying the TUI like this, it's usually fine to just
re-run the gdb.tui tests.  If you're unfamiliar with dejagnu, just ask
and I can give you a recipe.

thanks again,
Tom

Re: [PATCH] TUI disassembly window improvement - patch inline

[Vasili Burdo]

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

Hi, Tom

>Also, if you do not have a copyright assignment
I don't. What should I do to get it?

>Currently, patches require a ChangeLog entry.
All your patch-related comments are addressed.
Please review the patch in attachment.

>This is "PR tui/25347"
I marked my ChangeLog entry with this PR.

>Your mailer seems to have mangled the patch.
Now, I put the patch into the attachment.

>I wonder if the title setting is correct here.
Please, look at the screenshot in attachment.

The screen now looks like this:
+--funcaname2--------- <-- function name being executed, otherwise empty.
|   funcname: <--- always on the first line.
|   address <+255> insn <-- line format <address>
<+offset-from-the-func-start> <insn>
|   address <+265> insn
|   address <+270> insn
|   funcaname2:
|   address  <+0>  insn
| > [address <+5>  insn] <-- the line being executed.
|   address <+10>  insn
+----------------------

Thanks, Vasili

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

[-- Attachment #3: vburdo-tui-asm.patch --]
[-- Type: application/x-patch, Size: 5842 bytes --]