[review] Style diassembly in the TUI

[review] Style diassembly in the TUI

[Tom Tromey (Code Review)]

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(-)



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.  */
 

[review] Style diassembly in the TUI

[Simon Marchi (Code Review)]

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

[review] Style diassembly in the TUI

[Tom Tromey (Code Review)]

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?

[review] Style diassembly in the TUI

[Simon Marchi (Code Review)]

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.

[review v2] Style diassembly in the TUI

[Tom Tromey (Code Review)]

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".

This version introduces a new function to strip the styling from the
address string when computing the length.  As a byproduct, it also
removes the unused "insn_size" computation from
tui_disasm_window::set_contents.

gdb/ChangeLog
2019-10-23  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.  Add "addr_size" parameter.
	(tui_disasm_window::set_contents): Use tui_copy_source_line.
	Don't compute maximum size.
	(len_without_escapes): New function

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, 205 insertions(+), 142 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index edc9171..214dfef 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,32 @@
 2019-10-23  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.  Add "addr_size" parameter.
+	(tui_disasm_window::set_contents): Use tui_copy_source_line.
+	Don't compute maximum size.
+	(len_without_escapes): New function
+
+2019-10-23  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..7178326 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"
 
@@ -49,15 +50,47 @@
   std::string insn;
 };
 
+/* Helper function to find the number of characters in STR, skipping
+   any ANSI escape sequences.  */
+static size_t
+len_without_escapes (const std::string &str)
+{
+  size_t len = 0;
+  const char *ptr = str.c_str ();
+  char c;
+
+  while ((c = *ptr++) != '\0')
+    {
+      if (c == '\033')
+	{
+	  ui_file_style style;
+	  size_t n_read;
+	  if (style.parse (ptr, &n_read))
+	    ptr += n_read;
+	  else
+	    {
+	      /* Shouldn't happen, but just skip the ESC if it somehow
+		 does.  */
+	      ++ptr;
+	    }
+	}
+      else
+	++len;
+    }
+  return len;
+}
+
 /* Function to set the disassembly window's content.
    Disassemble count lines starting at pc.
    Return address of the count'th instruction after pc.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
 		 std::vector<tui_asm_line> &asm_lines,
-		 CORE_ADDR pc, int pos, int count)
+		 CORE_ADDR pc, int pos, int count,
+		 size_t *addr_size = nullptr)
 {
-  string_file gdb_dis_out;
+  bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
+  string_file gdb_dis_out (term_out);
 
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
@@ -68,6 +101,17 @@
 
       gdb_dis_out.clear ();
 
+      if (addr_size != nullptr)
+	{
+	  size_t new_size;
+
+	  if (term_out)
+	    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+	  else
+	    new_size = asm_lines[pos + i].addr_string.size ();
+	  *addr_size = std::max (*addr_size, new_size);
+	}
+
       pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
 
       asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
@@ -164,8 +208,7 @@
   struct tui_locator_window *locator = tui_locator_win_info_ptr ();
   int tab_len = tui_tab_width;
   int insn_pos;
-  int addr_size, insn_size;
-  
+
   gdb_assert (line_or_addr.loa == LOA_ADDRESS);
   CORE_ADDR pc = line_or_addr.u.addr;
   if (pc == 0)
@@ -182,23 +225,8 @@
 
   /* Get temporary table that will hold all strings (addr & insn).  */
   std::vector<tui_asm_line> asm_lines (max_lines);
-
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-
-  /* Determine maximum address- and instruction lengths.  */
-  addr_size = 0;
-  insn_size = 0;
-  for (i = 0; i < max_lines; i++)
-    {
-      size_t len = asm_lines[i].addr_string.size ();
-
-      if (len > addr_size)
-        addr_size = len;
-
-      len = asm_lines[i].insn.size ();
-      if (len > insn_size)
-	insn_size = len;
-    }
+  size_t addr_size = 0;
+  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -215,11 +243,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.  */
 

[review v2] Style diassembly in the TUI

[Tom Tromey (Code Review)]

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.

[review v2] Style diassembly in the TUI

[Simon Marchi (Code Review)]

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.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
Gerrit-Change-Number: 179
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 30 Oct 2019 19:30:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment

Re: [review] Style diassembly in the TUI

[Matt Rice]

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

[review v3] Style disassembly in the TUI

[Tom Tromey (Code Review)]

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

Style disassembly 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".

This version introduces a new function to strip the styling from the
address string when computing the length.  As a byproduct, it also
removes the unused "insn_size" computation from
tui_disasm_window::set_contents.

gdb/ChangeLog
2019-10-23  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.  Add "addr_size" parameter.
	(tui_disasm_window::set_contents): Use tui_copy_source_line.
	Don't compute maximum size.
	(len_without_escapes): New function

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, 205 insertions(+), 142 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5f93b2b..f3871ee 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,32 @@
 2019-10-23  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.  Add "addr_size" parameter.
+	(tui_disasm_window::set_contents): Use tui_copy_source_line.
+	Don't compute maximum size.
+	(len_without_escapes): New function
+
+2019-10-23  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..7178326 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"
 
@@ -49,15 +50,47 @@
   std::string insn;
 };
 
+/* Helper function to find the number of characters in STR, skipping
+   any ANSI escape sequences.  */
+static size_t
+len_without_escapes (const std::string &str)
+{
+  size_t len = 0;
+  const char *ptr = str.c_str ();
+  char c;
+
+  while ((c = *ptr++) != '\0')
+    {
+      if (c == '\033')
+	{
+	  ui_file_style style;
+	  size_t n_read;
+	  if (style.parse (ptr, &n_read))
+	    ptr += n_read;
+	  else
+	    {
+	      /* Shouldn't happen, but just skip the ESC if it somehow
+		 does.  */
+	      ++ptr;
+	    }
+	}
+      else
+	++len;
+    }
+  return len;
+}
+
 /* Function to set the disassembly window's content.
    Disassemble count lines starting at pc.
    Return address of the count'th instruction after pc.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
 		 std::vector<tui_asm_line> &asm_lines,
-		 CORE_ADDR pc, int pos, int count)
+		 CORE_ADDR pc, int pos, int count,
+		 size_t *addr_size = nullptr)
 {
-  string_file gdb_dis_out;
+  bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
+  string_file gdb_dis_out (term_out);
 
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
@@ -68,6 +101,17 @@
 
       gdb_dis_out.clear ();
 
+      if (addr_size != nullptr)
+	{
+	  size_t new_size;
+
+	  if (term_out)
+	    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+	  else
+	    new_size = asm_lines[pos + i].addr_string.size ();
+	  *addr_size = std::max (*addr_size, new_size);
+	}
+
       pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
 
       asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
@@ -164,8 +208,7 @@
   struct tui_locator_window *locator = tui_locator_win_info_ptr ();
   int tab_len = tui_tab_width;
   int insn_pos;
-  int addr_size, insn_size;
-  
+
   gdb_assert (line_or_addr.loa == LOA_ADDRESS);
   CORE_ADDR pc = line_or_addr.u.addr;
   if (pc == 0)
@@ -182,23 +225,8 @@
 
   /* Get temporary table that will hold all strings (addr & insn).  */
   std::vector<tui_asm_line> asm_lines (max_lines);
-
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-
-  /* Determine maximum address- and instruction lengths.  */
-  addr_size = 0;
-  insn_size = 0;
-  for (i = 0; i < max_lines; i++)
-    {
-      size_t len = asm_lines[i].addr_string.size ();
-
-      if (len > addr_size)
-        addr_size = len;
-
-      len = asm_lines[i].insn.size ();
-      if (len > insn_size)
-	insn_size = len;
-    }
+  size_t addr_size = 0;
+  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -215,11 +243,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.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
Gerrit-Change-Number: 179
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

Re: [review] Style diassembly in the TUI

[Tom Tromey]

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

[review v3] Style disassembly in the TUI

[Tom Tromey (Code Review)]

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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
Gerrit-Change-Number: 179
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 22:23:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[pushed] Style disassembly in the TUI

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Tom Tromey.

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

Style disassembly 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".

This version introduces a new function to strip the styling from the
address string when computing the length.  As a byproduct, it also
removes the unused "insn_size" computation from
tui_disasm_window::set_contents.

gdb/ChangeLog
2019-11-05  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.  Add "addr_size" parameter.
	(tui_disasm_window::set_contents): Use tui_copy_source_line.
	Don't compute maximum size.
	(len_without_escapes): New function

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, 205 insertions(+), 142 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0c05afe..00d21fa 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,32 @@
 2019-11-05  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.  Add "addr_size" parameter.
+	(tui_disasm_window::set_contents): Use tui_copy_source_line.
+	Don't compute maximum size.
+	(len_without_escapes): New function
+
+2019-11-05  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..7178326 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"
 
@@ -49,15 +50,47 @@
   std::string insn;
 };
 
+/* Helper function to find the number of characters in STR, skipping
+   any ANSI escape sequences.  */
+static size_t
+len_without_escapes (const std::string &str)
+{
+  size_t len = 0;
+  const char *ptr = str.c_str ();
+  char c;
+
+  while ((c = *ptr++) != '\0')
+    {
+      if (c == '\033')
+	{
+	  ui_file_style style;
+	  size_t n_read;
+	  if (style.parse (ptr, &n_read))
+	    ptr += n_read;
+	  else
+	    {
+	      /* Shouldn't happen, but just skip the ESC if it somehow
+		 does.  */
+	      ++ptr;
+	    }
+	}
+      else
+	++len;
+    }
+  return len;
+}
+
 /* Function to set the disassembly window's content.
    Disassemble count lines starting at pc.
    Return address of the count'th instruction after pc.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
 		 std::vector<tui_asm_line> &asm_lines,
-		 CORE_ADDR pc, int pos, int count)
+		 CORE_ADDR pc, int pos, int count,
+		 size_t *addr_size = nullptr)
 {
-  string_file gdb_dis_out;
+  bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
+  string_file gdb_dis_out (term_out);
 
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
@@ -68,6 +101,17 @@
 
       gdb_dis_out.clear ();
 
+      if (addr_size != nullptr)
+	{
+	  size_t new_size;
+
+	  if (term_out)
+	    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+	  else
+	    new_size = asm_lines[pos + i].addr_string.size ();
+	  *addr_size = std::max (*addr_size, new_size);
+	}
+
       pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
 
       asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
@@ -164,8 +208,7 @@
   struct tui_locator_window *locator = tui_locator_win_info_ptr ();
   int tab_len = tui_tab_width;
   int insn_pos;
-  int addr_size, insn_size;
-  
+
   gdb_assert (line_or_addr.loa == LOA_ADDRESS);
   CORE_ADDR pc = line_or_addr.u.addr;
   if (pc == 0)
@@ -182,23 +225,8 @@
 
   /* Get temporary table that will hold all strings (addr & insn).  */
   std::vector<tui_asm_line> asm_lines (max_lines);
-
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-
-  /* Determine maximum address- and instruction lengths.  */
-  addr_size = 0;
-  insn_size = 0;
-  for (i = 0; i < max_lines; i++)
-    {
-      size_t len = asm_lines[i].addr_string.size ();
-
-      if (len > addr_size)
-        addr_size = len;
-
-      len = asm_lines[i].insn.size ();
-      if (len > insn_size)
-	insn_size = len;
-    }
+  size_t addr_size = 0;
+  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -215,11 +243,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.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
Gerrit-Change-Number: 179
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[pushed] Style disassembly in the TUI

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

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

Style disassembly 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".

This version introduces a new function to strip the styling from the
address string when computing the length.  As a byproduct, it also
removes the unused "insn_size" computation from
tui_disasm_window::set_contents.

gdb/ChangeLog
2019-11-05  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.  Add "addr_size" parameter.
	(tui_disasm_window::set_contents): Use tui_copy_source_line.
	Don't compute maximum size.
	(len_without_escapes): New function

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, 205 insertions(+), 142 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0c05afe..00d21fa 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,32 @@
 2019-11-05  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.  Add "addr_size" parameter.
+	(tui_disasm_window::set_contents): Use tui_copy_source_line.
+	Don't compute maximum size.
+	(len_without_escapes): New function
+
+2019-11-05  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..7178326 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"
 
@@ -49,15 +50,47 @@
   std::string insn;
 };
 
+/* Helper function to find the number of characters in STR, skipping
+   any ANSI escape sequences.  */
+static size_t
+len_without_escapes (const std::string &str)
+{
+  size_t len = 0;
+  const char *ptr = str.c_str ();
+  char c;
+
+  while ((c = *ptr++) != '\0')
+    {
+      if (c == '\033')
+	{
+	  ui_file_style style;
+	  size_t n_read;
+	  if (style.parse (ptr, &n_read))
+	    ptr += n_read;
+	  else
+	    {
+	      /* Shouldn't happen, but just skip the ESC if it somehow
+		 does.  */
+	      ++ptr;
+	    }
+	}
+      else
+	++len;
+    }
+  return len;
+}
+
 /* Function to set the disassembly window's content.
    Disassemble count lines starting at pc.
    Return address of the count'th instruction after pc.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
 		 std::vector<tui_asm_line> &asm_lines,
-		 CORE_ADDR pc, int pos, int count)
+		 CORE_ADDR pc, int pos, int count,
+		 size_t *addr_size = nullptr)
 {
-  string_file gdb_dis_out;
+  bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
+  string_file gdb_dis_out (term_out);
 
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
@@ -68,6 +101,17 @@
 
       gdb_dis_out.clear ();
 
+      if (addr_size != nullptr)
+	{
+	  size_t new_size;
+
+	  if (term_out)
+	    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+	  else
+	    new_size = asm_lines[pos + i].addr_string.size ();
+	  *addr_size = std::max (*addr_size, new_size);
+	}
+
       pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
 
       asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
@@ -164,8 +208,7 @@
   struct tui_locator_window *locator = tui_locator_win_info_ptr ();
   int tab_len = tui_tab_width;
   int insn_pos;
-  int addr_size, insn_size;
-  
+
   gdb_assert (line_or_addr.loa == LOA_ADDRESS);
   CORE_ADDR pc = line_or_addr.u.addr;
   if (pc == 0)
@@ -182,23 +225,8 @@
 
   /* Get temporary table that will hold all strings (addr & insn).  */
   std::vector<tui_asm_line> asm_lines (max_lines);
-
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-
-  /* Determine maximum address- and instruction lengths.  */
-  addr_size = 0;
-  insn_size = 0;
-  for (i = 0; i < max_lines; i++)
-    {
-      size_t len = asm_lines[i].addr_string.size ();
-
-      if (len > addr_size)
-        addr_size = len;
-
-      len = asm_lines[i].insn.size ();
-      if (len > insn_size)
-	insn_size = len;
-    }
+  size_t addr_size = 0;
+  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -215,11 +243,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.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8722635eeecbbb1633d943a65b856404c2d467b0
Gerrit-Change-Number: 179
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged