[RFC 0/8] add terminal styling to gdb

[RFC 0/8] add terminal styling to gdb

[Tom Tromey]

This series is not ready to review, but I wanted some feedback on the
general approach before committing to writing documentation, test
cases, and comments.

I've wanted gdb to use colors on the terminal for a while now.  I've
actually tried implementing this a few different ways at different
times, the most successful approach so far being a colorizing frame
filter.

This series takes a more direct approach, namely integrating the code
into gdb.  The basic approach is to have gdb know how to emit ANSI
terminal escape codes to control the color and style (currently just
the intensity).  Then, cli-out is changed to style certain fields as
it emits them.  However, because some places do not use ui-out, some
ad hoc changes are also done.

This series styles function names, file names, and variable names.  It
also styles the gdb welcome message for fun.

I think my earlier patch to make the TUI understand ANSI terminal
escapes will make this patch work there.  (I have not tried this yet.)

Other things could be styled as well.  A few ideas I had:

* Style addresses specially
* Style strings specially
* Add styling to "print" and "ptype" (like, field names could be
  styled the way they are in other spots in this series)
* Go the other way and instead of styling the names of variables,
  style their values
* Style gdb-provided identifiers specially so they can be found
  more easily; here I mean something like the thread number or
  breakpoint number, which you have to read to type back to gdb
  sometimes
* Inferior-provided identifiers (PID or thread name) could be styled
  differently too
* ... your idea here, let me know

Tom

[RFC 3/8] Add output styles to gdb

[Tom Tromey]

This adds some output styling to the CLI.

A style is currently a foreground color, a background color, and an
intensity (dim or bold).  (This list could be expanded depending on
terminal capabilities.)

A style can be applied while printing.  This patch changes cli-out.c
to apply styles just to certain fields, recognized by name.  In
particular, function names and file names are stylized.

This seemed like a reasonable approach, because the names are fixed
due to their use in MI.  That is, the CLI is just as exposed to future
changes as MI is.

Users can control the style via a number of new set/show commands.  In
the interest of not typing many nearly-identical documentation
strings, I automated this.  On the down side, this is not very
i18n-friendly.

I've chose some default colors to use.  I think it would be good to
enable this by default, so that when users start the new gdb, they
will see the new feature.

Stylizing is done if TERM is set and is not "dumb".  This could be
improved when the TUI is available by using the curses has_colors
call.  That is, the lowest layer could call this without committing to
using curses everywhere; see my other patch for TUI colorizing.

I considered adding a new "set_style" method to ui_file.  However,
because the implementation had to interact with the pager code, I
didn't take this approach.  But, one idea might be to put the isatty
check there and then have it defer to the lower layers.
---
 gdb/Makefile.in     |   2 +
 gdb/cli-out.c       |  19 ++++-
 gdb/cli/cli-style.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/cli/cli-style.h |  67 +++++++++++++++++
 gdb/ui-file.h       |  42 +++++++++++
 gdb/utils.c         |  69 +++++++++++++++++-
 gdb/utils.h         |   4 +
 7 files changed, 408 insertions(+), 2 deletions(-)
 create mode 100644 gdb/cli/cli-style.c
 create mode 100644 gdb/cli/cli-style.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c76a4e4394c..3117bf74094 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -239,6 +239,7 @@ SUBDIR_CLI_SRCS = \
 	cli/cli-logging.c \
 	cli/cli-script.c \
 	cli/cli-setshow.c \
+	cli/cli-style.c \
 	cli/cli-utils.c
 
 SUBDIR_CLI_OBS = $(patsubst %.c,%.o,$(SUBDIR_CLI_SRCS))
@@ -1415,6 +1416,7 @@ HFILES_NO_SRCDIR = \
 	cli/cli-decode.h \
 	cli/cli-script.h \
 	cli/cli-setshow.h \
+	cli/cli-style.h \
 	cli/cli-utils.h \
 	common/buffer.h \
 	common/cleanups.h \
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index ad0a34ed39f..eeb3a64cbae 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -25,6 +25,7 @@
 #include "cli-out.h"
 #include "completer.h"
 #include "readline/readline.h"
+#include "cli/cli-style.h"
 
 /* These are the CLI output functions */
 
@@ -156,7 +157,23 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
     spaces (before);
 
   if (string)
-    fputs_filtered (string, m_streams.back ());
+    {
+      cli_style_option *style = nullptr;
+      if (fldname == nullptr)
+	{
+	  /* Nothing.  */
+	}
+      else if (!strcmp (fldname, "file"))
+	style = &file_name_style;
+      else if (!strcmp (fldname, "func") || !strcmp (fldname, "function"))
+	style = &function_name_style;
+
+      if (style != nullptr)
+	set_output_style (m_streams.back (), style->style ());
+      fputs_filtered (string, m_streams.back ());
+      if (style != nullptr)
+	set_output_style (m_streams.back (), ui_file_style ());
+    }
 
   if (after)
     spaces (after);
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
new file mode 100644
index 00000000000..9b210863bd3
--- /dev/null
+++ b/gdb/cli/cli-style.c
@@ -0,0 +1,207 @@
+/* CLI colorizing
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "cli/cli-cmds.h"
+#include "cli/cli-style.h"
+
+static const char * const cli_colors[] = {
+  "none",
+  "black",
+  "red",
+  "green",
+  "yellow",
+  "blue",
+  "magenta",
+  "cyan",
+  "white",
+  nullptr
+};
+
+static const char * const cli_intensities[] = {
+  "normal",
+  "bold",
+  "dim",
+  nullptr
+};
+
+cli_style_option file_name_style (ui_file_style::GREEN);
+cli_style_option function_name_style (ui_file_style::YELLOW);
+
+cli_style_option::cli_style_option (ui_file_style::color fg)
+  : m_foreground (cli_colors[fg - ui_file_style::NONE]),
+    m_background (cli_colors[0]),
+    m_intensity (cli_intensities[ui_file_style::NORMAL])
+{
+}
+
+static int
+color_number (const char *color)
+{
+  for (int i = 0; i < ARRAY_SIZE (cli_colors); ++i)
+    {
+      if (color == cli_colors[i])
+	return i - 1;
+    }
+  gdb_assert_not_reached ("color not found");
+}
+
+ui_file_style
+cli_style_option::style () const
+{
+  ui_file_style result;
+
+  result.m_foreground = (ui_file_style::color) color_number (m_foreground);
+  result.m_background = (ui_file_style::color) color_number (m_background);
+
+  for (int i = 0; i < ARRAY_SIZE (cli_intensities); ++i)
+    {
+      if (m_intensity == cli_intensities[i])
+	{
+	  result.m_intensity = (ui_file_style::intensity) i;
+	  break;
+	}
+    }
+
+  return result;
+}
+
+void
+cli_style_option::do_set (const char *args, int from_tty)
+{
+}
+
+void
+cli_style_option::do_show (const char *args, int from_tty)
+{
+}
+
+void
+cli_style_option::do_show_foreground (struct ui_file *file, int from_tty,
+				      struct cmd_list_element *cmd,
+				      const char *value)
+{
+  const char *name = (const char *) get_cmd_context (cmd);
+  fprintf_filtered (file, _("The \"%s\" foreground color is: %s\n"),
+		    name, value);
+}
+
+void
+cli_style_option::do_show_background (struct ui_file *file, int from_tty,
+				      struct cmd_list_element *cmd,
+				      const char *value)
+{
+  const char *name = (const char *) get_cmd_context (cmd);
+  fprintf_filtered (file, _("The \"%s\" background color is: %s\n"),
+		    name, value);
+}
+
+void
+cli_style_option::do_show_intensity (struct ui_file *file, int from_tty,
+				     struct cmd_list_element *cmd,
+				     const char *value)
+{
+  const char *name = (const char *) get_cmd_context (cmd);
+  fprintf_filtered (file, _("The \"%s\" display intensity is: %s\n"),
+		    name, value);
+}
+
+void
+cli_style_option::add_setshow_commands (const char *name,
+					enum command_class theclass,
+					const char *prefix_doc,
+					const char *prefixname,
+					struct cmd_list_element **set_list,
+					struct cmd_list_element **show_list)
+{
+  m_show_prefix = std::string ("set ") + prefixname + " ";
+  m_show_prefix = std::string ("show ") + prefixname + " ";
+
+  add_prefix_cmd (name, no_class, do_set, prefix_doc, &m_set_list, 
+		  m_show_prefix.c_str (), 0, set_list);
+  add_prefix_cmd (name, no_class, do_show, prefix_doc, &m_show_list,
+		  m_set_prefix.c_str (), 0, show_list);
+
+  add_setshow_enum_cmd ("foreground", theclass, cli_colors,
+			&m_foreground,
+			_("Set the foreground color for this property"),
+			_("Show the foreground color for this property"),
+			nullptr,
+			nullptr,
+			do_show_foreground,
+			&m_set_list, &m_show_list, (void *) name);
+  add_setshow_enum_cmd ("background", theclass, cli_colors,
+			&m_background,
+			_("Set the background color for this property"),
+			_("Show the background color for this property"),
+			nullptr,
+			nullptr,
+			do_show_background,
+			&m_set_list, &m_show_list, (void *) name);
+  add_setshow_enum_cmd ("intensity", theclass, cli_intensities,
+			&m_intensity,
+			_("Set the display intensity color for this property"),
+			_("\
+Show the display intensity color for this property"),
+			nullptr,
+			nullptr,
+			do_show_intensity,
+			&m_set_list, &m_show_list, (void *) name);
+}
+
+static void
+set_style (const char *arg, int from_tty)
+{
+}
+
+static void
+show_style (const char *arg, int from_tty)
+{
+}
+
+void
+_initialize_cli_style ()
+{
+  static cmd_list_element *style_set_list;
+  static cmd_list_element *style_show_list;
+
+  add_prefix_cmd ("style", no_class, set_style, _("\
+Style-specific settings\n\
+Configure various style-related variables, such as colors"),
+		  &style_set_list, "set style ", 0, &setlist);
+  add_prefix_cmd ("style", no_class, show_style, _("\
+Style-specific settings\n\
+Configure various style-related variables, such as colors"),
+		  &style_show_list, "show style ", 0, &showlist);
+
+  file_name_style.add_setshow_commands ("filename", no_class,
+					_("\
+Filename display styling\n\
+Configure filename colors and display intensity."),
+					"style filename",
+					&style_set_list,
+					&style_show_list);
+  function_name_style.add_setshow_commands ("function", no_class,
+					    _("\
+Function name display styling\n\
+Configure function name colors and display intensity"),
+					    "style function",
+					    &style_set_list,
+					    &style_show_list);
+}
diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
new file mode 100644
index 00000000000..d9c00003bd4
--- /dev/null
+++ b/gdb/cli/cli-style.h
@@ -0,0 +1,67 @@
+/* CLI stylizing
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef CLI_STYLE_H
+#define CLI_STYLE_H
+
+#include "ui-file.h"
+
+class cli_style_option
+{
+public:
+
+  cli_style_option (ui_file_style::color fg);
+
+  ui_file_style style () const;
+
+  void add_setshow_commands (const char *name,
+			     enum command_class theclass,
+			     const char *prefix_doc,
+			     const char *prefixname,
+			     struct cmd_list_element **set_list,
+			     struct cmd_list_element **show_list);
+
+private:
+
+  const char *m_foreground;
+  const char *m_background;
+  const char *m_intensity;
+
+  std::string m_show_prefix;
+  std::string m_set_prefix;
+  struct cmd_list_element *m_set_list = nullptr;
+  struct cmd_list_element *m_show_list = nullptr;
+
+  static void do_set (const char *args, int from_tty);
+  static void do_show (const char *args, int from_tty);
+  static void do_show_foreground (struct ui_file *file, int from_tty,
+				  struct cmd_list_element *cmd,
+				  const char *value);
+  static void do_show_background (struct ui_file *file, int from_tty,
+				  struct cmd_list_element *cmd,
+				  const char *value);
+  static void do_show_intensity (struct ui_file *file, int from_tty,
+				 struct cmd_list_element *cmd,
+				 const char *value);
+};
+
+extern cli_style_option file_name_style;
+extern cli_style_option function_name_style;
+
+#endif /* CLI_STYLE_H */
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 2cf5f83d473..630fcffef69 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -21,6 +21,48 @@
 
 #include <string>
 
+/* Styles that can be applied to a ui_file.  */
+struct ui_file_style
+{
+  enum color
+  {
+    NONE = -1,
+    BLACK,
+    RED,
+    GREEN,
+    YELLOW,
+    BLUE,
+    MAGENTA,
+    CYAN,
+    WHITE
+  };
+
+  enum intensity
+  {
+    NORMAL = 0,
+    BOLD,
+    DIM
+  };
+
+
+  enum color m_foreground = NONE;
+  enum color m_background = NONE;
+  enum intensity m_intensity = NORMAL;
+
+  bool operator== (const ui_file_style &other) const
+  {
+    return (m_foreground == other.m_foreground
+	    && m_background == other.m_background
+	    && m_intensity == other.m_intensity);
+  }
+
+  bool operator!= (const ui_file_style &other) const
+  {
+    return !(*this == other);
+  }
+};
+
+
 /* The abstract ui_file base class.  */
 
 class ui_file
diff --git a/gdb/utils.c b/gdb/utils.c
index 1982fa20e64..ca69d5fc57c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1422,6 +1422,63 @@ set_screen_width_and_height (int width, int height)
   set_width ();
 }
 
+static ui_file_style applied_style;
+static ui_file_style desired_style;
+
+static void
+emit (const char *str)
+{
+  if (wrap_column)
+    wrap_buffer.append (str);
+  else
+    puts_unfiltered (str);
+}
+
+static void
+emit_style_escape (const ui_file_style &style)
+{
+  if (applied_style == style)
+    return;
+  applied_style = style;
+
+  emit ("\e[");
+  bool need_semi = false;
+  if (style.m_foreground != ui_file_style::NONE)
+    {
+      emit (std::to_string (30 + style.m_foreground).c_str ());
+      need_semi = true;
+    }
+  if (style.m_background != ui_file_style::NONE)
+    {
+      if (need_semi)
+	emit (";");
+      emit (std::to_string (40 + style.m_background).c_str ());
+      need_semi = true;
+    }
+  if (style.m_intensity != ui_file_style::NORMAL)
+    {
+      if (need_semi)
+	emit (";");
+      emit (std::to_string (style.m_intensity).c_str ());
+    }
+  emit ("m");
+}
+
+void
+set_output_style (struct ui_file *stream, const ui_file_style &style)
+{
+  if (stream != gdb_stdout
+      || style == desired_style
+      || !ui_file_isatty (stream))
+    return;
+  const char *term = getenv ("TERM");
+  if (term == nullptr || !strcmp (term, "dumb"))
+    return;
+
+  desired_style = style;
+  emit_style_escape (style);
+}
+
 /* Wait, so the user can read what's on the screen.  Prompt the user
    to continue by pressing RETURN.  'q' is also provided because
    telling users what to do in the prompt is more user-friendly than
@@ -1437,6 +1494,9 @@ prompt_for_continue (void)
   steady_clock::time_point prompt_started = steady_clock::now ();
   bool disable_pagination = pagination_disabled_for_command;
 
+  /* Clear the current styling.  */
+  emit_style_escape (ui_file_style ());
+
   if (annotation_level > 1)
     printf_unfiltered (("\n\032\032pre-prompt-for-continue\n"));
 
@@ -1481,6 +1541,9 @@ prompt_for_continue (void)
   reinitialize_more_filter ();
   pagination_disabled_for_command = disable_pagination;
 
+  /* Restore the current styling.  */
+  emit_style_escape (desired_style);
+
   dont_repeat ();		/* Forget prev cmd -- CR won't repeat it.  */
 }
 
@@ -1709,7 +1772,10 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	         if chars_per_line is right, we probably just overflowed
 	         anyway; if it's wrong, let us keep going.  */
 	      if (wrap_column)
-		fputc_unfiltered ('\n', stream);
+		{
+		  emit_style_escape (ui_file_style ());
+		  fputc_unfiltered ('\n', stream);
+		}
 
 	      /* Possible new page.  Note that
 		 PAGINATION_DISABLED_FOR_COMMAND might be set during
@@ -1722,6 +1788,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	      if (wrap_column)
 		{
 		  fputs_unfiltered (wrap_indent, stream);
+		  emit_style_escape (desired_style);
 		  fputs_unfiltered (wrap_buffer.c_str (), stream);
 		  /* FIXME, this strlen is what prevents wrap_indent from
 		     containing tabs.  However, if we recurse to print it
diff --git a/gdb/utils.h b/gdb/utils.h
index 68523994b94..0c8a2e1da16 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -423,6 +423,10 @@ extern void fputstrn_unfiltered (const char *str, int n, int quotr,
 /* Return nonzero if filtered printing is initialized.  */
 extern int filtered_printing_initialized (void);
 
+class ui_file_style;
+extern void set_output_style (struct ui_file *stream,
+			      const ui_file_style &style);
+
 /* Display the host ADDR on STREAM formatted as ``0x%x''.  */
 extern void gdb_print_host_address_1 (const void *addr, struct ui_file *stream);
 
-- 
2.13.6

Re: [RFC 3/8] Add output styles to gdb

[Simon Marchi]

On 2018-09-06 5:12 p.m., Tom Tromey wrote:
> This adds some output styling to the CLI.
> 
> A style is currently a foreground color, a background color, and an
> intensity (dim or bold).  (This list could be expanded depending on
> terminal capabilities.)
> 
> A style can be applied while printing.  This patch changes cli-out.c
> to apply styles just to certain fields, recognized by name.  In
> particular, function names and file names are stylized.
> 
> This seemed like a reasonable approach, because the names are fixed
> due to their use in MI.  That is, the CLI is just as exposed to future
> changes as MI is.
> 
> Users can control the style via a number of new set/show commands.  In
> the interest of not typing many nearly-identical documentation
> strings, I automated this.  On the down side, this is not very
> i18n-friendly.
> 
> I've chose some default colors to use.  I think it would be good to
> enable this by default, so that when users start the new gdb, they
> will see the new feature.
> 
> Stylizing is done if TERM is set and is not "dumb".  This could be
> improved when the TUI is available by using the curses has_colors
> call.  That is, the lowest layer could call this without committing to
> using curses everywhere; see my other patch for TUI colorizing.
> 
> I considered adding a new "set_style" method to ui_file.  However,
> because the implementation had to interact with the pager code, I
> didn't take this approach.  But, one idea might be to put the isatty
> check there and then have it defer to the lower layers.

Hi Tom,

Overall this looks great.  I'm not too worried about internationalization.
I think in this case for example:

  _("The \"%s\" display intensity is: %s\n"), name, value

If "name" corresponds to a field name or sub-command name (like "filename"),
it's probably better to leave it in english.  If it refers to the concept
of a filename, then we would want to translate it.

In the latter case, I guess we could do it in two steps, and also pass the value
through gettext:

  std::string tmpl = string_printf ("The %s display intensity is: %%s\n", name);
  printf (_(tmpl.c_str ()), _(value));

So tmpl would contain "The filename display intensity is: %s\n", which could be
looked up by gettext to something that has the proper translation for "filename".
Then, the color name would be translated too.

Or maybe I don't understand how gettext works.


I'm just not sure about choosing styles using the field name.  For a filename, you could
have a bunch of different field names, file filename, original_filename, absolute_filename,
etc.  So it can quickly become a bit crowded here.

Could we pass an additional enum parameter to do_field_string to indicate the type of
element this field represents?  If this parameter has a default value of "NOTHING",
then we can add then incrementally.  Or it can even be an hybrid approach, where we
try to match field names, which works 95% of the time, and then have this enum parameter
to override the auto-detection if needed.

Simon

Re: [RFC 3/8] Add output styles to gdb

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Could we pass an additional enum parameter to do_field_string to
Simon> indicate the type of element this field represents?  If this
Simon> parameter has a default value of "NOTHING", then we can add then
Simon> incrementally.

Yes, either this or, as you mentioned in another note, passing the style
directly, could be done.  I suppose I tend to lean more toward the
semantic approach than the style approach, because maybe having some
kind of "type" attached to a field could be useful in other situations.

Be sure to read this as well:

https://sourceware.org/ml/gdb-patches/2018-10/msg00107.html

It presents another alternative -- one I rejected but maybe it can be
rehabilitated.

These questions about the overall approach and the API are the main
things to resolve up front, since they have the highest cost to change.

Tom

Re: [RFC 3/8] Add output styles to gdb

[Simon Marchi]

On 2018-10-06 3:06 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Could we pass an additional enum parameter to do_field_string to
> Simon> indicate the type of element this field represents?  If this
> Simon> parameter has a default value of "NOTHING", then we can add then
> Simon> incrementally.
> 
> Yes, either this or, as you mentioned in another note, passing the style
> directly, could be done.  I suppose I tend to lean more toward the
> semantic approach than the style approach, because maybe having some
> kind of "type" attached to a field could be useful in other situations.
> 
> Be sure to read this as well:
> 
> https://sourceware.org/ml/gdb-patches/2018-10/msg00107.html
> 
> It presents another alternative -- one I rejected but maybe it can be
> rehabilitated.

I guess you refer to Pedro's suggestion of having custom format strings.  I am not
against that, I am just afraid that because it's a lot more efforts than something
like you did, it would postpone the results indefinitely (unless somebody is super
motivated to do it right now).

Also, I think these custom format strings (LLVM uses its own style too, as mentioned)
need to bring significantly more value (in terms of code simplicity) than the other
alternatives, because they are harder to learn, modify and debug than simple function
calls.
> These questions about the overall approach and the API are the main
> things to resolve up front, since they have the highest cost to change.

Indeed, but at least it's not external API, so we have the option to change.

Simon

Re: [RFC 3/8] Add output styles to gdb

[Tom Tromey]

>> These questions about the overall approach and the API are the main
>> things to resolve up front, since they have the highest cost to change.

Simon> Indeed, but at least it's not external API, so we have the option to change.

The main thing I am concerned about is that if we pick the approach I've
implemented, then changing our minds to a format-based approach will be
harder.  I think that's the case because the user-facing "set"s would be
hard to migrate to a format-based approach.

And, FWIW, I think the format-based approach isn't super hard to
implement.  The main problems there are design problems: whether to
remap the names of tables and fields; how to handle the trouble cases I
found in my initial investigation; and of course whether there are more
difficult cases lurking.

Overall I think I'm in favor of the approach I've posted, but I think
it's a good to examine this idea critically.

Tom

Re: [RFC 3/8] Add output styles to gdb

[Simon Marchi]

On 2018-10-07 20:23, Tom Tromey wrote:
> Simon> Indeed, but at least it's not external API, so we have the
> option to change.
> 
> The main thing I am concerned about is that if we pick the approach 
> I've
> implemented, then changing our minds to a format-based approach will be
> harder.  I think that's the case because the user-facing "set"s would 
> be
> hard to migrate to a format-based approach.
> 
> And, FWIW, I think the format-based approach isn't super hard to
> implement.  The main problems there are design problems: whether to
> remap the names of tables and fields; how to handle the trouble cases I
> found in my initial investigation; and of course whether there are more
> difficult cases lurking.

Ok, so I'd like to understand better the idea of the format-based 
approach, maybe I don't quite get what you mean.  It would be easier to 
comment on something concrete (even without code, just a relatively 
well-defined description of the format).

Simon

Re: [RFC 3/8] Add output styles to gdb

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Ok, so I'd like to understand better the idea of the format-based
Simon> approach, maybe I don't quite get what you mean.  It would be easier
Simon> to comment on something concrete (even without code, just a relatively
Simon> well-defined description of the format).

The minimum, I think, is to have something that encompasses the
formatting, but not the titling, emitted by table headers.  So instead
of:

  uiout->table_header (10, ui_left, "refcount", "Refcount");

the example I gave said

  "{refcount<10}"

where the idea is that "refcount" is the field name, "<" says to
left-justify, and "10" is the field size.  I pictured using ">" for
right justification, and something like "|" for center and ":" or "="
for "none".

Anything outside of {} would be plain text that is just emitted
directly.

Exactly how to spell the formatting can be debated, like should there
be a lead-in character ("set extended-prompt" uses backslash, C uses %).

So, something like maintenance_info_bfds currently reads:

  ui_out_emit_table table_emitter (uiout, 3, -1, "bfds");
  uiout->table_header (10, ui_left, "refcount", "Refcount");
  uiout->table_header (18, ui_left, "addr", "Address");
  uiout->table_header (40, ui_left, "filename", "Filename");

... but in the format approach the first 2 arguments of each call could
be removed.

Then when printing the body:

  ui_out_emit_tuple tuple_emitter (uiout, NULL);
  uiout->field_int ("refcount", gdata->refc);
  uiout->field_string ("addr", host_address_to_string (abfd));
  uiout->field_string ("filename", bfd_get_filename (abfd));
  uiout->text ("\n");

Here the ->text call could be removed and the text put into the format
string.


I didn't do an exhaustive survey of lists or tuples in the way that I
looked at tables.  There are probably difficult cases lurking in there,
just as there were for tables.  And, the difficult table cases remain
unsolved -- I haven't really given them much thought.

Maybe another tricky case is handling indentation of elided stack
frames.


Something I like about the approach I implemented is that it's
relatively easy to explain and use.

The upside of the foramtting approach is that it gives more power to
users.  For example maybe there are things that are currently hidden
behind mi-like-p that could be exposed; or at least users could arrange
to drop fields they are not interested in.

Tom

Re: [RFC 3/8] Add output styles to gdb

[Simon Marchi]

On 2018-10-07 22:49, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Ok, so I'd like to understand better the idea of the 
> format-based
> Simon> approach, maybe I don't quite get what you mean.  It would be 
> easier
> Simon> to comment on something concrete (even without code, just a 
> relatively
> Simon> well-defined description of the format).
> 
> The minimum, I think, is to have something that encompasses the
> formatting, but not the titling, emitted by table headers.  So instead
> of:
> 
>   uiout->table_header (10, ui_left, "refcount", "Refcount");
> 
> the example I gave said
> 
>   "{refcount<10}"
> 
> where the idea is that "refcount" is the field name, "<" says to
> left-justify, and "10" is the field size.  I pictured using ">" for
> right justification, and something like "|" for center and ":" or "="
> for "none".
> 
> Anything outside of {} would be plain text that is just emitted
> directly.

Do we pass in the values as with printf?  Something like

   ui_out->format ("{refcount<10}{addr<18}{filename<40}\n", refcount, 
addr, filename);

How does the underlying implementation of format knows that the refcount 
variable is an int, and that addr and filename are char*?  We go by 
field name too?  Or should the format specifier contain the data type?

> Exactly how to spell the formatting can be debated, like should there
> be a lead-in character ("set extended-prompt" uses backslash, C uses 
> %).
> 
> So, something like maintenance_info_bfds currently reads:
> 
>   ui_out_emit_table table_emitter (uiout, 3, -1, "bfds");
>   uiout->table_header (10, ui_left, "refcount", "Refcount");
>   uiout->table_header (18, ui_left, "addr", "Address");
>   uiout->table_header (40, ui_left, "filename", "Filename");
> 
> ... but in the format approach the first 2 arguments of each call could
> be removed.
> 
> Then when printing the body:
> 
>   ui_out_emit_tuple tuple_emitter (uiout, NULL);
>   uiout->field_int ("refcount", gdata->refc);
>   uiout->field_string ("addr", host_address_to_string (abfd));
>   uiout->field_string ("filename", bfd_get_filename (abfd));
>   uiout->text ("\n");
> 
> Here the ->text call could be removed and the text put into the format
> string.
> 
> 
> I didn't do an exhaustive survey of lists or tuples in the way that I
> looked at tables.  There are probably difficult cases lurking in there,
> just as there were for tables.  And, the difficult table cases remain
> unsolved -- I haven't really given them much thought.
> 
> Maybe another tricky case is handling indentation of elided stack
> frames.
> 
> 
> Something I like about the approach I implemented is that it's
> relatively easy to explain and use.
> 
> The upside of the foramtting approach is that it gives more power to
> users.  For example maybe there are things that are currently hidden
> behind mi-like-p that could be exposed; or at least users could arrange
> to drop fields they are not interested in.

I tend to think that introducing a format-based system like this is 
relatively orthogonal to the coloring/styling issue.  I see it as 
syntactic sugar over the field_* methods.  Something would parse that 
format string and do the appropriate field_* calls for you under the 
hood.  If we color based on field names, as the current patch does, then 
it wouldn't make any difference.  If we provide a way for the caller to 
override the element type (on which the color/style choice is based), 
then we need to consider that in the format string syntax.  If field 
"foo" is a filename, we can have something like

     "{foo<40:filename}"

So my opinion is that introducing something like this isn't in the way 
of introducing coloring with the current system.

Simon

Re: [RFC 3/8] Add output styles to gdb

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> "{refcount<10}"

Simon> Do we pass in the values as with printf?  Something like
Simon> ui_out-> format ("{refcount<10}{addr<18}{filename<40}\n", refcount,
Simon> addr, filename);

Nope, the idea is to continue to use the existing ui-out methods.  So
continuing the gdb_bfd example, this code in print_one_bfd:

  uiout->field_int ("refcount", gdata->refc);
  uiout->field_string ("addr", host_address_to_string (abfd));
  uiout->field_string ("filename", bfd_get_filename (abfd));

... would not change at all.

The idea is that the format would be registered as a set/show command
somewhere.  For tables, cli-out could look up the appropriate format
string using the table name; not sure how tuples or lists would work.

Simon> How does the underlying implementation of format knows that the
Simon> refcount variable is an int, and that addr and filename are char*?  We
Simon> go by field name too?  Or should the format specifier contain the data
Simon> type?

The above answers, but basically it would work like today -- the type is
implicit in the ui-out calls, though really at the bottom layer it is
all just strings.

Simon> I tend to think that introducing a format-based system like this is
Simon> relatively orthogonal to the coloring/styling issue.

Part of the overall idea was to let users put styling directly in the
format strings -- sorry, maybe I didn't really spell this out.  So
another example would be

  "{fg=red}{field<40}{fg=default}"

... or something along those lines.  This would conflict a bit with the
idea of styling elements the way that the current patch does it, since
then you'd have to decide what takes precedence.

Though, your note makes me think that this potential conflict really
isn't so bad and we could push forward with the current approach and
deal with more customizable formatting later.

Tom

[RFC 2/8] Add a "context" argument to add_setshow_enum_cmd

[Tom Tromey]

This adds a "context" argument to add_setshow_enum_cmd.  Now
add_setshow_enum_cmd will call set_cmd_context on both of the new
commands.  This is used in a later patch.

gdb/ChangeLog
2018-09-06  Tom Tromey  <tom@tromey.com>

	* command.h (add_setshow_enum_cmd): Add "context" argument.
	* cli/cli-decode.c (add_setshow_enum_cmd): Add "context"
	argument.  Call set_cmd_context.
---
 gdb/ChangeLog        |  6 ++++++
 gdb/cli/cli-decode.c | 10 +++++++---
 gdb/command.h        |  3 ++-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 83dd67efe3f..4a75c41c910 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -495,16 +495,20 @@ add_setshow_enum_cmd (const char *name,
 		      cmd_const_sfunc_ftype *set_func,
 		      show_value_ftype *show_func,
 		      struct cmd_list_element **set_list,
-		      struct cmd_list_element **show_list)
+		      struct cmd_list_element **show_list,
+		      void *context)
 {
-  struct cmd_list_element *c;
+  struct cmd_list_element *c, *show;
 
   add_setshow_cmd_full (name, theclass, var_enum, var,
 			set_doc, show_doc, help_doc,
 			set_func, show_func,
 			set_list, show_list,
-			&c, NULL);
+			&c, &show);
   c->enums = enumlist;
+
+  set_cmd_context (c, context);
+  set_cmd_context (show, context);
 }
 
 const char * const auto_boolean_enums[] = { "on", "off", "auto", NULL };
diff --git a/gdb/command.h b/gdb/command.h
index 3dde2475cb1..cf4def2bcdb 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -300,7 +300,8 @@ extern void add_setshow_enum_cmd (const char *name,
 				  cmd_const_sfunc_ftype *set_func,
 				  show_value_ftype *show_func,
 				  struct cmd_list_element **set_list,
-				  struct cmd_list_element **show_list);
+				  struct cmd_list_element **show_list,
+				  void *context = nullptr);
 
 extern void add_setshow_auto_boolean_cmd (const char *name,
 					  enum command_class theclass,
-- 
2.13.6

[RFC 6/8] Style print_address_symbolic

[Tom Tromey]

print_address_symbolic does not use ui-out, so it did not style
function names.  This patch changes it to use the low-level style code
directly.
---
 gdb/printcmd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index c63d567e487..18721de425f 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -49,6 +49,7 @@
 #include "format.h"
 #include "source.h"
 #include "common/byte-vector.h"
+#include "cli/cli-style.h"
 
 /* Last specified output format.  */
 
@@ -534,7 +535,9 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
     fputs_filtered ("<*", stream);
   else
     fputs_filtered ("<", stream);
+  set_output_style (stream, function_name_style.style ());
   fputs_filtered (name.c_str (), stream);
+  set_output_style (stream, ui_file_style ());
   if (offset != 0)
     fprintf_filtered (stream, "+%u", (unsigned int) offset);
 
@@ -542,10 +545,12 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
      line # of this addr, if we have it; else line # of the nearest symbol.  */
   if (print_symbol_filename && !filename.empty ())
     {
+      fputs_filtered (line == -1 ? " in " : " at ", stream);
+      set_output_style (stream, file_name_style.style ());
+      fputs_filtered (filename.c_str (), stream);
+      set_output_style (stream, ui_file_style ());
       if (line != -1)
-	fprintf_filtered (stream, " at %s:%d", filename.c_str (), line);
-      else
-	fprintf_filtered (stream, " in %s", filename.c_str ());
+	fprintf_filtered (stream, ":%d", line);
     }
   if (unmapped)
     fputs_filtered ("*>", stream);
-- 
2.13.6

[RFC 4/8] Add variable name styling

[Tom Tromey]

This adds style support for variable names.  For the time being, this
is only done in backtraces, not in ptype or print; those places do not
use ui-out and so would need ad hoc changes.

Because "name" is a relatively common MI field name, simple name
recognition does not work for variable names.  So, this patch changes
cli-out to track the enclosing list name, and only colorize "name"
when appearing somewhere within a list named "args".

This also adds styling to the names printed for local variables in
"backtrace full".  This code does not use ui-out, so the styling is
done using the low-level API.
---
 gdb/cli-out.c       | 8 ++++++++
 gdb/cli-out.h       | 3 +++
 gdb/cli/cli-style.c | 8 ++++++++
 gdb/cli/cli-style.h | 1 +
 gdb/printcmd.c      | 7 ++++++-
 5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index eeb3a64cbae..4a5df6d3cff 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -80,6 +80,9 @@ cli_ui_out::do_table_header (int width, ui_align alignment,
 void
 cli_ui_out::do_begin (ui_out_type type, const char *id)
 {
+  m_in_args.push_back (id != nullptr && strcmp (id, "args") == 0);
+  if (m_in_args.back ())
+    ++m_in_args_count;
 }
 
 /* Mark end of a list */
@@ -87,6 +90,9 @@ cli_ui_out::do_begin (ui_out_type type, const char *id)
 void
 cli_ui_out::do_end (ui_out_type type)
 {
+  if (m_in_args.back ())
+    --m_in_args_count;
+  m_in_args.pop_back ();
 }
 
 /* output an int field */
@@ -167,6 +173,8 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
 	style = &file_name_style;
       else if (!strcmp (fldname, "func") || !strcmp (fldname, "function"))
 	style = &function_name_style;
+      else if (m_in_args_count > 0 && !strcmp (fldname, "name"))
+	style = &variable_name_style;
 
       if (style != nullptr)
 	set_output_style (m_streams.back (), style->style ());
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index f701da7bf38..2d26fd419a5 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -73,6 +73,9 @@ private:
 
   std::vector<ui_file *> m_streams;
   bool m_suppress_output;
+
+  std::vector<bool> m_in_args;
+  int m_in_args_count = 0;
 };
 
 extern cli_ui_out *cli_out_new (struct ui_file *stream);
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 9b210863bd3..c5828d65c26 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -43,6 +43,7 @@ static const char * const cli_intensities[] = {
 
 cli_style_option file_name_style (ui_file_style::GREEN);
 cli_style_option function_name_style (ui_file_style::YELLOW);
+cli_style_option variable_name_style (ui_file_style::CYAN);
 
 cli_style_option::cli_style_option (ui_file_style::color fg)
   : m_foreground (cli_colors[fg - ui_file_style::NONE]),
@@ -204,4 +205,11 @@ Configure function name colors and display intensity"),
 					    "style function",
 					    &style_set_list,
 					    &style_show_list);
+  variable_name_style.add_setshow_commands ("variable", no_class,
+					    "style variable",
+					    _("\
+Variable name display styling\n\
+Configure variable name colors and display intensity"),
+					    &style_set_list,
+					    &style_show_list);
 }
diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
index d9c00003bd4..e2840c6f490 100644
--- a/gdb/cli/cli-style.h
+++ b/gdb/cli/cli-style.h
@@ -63,5 +63,6 @@ private:
 
 extern cli_style_option file_name_style;
 extern cli_style_option function_name_style;
+extern cli_style_option variable_name_style;
 
 #endif /* CLI_STYLE_H */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 8c999188d71..c63d567e487 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2156,7 +2156,12 @@ print_variable_and_value (const char *name, struct symbol *var,
   if (!name)
     name = SYMBOL_PRINT_NAME (var);
 
-  fprintf_filtered (stream, "%s%s = ", n_spaces (2 * indent), name);
+  fputs_filtered (n_spaces (2 * indent), stream);
+  set_output_style (stream, variable_name_style.style ());
+  fputs_filtered (name, stream);
+  set_output_style (stream, ui_file_style ());
+  fputs_filtered (" = ", stream);
+
   TRY
     {
       struct value *val;
-- 
2.13.6

Re: [RFC 4/8] Add variable name styling

[Simon Marchi]

On 2018-09-06 5:12 p.m., Tom Tromey wrote:
> This adds style support for variable names.  For the time being, this
> is only done in backtraces, not in ptype or print; those places do not
> use ui-out and so would need ad hoc changes.
> 
> Because "name" is a relatively common MI field name, simple name
> recognition does not work for variable names.  So, this patch changes
> cli-out to track the enclosing list name, and only colorize "name"
> when appearing somewhere within a list named "args".

This patch misses an include for it to build on its own.

This is a case where I think the suggestion I made in my previous message
would help.  It would be much clearer and less magic if the caller just
explicitly communicated the intent of printing a variable, rather than
cli-out trying to guess.

So in print_frame_args, we could have

  -uiout->field_stream ("name", stb);
  +uiout->field_stream ("name", stb, print_style::VARIABLE);

or maybe even pass the style object directly

  uiout->field_stream ("name", stb, variable_name_style);

Simon

[RFC 7/8] Style the gdb welcome message

[Tom Tromey]

This changes gdb to style the welcome message that is shown by
default.  The styling is only done interactively.
---
 gdb/top.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/top.c b/gdb/top.c
index 470276e3106..4f4bdef945e 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1281,7 +1281,15 @@ print_gdb_version (struct ui_file *stream, bool interactive)
      program to parse, and is just canonical program name and version
      number, which starts after last space.  */
 
+  if (interactive)
+    {
+      ui_file_style style = { ui_file_style::MAGENTA, ui_file_style::NONE,
+			      ui_file_style::BOLD };
+      set_output_style (stream, style);
+    }
   fprintf_filtered (stream, "GNU gdb %s%s\n", PKGVERSION, version);
+  if (interactive)
+    set_output_style (stream, ui_file_style ());
 
   /* Second line is a copyright notice.  */
 
-- 
2.13.6

[RFC 1/8] Change wrap buffering to use a std::string

[Tom Tromey]

Currently wrap buffering is implemented by allocating a string that is
the same width as the window, and then writing characters into it.
However, if gdb emits terminal escapes, then these could possibly
overflow the buffer.

To prevent this, change the wrap buffer to be a std::string and update
the various uses.

gdb/ChangeLog
2018-09-06  Tom Tromey  <tom@tromey.com>

	* utils.c (filter_initalized): New global.
	(wrap_buffer): Now a std::string.
	(wrap_pointer): Remove.
	(filtered_printing_initialized, set_width, wrap_here)
	(fputs_maybe_filtered): Update.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/utils.c   | 47 ++++++++++++++++-------------------------------
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 7a8c80c64ed..1982fa20e64 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1268,13 +1268,11 @@ static bool pagination_disabled_for_command;
    the end of the line, we spit out a newline, the indent, and then
    the buffered output.  */
 
-/* Malloc'd buffer with chars_per_line+2 bytes.  Contains characters which
-   are waiting to be output (they have already been counted in chars_printed).
-   When wrap_buffer[0] is null, the buffer is empty.  */
-static char *wrap_buffer;
+static bool filter_initalized = false;
 
-/* Pointer in wrap_buffer to the next character to fill.  */
-static char *wrap_pointer;
+/* Contains characters which are waiting to be output (they have
+   already been counted in chars_printed).  */
+static std::string wrap_buffer;
 
 /* String to indent by if the wrap occurs.  Must not be NULL if wrap_column
    is non-zero.  */
@@ -1347,7 +1345,7 @@ init_page_info (void)
 int
 filtered_printing_initialized (void)
 {
-  return wrap_buffer != NULL;
+  return filter_initalized;
 }
 
 set_batch_flag_and_restore_page_info::set_batch_flag_and_restore_page_info ()
@@ -1387,8 +1385,7 @@ set_screen_size (void)
   rl_set_screen_size (rows, cols);
 }
 
-/* Reinitialize WRAP_BUFFER according to the current value of
-   CHARS_PER_LINE.  */
+/* Reinitialize WRAP_BUFFER.  */
 
 static void
 set_width (void)
@@ -1396,14 +1393,8 @@ set_width (void)
   if (chars_per_line == 0)
     init_page_info ();
 
-  if (!wrap_buffer)
-    {
-      wrap_buffer = (char *) xmalloc (chars_per_line + 2);
-      wrap_buffer[0] = '\0';
-    }
-  else
-    wrap_buffer = (char *) xrealloc (wrap_buffer, chars_per_line + 2);
-  wrap_pointer = wrap_buffer;	/* Start it at the beginning.  */
+  wrap_buffer.clear ();
+  filter_initalized = true;
 }
 
 static void
@@ -1546,17 +1537,13 @@ void
 wrap_here (const char *indent)
 {
   /* This should have been allocated, but be paranoid anyway.  */
-  if (!wrap_buffer)
+  if (!filter_initalized)
     internal_error (__FILE__, __LINE__,
 		    _("failed internal consistency check"));
 
-  if (wrap_buffer[0])
-    {
-      *wrap_pointer = '\0';
-      fputs_unfiltered (wrap_buffer, gdb_stdout);
-    }
-  wrap_pointer = wrap_buffer;
-  wrap_buffer[0] = '\0';
+  if (!wrap_buffer.empty ())
+    fputs_unfiltered (wrap_buffer.c_str (), gdb_stdout);
+  wrap_buffer.clear ();
   if (chars_per_line == UINT_MAX)	/* No line overflow checking.  */
     {
       wrap_column = 0;
@@ -1693,7 +1680,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  if (*lineptr == '\t')
 	    {
 	      if (wrap_column)
-		*wrap_pointer++ = '\t';
+		wrap_buffer.push_back ('\t');
 	      else
 		fputc_unfiltered ('\t', stream);
 	      /* Shifting right by 3 produces the number of tab stops
@@ -1705,7 +1692,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	  else
 	    {
 	      if (wrap_column)
-		*wrap_pointer++ = *lineptr;
+		wrap_buffer.push_back (*lineptr);
 	      else
 		fputc_unfiltered (*lineptr, stream);
 	      chars_printed++;
@@ -1735,8 +1722,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 	      if (wrap_column)
 		{
 		  fputs_unfiltered (wrap_indent, stream);
-		  *wrap_pointer = '\0';	/* Null-terminate saved stuff, */
-		  fputs_unfiltered (wrap_buffer, stream); /* and eject it.  */
+		  fputs_unfiltered (wrap_buffer.c_str (), stream);
 		  /* FIXME, this strlen is what prevents wrap_indent from
 		     containing tabs.  However, if we recurse to print it
 		     and count its chars, we risk trouble if wrap_indent is
@@ -1745,8 +1731,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 		     if we are printing a long string.  */
 		  chars_printed = strlen (wrap_indent)
 		    + (save_chars - wrap_column);
-		  wrap_pointer = wrap_buffer;	/* Reset buffer */
-		  wrap_buffer[0] = '\0';
+		  wrap_buffer.clear ();
 		  wrap_column = 0;	/* And disable fancy wrap */
 		}
 	    }
-- 
2.13.6

Re: [RFC 1/8] Change wrap buffering to use a std::string

[Simon Marchi]

On 2018-09-06 5:12 p.m., Tom Tromey wrote:
> Currently wrap buffering is implemented by allocating a string that is
> the same width as the window, and then writing characters into it.
> However, if gdb emits terminal escapes, then these could possibly
> overflow the buffer.
> 
> To prevent this, change the wrap buffer to be a std::string and update
> the various uses.

This looks like a good change to me, independently of this series.  I think you
should push it right away.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index 7a8c80c64ed..1982fa20e64 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1268,13 +1268,11 @@ static bool pagination_disabled_for_command;
>     the end of the line, we spit out a newline, the indent, and then
>     the buffered output.  */
>  
> -/* Malloc'd buffer with chars_per_line+2 bytes.  Contains characters which
> -   are waiting to be output (they have already been counted in chars_printed).
> -   When wrap_buffer[0] is null, the buffer is empty.  */
> -static char *wrap_buffer;
> +static bool filter_initalized = false;

Typo, "initalized".

> @@ -1546,17 +1537,13 @@ void
>  wrap_here (const char *indent)
>  {
>    /* This should have been allocated, but be paranoid anyway.  */
> -  if (!wrap_buffer)
> +  if (!filter_initalized)
>      internal_error (__FILE__, __LINE__,
>  		    _("failed internal consistency check"));

This should be gdb_assert (filter_initialized), IMO.

Simon

Re: [RFC 1/8] Change wrap buffering to use a std::string

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> Currently wrap buffering is implemented by allocating a string that is
>> the same width as the window, and then writing characters into it.
>> However, if gdb emits terminal escapes, then these could possibly
>> overflow the buffer.
>> 
>> To prevent this, change the wrap buffer to be a std::string and update
>> the various uses.

Simon> This looks like a good change to me, independently of this series.  I think you
Simon> should push it right away.

FWIW I think this patch will have to change to accommodate Windows -- or
at least be totally obsoleted by the needed change.  My plan is to have
a vector holding strings with their styling.  This has to happen because
styling on Windows is done via an API, not via an escape sequence.

Tom

Re: [RFC 1/8] Change wrap buffering to use a std::string

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>> To prevent this, change the wrap buffer to be a std::string and update
>>> the various uses.

Simon> This looks like a good change to me, independently of this series.  I think you
Simon> should push it right away.

Tom> FWIW I think this patch will have to change to accommodate Windows -- or
Tom> at least be totally obsoleted by the needed change.  My plan is to have
Tom> a vector holding strings with their styling.  This has to happen because
Tom> styling on Windows is done via an API, not via an escape sequence.

I think I'm going to change plans here, due to my desire to also
style source code in the TUI.

GNU Source Highlight and Pygments both have ANSI terminal escape back
ends.  And, I already have code that can parse ANSI escapes and turn
them into curses calls.

My new plan for handling Windows is to generalize this ANSI escape
parser and let it be specialized to emit console calls or whatever they
are.

Maybe I'll push this patch in independently after all... not sure yet.

Tom

[RFC 8/8] Style the "Reading symbols" message

[Tom Tromey]

The "Reading symbols" message does not use ui-out (perhaps it
should?), so this styles it using the low-level API.
---
 gdb/symfile.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 20697d9dca2..999e3935622 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -58,6 +58,7 @@
 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
 #include "selftest.h"
+#include "cli/cli-style.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -1111,7 +1112,11 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
 	deprecated_pre_add_symbol_hook (name);
       else
 	{
-	  printf_unfiltered (_("Reading symbols from %s..."), name);
+	  puts_unfiltered (_("Reading symbols from "));
+	  set_output_style (gdb_stdout, file_name_style.style ());
+	  puts_unfiltered (name);
+	  set_output_style (gdb_stdout, ui_file_style ());
+	  puts_unfiltered ("...");
 	  wrap_here ("");
 	  gdb_flush (gdb_stdout);
 	}
-- 
2.13.6

[RFC 5/8] Style locations when setting a breakpoint

[Tom Tromey]

say_where does not use ui-out, so function and file names printed by
it were not styled.  This patch changes say_where to use the low-level
style code directly.
---
 gdb/breakpoint.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4e7dac51574..62824351ed6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -68,6 +68,7 @@
 #include "format.h"
 #include "thread-fsm.h"
 #include "tid-parse.h"
+#include "cli/cli-style.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -12203,9 +12204,14 @@ say_where (struct breakpoint *b)
 	  /* If there is a single location, we can print the location
 	     more nicely.  */
 	  if (b->loc->next == NULL)
-	    printf_filtered (": file %s, line %d.",
-			     symtab_to_filename_for_display (b->loc->symtab),
-			     b->loc->line_number);
+	    {
+	      puts_filtered (": file ");
+	      set_output_style (gdb_stdout, file_name_style.style ());
+	      puts_filtered (symtab_to_filename_for_display (b->loc->symtab));
+	      set_output_style (gdb_stdout, ui_file_style ());
+	      printf_filtered (", line %d.",
+			       b->loc->line_number);
+	    }
 	  else
 	    /* This is not ideal, but each location may have a
 	       different file name, and this at least reflects the
-- 
2.13.6

Re: [RFC 5/8] Style locations when setting a breakpoint

[Simon Marchi]

On 2018-09-06 5:13 p.m., Tom Tromey wrote:
> say_where does not use ui-out, so function and file names printed by
> it were not styled.  This patch changes say_where to use the low-level
> style code directly.
> ---
>  gdb/breakpoint.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 4e7dac51574..62824351ed6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -68,6 +68,7 @@
>  #include "format.h"
>  #include "thread-fsm.h"
>  #include "tid-parse.h"
> +#include "cli/cli-style.h"
>  
>  /* readline include files */
>  #include "readline/readline.h"
> @@ -12203,9 +12204,14 @@ say_where (struct breakpoint *b)
>  	  /* If there is a single location, we can print the location
>  	     more nicely.  */
>  	  if (b->loc->next == NULL)
> -	    printf_filtered (": file %s, line %d.",
> -			     symtab_to_filename_for_display (b->loc->symtab),
> -			     b->loc->line_number);
> +	    {
> +	      puts_filtered (": file ");
> +	      set_output_style (gdb_stdout, file_name_style.style ());
> +	      puts_filtered (symtab_to_filename_for_display (b->loc->symtab));
> +	      set_output_style (gdb_stdout, ui_file_style ());
> +	      printf_filtered (", line %d.",
> +			       b->loc->line_number);
> +	    }

In general, I guess we'll want to use an RAII object that resets the output style,
in case the code between the two calls throws?

Simon

Re: [RFC 0/8] add terminal styling to gdb

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Thu,  6 Sep 2018 15:12:55 -0600
> 
> I've wanted gdb to use colors on the terminal for a while now.  I've
> actually tried implementing this a few different ways at different
> times, the most successful approach so far being a colorizing frame
> filter.
> 
> This series takes a more direct approach, namely integrating the code
> into gdb.  The basic approach is to have gdb know how to emit ANSI
> terminal escape codes to control the color and style (currently just
> the intensity).  Then, cli-out is changed to style certain fields as
> it emits them.  However, because some places do not use ui-out, some
> ad hoc changes are also done.

This is a worthy goal, IMO, but please allow a level of abstraction
between output styles and ANSI escape sequences.  In particular, the
assumption that changing a style or switching text attributes (like
color, bold, etc.) requires to "emit" something to the terminal, is
not necessarily true.  Please allow for terminals where doing that
requires a function call that doesn't necessarily write anything to
the terminal.

Another, perhaps alternative possibility, would be to use
curses/ncurses features for controlling text color.

Finally, did you consider the use case of running GDB from Emacs (via
the old GUD, which uses CLI I/O)?  Would the color escapes be disabled
in that case, or would that require Emacs to interpret them?  Same
question for other front-ends which use CLI, if there are such.

Thanks.

Re: [RFC 0/8] add terminal styling to gdb

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> This is a worthy goal, IMO, but please allow a level of abstraction
Eli> between output styles and ANSI escape sequences.  In particular, the
Eli> assumption that changing a style or switching text attributes (like
Eli> color, bold, etc.) requires to "emit" something to the terminal, is
Eli> not necessarily true.  Please allow for terminals where doing that
Eli> requires a function call that doesn't necessarily write anything to
Eli> the terminal.

I assume you're talking about Windows here.  I don't know anything about
Windows -- could you explain a bit about what's needed to support it?

I can't implement it but I could probably rework the low-level code --
the stuff that interacts with the pager and line wrap buffering -- to
let the stylizing be done in a host-dependent way.

One idea would be to have the line-wrap buffer be a vector that
alternates strings and styles.  Then the terminal-specific output
functions could apply styles in whatever way is appropriate.  Would this
work for Windows?

Eli> Another, perhaps alternative possibility, would be to use
Eli> curses/ncurses features for controlling text color.

Yeah.  I wrote some curses code for this for the TUI.  We could reuse
this when the TUI is available, but since gdb doesn't require curses
currently, this couldn't be the only approach.

It'd be cleaner if each ui-out handled this, so the TUI could implement
styles directly, but there is a layering problem where the pager needs
understand styles, and the pager is beneath ui-out.

Eli> Finally, did you consider the use case of running GDB from Emacs (via
Eli> the old GUD, which uses CLI I/O)?  Would the color escapes be disabled
Eli> in that case, or would that require Emacs to interpret them?  Same
Eli> question for other front-ends which use CLI, if there are such.

Based on experience with my colorizing frame filter, it should all be
fine.

First, looking at gdb in Emacs right now, I see:

    (top-gdb) show env TERM
    TERM = dumb

My patches use this to disable colorizing.

That's not the end of the story though.  comint.el puts
ansi-color-process-output onto comint-output-filter-functions by
default.  So, I think we could have gdb look at getenv("INSIDE_EMACS")
and re-enable the colorizing by default.

Tom

Re: [RFC 0/8] add terminal styling to gdb

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
> Date: Fri, 07 Sep 2018 08:36:09 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> This is a worthy goal, IMO, but please allow a level of abstraction
> Eli> between output styles and ANSI escape sequences.  In particular, the
> Eli> assumption that changing a style or switching text attributes (like
> Eli> color, bold, etc.) requires to "emit" something to the terminal, is
> Eli> not necessarily true.  Please allow for terminals where doing that
> Eli> requires a function call that doesn't necessarily write anything to
> Eli> the terminal.
> 
> I assume you're talking about Windows here.

Yes, and there's also go32 (a.k.a. DJGPP, a.k.a. MSDOS).

> I don't know anything about Windows -- could you explain a bit about
> what's needed to support it?

To change the visual appearance of the following text, such as colors
or standout, you need to call a function.  then you write text as
usual, with printf etc., and it appears with the attributes requested
by that function call.  Then, when you want to get back to the
defaults, you need to call the same function again, but with a
different argument.  The argument tells what video attributes shall be
turned on or off.

If you want a working example, you can look at pcterm.c in the
Texinfo's info/ directory, it is used for the stand-alone Info reader.

> I can't implement it but I could probably rework the low-level code --
> the stuff that interacts with the pager and line wrap buffering -- to
> let the stylizing be done in a host-dependent way.

Yes, that's what I was asking for.  The actual implementation should
come from someone else (perhaps myself ;-).

> One idea would be to have the line-wrap buffer be a vector that
> alternates strings and styles.  Then the terminal-specific output
> functions could apply styles in whatever way is appropriate.  Would this
> work for Windows?

Sorry, I don't think I follow.  What is the line-wrap buffer, and how
is it related to the issue at hand?

> Eli> Finally, did you consider the use case of running GDB from Emacs (via
> Eli> the old GUD, which uses CLI I/O)?  Would the color escapes be disabled
> Eli> in that case, or would that require Emacs to interpret them?  Same
> Eli> question for other front-ends which use CLI, if there are such.
> 
> Based on experience with my colorizing frame filter, it should all be
> fine.
> 
> First, looking at gdb in Emacs right now, I see:
> 
>     (top-gdb) show env TERM
>     TERM = dumb
> 
> My patches use this to disable colorizing.
> 
> That's not the end of the story though.  comint.el puts
> ansi-color-process-output onto comint-output-filter-functions by
> default.  So, I think we could have gdb look at getenv("INSIDE_EMACS")
> and re-enable the colorizing by default.

Yes, I agree on both counts.

(I presume this feature is just for CLI, not for the MI output,
right?)

Thanks.

Re: [RFC 0/8] add terminal styling to gdb

[Eli Zaretskii]

> Date: Fri, 07 Sep 2018 17:56:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> To change the visual appearance of the following text, such as colors
> or standout, you need to call a function.

Sorry, in case it isn't clear: calling that function produces no
output, i.e. nothing is written to the destination buffer or file.

Re: [RFC 0/8] add terminal styling to gdb

[Joel Brobecker]

Hi Tom,

> I've wanted gdb to use colors on the terminal for a while now.  I've
> actually tried implementing this a few different ways at different
> times, the most successful approach so far being a colorizing frame
> filter.
> 
> This series takes a more direct approach, namely integrating the code
> into gdb.  The basic approach is to have gdb know how to emit ANSI
> terminal escape codes to control the color and style (currently just
> the intensity).  Then, cli-out is changed to style certain fields as
> it emits them.  However, because some places do not use ui-out, some
> ad hoc changes are also done.
> 
> This series styles function names, file names, and variable names.  It
> also styles the gdb welcome message for fun.
> 
> I think my earlier patch to make the TUI understand ANSI terminal
> escapes will make this patch work there.  (I have not tried this yet.)
> 
> Other things could be styled as well.  A few ideas I had:

This is going to be a very nice improvement, thank you!

I am a bit tied for time to take a first look at the implementation,
but the approach sounds good to me.
-- 
Joel

Re: [RFC 0/8] add terminal styling to gdb

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This series is not ready to review, but I wanted some feedback on the
Tom> general approach before committing to writing documentation, test
Tom> cases, and comments.

Tom> I've wanted gdb to use colors on the terminal for a while now.  I've
Tom> actually tried implementing this a few different ways at different
Tom> times, the most successful approach so far being a colorizing frame
Tom> filter.

Pedro asked on irc about a styling design closer to what lldb does.
That program lets the user customize the output of commands like 'bt',
by providing a format that is followed.  Adding colorizing to this is
just a matter of adding a color specification to the format.

I did consider a design like this for gdb.  In particular what I
looked at was adding format strings based on the existing ui-out code.
Then, cli-out would be changed to parse these strings, collect fields,
and then format the fields according to the format.  Finally, explicit
formatting at the call sites would be removed and replaced with the
appropriate default format strings.

This seemed like an attractive idea because ui-out is close to what we
would want here, and since the field names are already an advertised
API they could probably be presented to the user as well.

A super simple example that in this plan we could remove:

  uiout->text ("\n");

from print_one_bfd and replace it with a format like

  "{refcount<10}{addr<18}{filename<40}\n"

(Just one possible syntax.)

Table headers would be emitted using the same format.


A further idea in this plan was to get rid of is-mi-like-p tests in
favor of just emitting everything and letting the format simply not
print fields that were uninteresting.  This is harder, though, because
there are more special cases that would have to be handled.


However, I rejected this plan because some of the existing exceptions
to the general approach seemed ugly to deal with.  The formatting
string would have to be a mini language of its own in order to work.

* info_osdata emits a variable number of columns
* print_program_space prints extra information between rows
* breakpoint_1 conditionally emits a column based on whether
  addressprint is set

Also some of the table names or column names are poorly chosen.  I
suppose this could be handled by having some mapping in the CLI.


One thing I like about the approach I took in the current series is
that it allows for semantic styling.  That is, we can style filenames
the same way everywhere.  Also it can easily be done in situations
where gdb is not using ui-out.

Now, one way to handle this in the formatting scenario would be to
introduce formatting constructs to use semantic styling, like "start a
file name now" or whatever.  In this idea, though, the two approaches
are complementary and so we wouldn't need to decide now: we could move
forward with the series as-is and add formatting at a later stage.


Speaking of that, right now the semantic styling is actually done by
matching field names.  I wonder if ui-out should have new methods for
things like "field_filename" or whatever other "type" information we'd
like to associate.  Alternatively I wonder if the style should just be
passed as an optional argument to the various ui-out methods.

Tom