[review] Style diassembly in the TUI

Message ID gerrit.1571678978000.I8722635eeecbbb1633d943a65b856404c2d467b0@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 21, 2019, 5:29 p.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................

Style diassembly in the TUI

This patch changes the TUI disassembly window to style its contents.
The styling should be identical to what is seen in the CLI.  This
involved a bit of rearrangement, so that the source and disassembly
windows could share both the copy_source_line utility function, and
the ability to react to changes in "set style enabled".

gdb/ChangeLog
2019-10-21  Tom Tromey  <tom@tromey.com>

	* tui/tui-source.h (struct tui_source_window): Inline
	constructor.  Remove destructor.
	<style_changed, m_observable>: Move to superclass.
	* tui/tui-winsource.h (tui_copy_source_line): Declare.
	(struct tui_source_window_base): Move private members to end.
	<style_changed, m_observable>: Move from tui_source_window.
	* tui/tui-winsource.c (tui_copy_source_line): Move from
	tui-source.c.  Rename from copy_source_line.  Add special handling
	for negative line number.
	(tui_source_window_base::style_changed): Move from
	tui_source_window.
	(tui_source_window_base): Register observer.
	(~tui_source_window_base): New.
	* tui/tui-source.c (copy_source_line): Move to tui-winsource.c;
	rename.
	(tui_source_window::set_contents): Use tui_copy_source_line.
	(tui_source_window::tui_source_window): Move to tui-source.h.
	(tui_source_window::~tui_source_window): Remove.
	(tui_source_window::style_changed): Move to superclass.
	* tui/tui-disasm.c (tui_disassemble): Create string file with
	styling, when possible.
	(tui_disasm_window::set_contents): Use tui_copy_source_line.

Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
---
M gdb/ChangeLog
M gdb/tui/tui-disasm.c
M gdb/tui/tui-source.c
M gdb/tui/tui-source.h
M gdb/tui/tui-winsource.c
M gdb/tui/tui-winsource.h
6 files changed, 157 insertions(+), 122 deletions(-)
  

Comments

Simon Marchi (Code Review) Oct. 21, 2019, 10:16 p.m. UTC | #1
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

I am completely clueless about how the TUI works, but I tried it briefly locally and it works as advertised.

I noticed a small bug, where when I turn the styling on and off, it seems to affect the character count on some lines and shifts the content left and right.  It's easier shown with a video:

https://nova.polymtl.ca/~simark/Peek%202019-10-21%2018-11.webm

I wouldn't consider this a blocker, but I thought it would be good to mention it.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/1//COMMIT_MSG@7 
PS1, Line 7: diassembly
disassembly
  
Simon Marchi (Code Review) Oct. 22, 2019, 1:25 a.m. UTC | #2
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 1:

> Patch Set 1: Code-Review+1
> 
> (1 comment)
> 
> I am completely clueless about how the TUI works, but I tried it briefly locally and it works as advertised.
> 
> I noticed a small bug, where when I turn the styling on and off, it seems to affect the character count on some lines and shifts the content left and right.  It's easier shown with a video:
> 
> https://nova.polymtl.ca/~simark/Peek%202019-10-21%2018-11.webm
> 
> I wouldn't consider this a blocker, but I thought it would be good to mention it.

Good catch, thanks.  I think the problem is that the styling escapes are counted
as characters.

The only straightforward way I see to fix this is to call `print_address` twice --
once with styling, and once without. A more complicated way would be to strip the
escapes from the string before computing the length.

WDTY?
  
Simon Marchi (Code Review) Oct. 22, 2019, 4:27 a.m. UTC | #3
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 1:

> The only straightforward way I see to fix this is to call `print_address` twice --
> once with styling, and once without. A more complicated way would be to strip the
> escapes from the string before computing the length.
> 
> WDTY?

Do we already have a function to compute the length of a string, excluding the escape characters?  I would have thought that we would have needed this before for the colorization work done so far.  If we don't, I am fine to call print_address twice.
  
Simon Marchi (Code Review) Oct. 23, 2019, 2:21 p.m. UTC | #4
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/2//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/2//COMMIT_MSG@7 
PS2, Line 7: Style diassembly in the TUI
I noticed a typo here.  I'll fix this locally, but it seems too
trivial to warrant sending another revision to gerrit.
  
Simon Marchi (Code Review) Oct. 30, 2019, 7:30 p.m. UTC | #5
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/2//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179/2//COMMIT_MSG@7 
PS2, Line 7: 
 2 | Author:     Tom Tromey <tom@tromey.com>
 3 | AuthorDate: 2019-10-21 11:21:14 -0600
 4 | Commit:     Tom Tromey <tom@tromey.com>
 5 | CommitDate: 2019-10-23 07:50:54 -0600
 6 | 
 7 > Style diassembly in the TUI
 8 | 
 9 | This patch changes the TUI disassembly window to style its contents.
10 | The styling should be identical to what is seen in the CLI.  This
11 | involved a bit of rearrangement, so that the source and disassembly
12 | windows could share both the copy_source_line utility function, and

> I noticed a typo here.  I'll fix this locally, but it seems too […]

Actually, I did point it out when reviewing v1.

Perhaps Gerrit doesn't make it clear enough when you have unresolved comments on prior patch versions, so they are easy to miss.  But that's where I usually look to see if there are any unresolved comments across all verions:

https://nova.polymtl.ca/~simark/ss/unresolved.png

Gerrit revisions are cheap, so I don't think it's a problem uploading a new one to fix a typo if you want.
  
Matt Rice Oct. 30, 2019, 9:13 p.m. UTC | #6
On Tue, Oct 22, 2019 at 4:27 AM Simon Marchi (Code Review)
<gerrit@gnutoolchain-gerrit.osci.io> wrote:
>
> Simon Marchi has posted comments on this change.
>
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
> ......................................................................
>
>
> Patch Set 1:
>
> > The only straightforward way I see to fix this is to call `print_address` twice --
> > once with styling, and once without. A more complicated way would be to strip the
> > escapes from the string before computing the length.
> >
> > WDTY?
>
> Do we already have a function to compute the length of a string, excluding the escape characters?  I would have thought that we would have needed this before for the colorization work done so far.  If we don't, I am fine to call print_address twice.
>

I believe there must be something in the extended-prompt code,
which implements \[ \] for excluding escape sequences from the length.

https://sourceware.org/gdb/current/onlinedocs/gdb/gdb_002eprompt.html#gdb_002eprompt
  
Tom Tromey Oct. 30, 2019, 11:11 p.m. UTC | #7
>>>>> "Matt" == Matt Rice <ratmice@gmail.com> writes:

Matt> I believe there must be something in the extended-prompt code,
Matt> which implements \[ \] for excluding escape sequences from the length.

This works in a different way and is kind of readline-specific.

I went ahead and just implemented the necessary function in the 2nd
revision of the patch.

Tom
  
Simon Marchi (Code Review) Nov. 5, 2019, 10:23 p.m. UTC | #8
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/179
......................................................................


Patch Set 3:

I'm going to check these in now.
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b0268e3..b57c699 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@ 
 2019-10-21  Tom Tromey  <tom@tromey.com>
 
+	* tui/tui-source.h (struct tui_source_window): Inline
+	constructor.  Remove destructor.
+	<style_changed, m_observable>: Move to superclass.
+	* tui/tui-winsource.h (tui_copy_source_line): Declare.
+	(struct tui_source_window_base): Move private members to end.
+	<style_changed, m_observable>: Move from tui_source_window.
+	* tui/tui-winsource.c (tui_copy_source_line): Move from
+	tui-source.c.  Rename from copy_source_line.  Add special handling
+	for negative line number.
+	(tui_source_window_base::style_changed): Move from
+	tui_source_window.
+	(tui_source_window_base): Register observer.
+	(~tui_source_window_base): New.
+	* tui/tui-source.c (copy_source_line): Move to tui-winsource.c;
+	rename.
+	(tui_source_window::set_contents): Use tui_copy_source_line.
+	(tui_source_window::tui_source_window): Move to tui-source.h.
+	(tui_source_window::~tui_source_window): Remove.
+	(tui_source_window::style_changed): Move to superclass.
+	* tui/tui-disasm.c (tui_disassemble): Create string file with
+	styling, when possible.
+	(tui_disasm_window::set_contents): Use tui_copy_source_line.
+
+2019-10-21  Tom Tromey  <tom@tromey.com>
+
 	* tui/tui-winsource.h (struct tui_source_element) <line>: Now a
 	std::string.
 	* tui/tui-winsource.c (tui_show_source_line): Update.
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 91c9845..d27ddb8 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -39,6 +39,7 @@ 
 #include "tui/tui-source.h"
 #include "progspace.h"
 #include "objfiles.h"
+#include "cli/cli-style.h"
 
 #include "gdb_curses.h"
 
@@ -57,7 +58,8 @@ 
 		 std::vector<tui_asm_line> &asm_lines,
 		 CORE_ADDR pc, int pos, int count)
 {
-  string_file gdb_dis_out;
+  string_file gdb_dis_out (source_styling
+			   && gdb_stdout->can_emit_style_escape ());
 
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
@@ -215,11 +217,8 @@ 
 		       - asm_lines[i].addr_string.size ())
 	   + asm_lines[i].insn);
 
-      /* Now copy the line taking the offset into account.  */
-      if (line.size () > offset)
-	src->line = line.substr (offset, line_width);
-      else
-	src->line.clear ();
+      const char *ptr = line.c_str ();
+      src->line = tui_copy_source_line (&ptr, -1, offset, line_width);
 
       src->line_or_addr.loa = LOA_ADDRESS;
       src->line_or_addr.u.addr = asm_lines[i].addr;
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index f956645..915f9e3 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -37,90 +37,6 @@ 
 #include "tui/tui-source.h"
 #include "gdb_curses.h"
 
-/* A helper function for tui_set_source_content that extracts some
-   source text from PTR.  LINE_NO is the line number; FIRST_COL is the
-   first column to extract, and LINE_WIDTH is the number of characters
-   to display.  Returns a string holding the desired text.  */
-
-static std::string
-copy_source_line (const char **ptr, int line_no, int first_col,
-		  int line_width)
-{
-  const char *lineptr = *ptr;
-
-  /* Init the line with the line number.  */
-  std::string result = string_printf ("%-6d", line_no);
-  int len = result.size ();
-  len = len - ((len / tui_tab_width) * tui_tab_width);
-  result.append (len, ' ');
-
-  int column = 0;
-  char c;
-  do
-    {
-      int skip_bytes;
-
-      c = *lineptr;
-      if (c == '\033' && skip_ansi_escape (lineptr, &skip_bytes))
-	{
-	  /* We always have to preserve escapes.  */
-	  result.append (lineptr, lineptr + skip_bytes);
-	  lineptr += skip_bytes;
-	  continue;
-	}
-
-      ++lineptr;
-      ++column;
-
-      auto process_tab = [&] ()
-	{
-	  int max_tab_len = tui_tab_width;
-
-	  --column;
-	  for (int j = column % max_tab_len;
-	       j < max_tab_len && column < first_col + line_width;
-	       column++, j++)
-	    if (column >= first_col)
-	      result.push_back (' ');
-	};
-
-      /* We have to process all the text in order to pick up all the
-	 escapes.  */
-      if (column <= first_col || column > first_col + line_width)
-	{
-	  if (c == '\t')
-	    process_tab ();
-	  continue;
-	}
-
-      if (c == '\n' || c == '\r' || c == '\0')
-	{
-	  /* Nothing.  */
-	}
-      else if (c < 040 && c != '\t')
-	{
-	  result.push_back ('^');
-	  result.push_back (c + 0100);
-	}
-      else if (c == 0177)
-	{
-	  result.push_back ('^');
-	  result.push_back ('?');
-	}
-      else if (c == '\t')
-	process_tab ();
-      else
-	result.push_back (c);
-    }
-  while (c != '\0' && c != '\n' && c != '\r');
-
-  if (c == '\r' && *lineptr == '\n')
-    ++lineptr;
-  *ptr = lineptr;
-
-  return result;
-}
-
 /* Function to display source in the source window.  */
 enum tui_status
 tui_source_window::set_contents (struct gdbarch *arch,
@@ -171,8 +87,9 @@ 
 
 	      std::string text;
 	      if (*iter != '\0')
-		text = copy_source_line (&iter, cur_line_no, horizontal_offset,
-					 line_width);
+		text = tui_copy_source_line (&iter, cur_line_no,
+					     horizontal_offset,
+					     line_width);
 
 	      /* Set whether element is the execution point
 		 and whether there is a break point on it.  */
@@ -249,26 +166,6 @@ 
     }
 }
 
-tui_source_window::tui_source_window ()
-  : tui_source_window_base (SRC_WIN)
-{
-  gdb::observers::source_styling_changed.attach
-    (std::bind (&tui_source_window::style_changed, this),
-     m_observable);
-}
-
-tui_source_window::~tui_source_window ()
-{
-  gdb::observers::source_styling_changed.detach (m_observable);
-}
-
-void
-tui_source_window::style_changed ()
-{
-  if (tui_active && is_visible ())
-    refill ();
-}
-
 bool
 tui_source_window::location_matches_p (struct bp_location *loc, int line_no)
 {
diff --git a/gdb/tui/tui-source.h b/gdb/tui/tui-source.h
index 3ef737c..a2b7754 100644
--- a/gdb/tui/tui-source.h
+++ b/gdb/tui/tui-source.h
@@ -31,8 +31,10 @@ 
 
 struct tui_source_window : public tui_source_window_base
 {
-  tui_source_window ();
-  ~tui_source_window ();
+  tui_source_window ()
+    : tui_source_window_base (SRC_WIN)
+  {
+  }
 
   DISABLE_COPY_AND_ASSIGN (tui_source_window);
 
@@ -70,17 +72,12 @@ 
 
 private:
 
-  void style_changed ();
-
   /* Answer whether a particular line number or address is displayed
      in the current source window.  */
   bool line_is_displayed (int line) const;
 
   /* It is the resolved form as returned by symtab_to_fullname.  */
   gdb::unique_xmalloc_ptr<char> m_fullname;
-
-  /* A token used to register and unregister an observer.  */
-  gdb::observers::token m_observable;
 };
 
 #endif /* TUI_TUI_SOURCE_H */
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 5d0bcb4..3ca723c 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -65,7 +65,98 @@ 
     }
 }
 
+/* See tui-winsource.h.  */
 
+std::string
+tui_copy_source_line (const char **ptr, int line_no, int first_col,
+		      int line_width)
+{
+  const char *lineptr = *ptr;
+
+  /* Init the line with the line number.  */
+  std::string result;
+
+  if (line_no > 0)
+    {
+      result = string_printf ("%-6d", line_no);
+      int len = result.size ();
+      len = len - ((len / tui_tab_width) * tui_tab_width);
+      result.append (len, ' ');
+    }
+
+  int column = 0;
+  char c;
+  do
+    {
+      int skip_bytes;
+
+      c = *lineptr;
+      if (c == '\033' && skip_ansi_escape (lineptr, &skip_bytes))
+	{
+	  /* We always have to preserve escapes.  */
+	  result.append (lineptr, lineptr + skip_bytes);
+	  lineptr += skip_bytes;
+	  continue;
+	}
+
+      ++lineptr;
+      ++column;
+
+      auto process_tab = [&] ()
+	{
+	  int max_tab_len = tui_tab_width;
+
+	  --column;
+	  for (int j = column % max_tab_len;
+	       j < max_tab_len && column < first_col + line_width;
+	       column++, j++)
+	    if (column >= first_col)
+	      result.push_back (' ');
+	};
+
+      /* We have to process all the text in order to pick up all the
+	 escapes.  */
+      if (column <= first_col || column > first_col + line_width)
+	{
+	  if (c == '\t')
+	    process_tab ();
+	  continue;
+	}
+
+      if (c == '\n' || c == '\r' || c == '\0')
+	{
+	  /* Nothing.  */
+	}
+      else if (c < 040 && c != '\t')
+	{
+	  result.push_back ('^');
+	  result.push_back (c + 0100);
+	}
+      else if (c == 0177)
+	{
+	  result.push_back ('^');
+	  result.push_back ('?');
+	}
+      else if (c == '\t')
+	process_tab ();
+      else
+	result.push_back (c);
+    }
+  while (c != '\0' && c != '\n' && c != '\r');
+
+  if (c == '\r' && *lineptr == '\n')
+    ++lineptr;
+  *ptr = lineptr;
+
+  return result;
+}
+
+void
+tui_source_window_base::style_changed ()
+{
+  if (tui_active && is_visible ())
+    refill ();
+}
 
 /* Function to display source in the source window.  This function
    initializes the horizontal scroll to 0.  */
@@ -253,8 +344,16 @@ 
   gdb_assert (type == SRC_WIN || type == DISASSEM_WIN);
   start_line_or_addr.loa = LOA_ADDRESS;
   start_line_or_addr.u.addr = 0;
+
+  gdb::observers::source_styling_changed.attach
+    (std::bind (&tui_source_window::style_changed, this),
+     m_observable);
 }
 
+tui_source_window_base::~tui_source_window_base ()
+{
+  gdb::observers::source_styling_changed.detach (m_observable);
+}
 
 /* See tui-data.h.  */
 
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index 185d3dd..7c3c626 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -74,11 +74,9 @@ 
 
 struct tui_source_window_base : public tui_win_info
 {
-private:
-  void show_source_content ();
-
 protected:
   explicit tui_source_window_base (enum tui_win_type type);
+  ~tui_source_window_base ();
 
   DISABLE_COPY_AND_ASSIGN (tui_source_window_base);
 
@@ -140,6 +138,16 @@ 
   struct gdbarch *gdbarch = nullptr;
 
   std::vector<tui_source_element> content;
+
+private:
+
+  void show_source_content ();
+
+  /* Called when the user "set style enabled" setting is changed.  */
+  void style_changed ();
+
+  /* A token used to register and unregister an observer.  */
+  gdb::observers::token m_observable;
 };
 
 
@@ -227,6 +235,16 @@ 
 extern void tui_update_source_windows_with_line (struct symtab *, 
 						 int);
 
+/* Extract some source text from PTR.  LINE_NO is the line number.  If
+   it is positive, it is printed at the start of the line.  FIRST_COL
+   is the first column to extract, and LINE_WIDTH is the number of
+   characters to display.  Returns a string holding the desired text.
+   PTR is updated to point to the start of the next line.  */
+
+extern std::string tui_copy_source_line (const char **ptr,
+					 int line_no, int first_col,
+					 int line_width);
+
 /* Constant definitions. */
 #define SCROLL_THRESHOLD 2	/* Threshold for lazy scroll.  */