ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output)

Message ID b0299d9c-22d8-de30-72b5-99de4605d5dd@redhat.com
State New
Headers show
Series
  • ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output)
Related show

Commit Message

Pedro Alves June 5, 2019, 6:12 p.m.
On 6/5/19 4:21 PM, Pedro Alves wrote:
> On 6/5/19 2:42 PM, Tom Tromey wrote:

>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>>

>> Pedro> I wish we didn't have to split the lines across different calls though.

>> Pedro> I know we don't officially do i18n yet (*), but these kinds of changes will

>> Pedro> only make it more difficult to get there.

>>

>> Pedro> Maybe with something like:

>>

>> Pedro>   if (strcmp (cwd.get (), current_directory) != 0)

>> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),

>> Pedro> 		       current_directory, cwd.get ());

>> Pedro>   else

>> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);

>>

>> Pedro> I'm sure you've considered something like that; I think we've discussed it

>> Pedro> before.  What are your current thoughts?

>>

>> I think what would be nice is a gdb-specific printf extension for this,

>> e.g.:

>>

>>     printf_unfiltered ("Working directory %<%s%>\n",

>>                        ui_out_style_kind::FILE, 

>>                        current_directory);

>>

>> %< takes a style argument and %> does not.

>>

>> However, for that to work, we'd need a GCC enhancement to let gdb modify

>> the printf format checking.

> 

> While not so good looking, instead of a GCC enhancement, we could maybe

> do it the Linux way, like binutils did too:

> 

>  https://sourceware.org/ml/binutils/2018-02/msg00306.html

> 

> See 871b3ab29e87c.

> 

> So e.g., %pS would be a style, a %pN would be no style.

> 

>     printf_unfiltered ("Working directory %pS%s%pN\n",

>                        ui_out_style_kind::FILE, 

>                        current_directory);

> 

>>

>>

>> Note that if we want to be serious about i18n then there is also the

>> problem that the whole MI approach is not very i18n-friendly.  There are

>> spots that split calls like this, but which can't easily be unsplit

>> (even with a printf extension) due to MI.  So maybe something bigger is

>> needed.

> 

> Right.

> 

> So instead of e.g.:

> 

>  uiout->text ("\nWatchpoint ");

>  uiout->field_int ("wpnum", b->number);

>  uiout->text (" deleted because the program has left the block in\n"

>               "which its expression is valid.\n");

> 

> We could have:

> 

>  uiout->field_fmt ("\nWatchpoint %pF deleted because the program "

>                     "has left the block in\n"

>                     "which its expression is valid.\n"", 

>                     int_field ("wpnum", b->number).ptr ());

> 

> With ui_out_field being something like 

> 

>  struct int_field

>  {

>     int_field (const char *field_name, int val);

> 

>     // need this because we can't pass a reference via va_args.

>     void *ptr () { return this; }

> 

>     const char *m_field_name;

>     int m_val;

>  };

> 

> There would then be another class for CORE_ADDR, another for strings,

> etc, matching the ui_out::field_int, ui_out::field_string, etc. methods.

> Or alternatively, we could have a single uiout_field class that records 

> its type depending on which ctor was called.

> 


Prototype time!

So I looked around a bit how to prototype this, and borrowed format_pieces,
and some code from printf_command.

I hooked up "%pF" for ui_out fields, and added that int_field struct above.

I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
Is there a global current style stack we can push a style to/from?

I tested this by loading a program under gdb, and using "start", and
checking that the breakpoint number came out as expected, both in
CLI mode:

 Temporary breakpoint 1, main () at threads.c:57
 57          long i = 0;

and in MI mode:
  *stopped,reason="breakpoint-hit",disp="del",bkptno="1", ....

Pushed to the users/palves/format_strings branch.

From bef62542b4f0f650e65ddcb72611116e73df9269 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 5 Jun 2019 09:17:16 +0100
Subject: [PATCH] Make ui_out::message support %pF, %pS, %pN

---
 gdb/breakpoint.c    | 25 ++++++++--------
 gdb/common/format.c | 15 +++++++++-
 gdb/common/format.h |  2 +-
 gdb/ui-out.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/ui-out.h        | 21 +++++++++++++
 5 files changed, 133 insertions(+), 16 deletions(-)

-- 
2.14.5

Comments

Tom Tromey June 5, 2019, 8:27 p.m. | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Prototype time!

Nice.

Pedro> I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
Pedro> Is there a global current style stack we can push a style to/from?

No, there's only a way to set the current style.  However, it's not
really intended for callers to set the style and then just leave it set
-- all the existing calls should set the style, then later set it back
to the default style.

It seems to me that this could be enforced by our printf.  Or, if we
really do want stacking, the stack could just be local to this function.

I wouldn't mind some kind of brackets being used as the characters here,
like %p[ ... %p].  It's too bad that we still have to pass some pointer
value for the closer.

Tom
Pedro Alves June 5, 2019, 8:39 p.m. | #2
On 6/5/19 9:27 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Prototype time!

> 

> Nice.

> 

> Pedro> I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.

> Pedro> Is there a global current style stack we can push a style to/from?

> 

> No, there's only a way to set the current style.  However, it's not

> really intended for callers to set the style and then just leave it set

> -- all the existing calls should set the style, then later set it back

> to the default style.

> 

> It seems to me that this could be enforced by our printf.  Or, if we

> really do want stacking, the stack could just be local to this function.

> 

> I wouldn't mind some kind of brackets being used as the characters here,

> like %p[ ... %p].  It's too bad that we still have to pass some pointer

> value for the closer.

Yeah.  I prototyped %pS/%pN now, and having to pass a value for the closer
is ... yeah, a bit ugly.

Also, since gcc expects a pointer for %p, and we want to pass an
enum for the style, I added a small pointer-wrapper hack -- see the
ptr function.  Yay C++.

I've not been paying much attention to the styling patches, so I can't
off hand tell which places would benefit the most from this.  So I just
grepped for _styled and replaced a couple spots.  Likely there are better
examples.

Anyway, see below.

The format strings do get a bit ... wild:

   "%pS%s%pN  %pS%s%pN\n"

With your suggestion, it'd look like:

   "%p[%s%p]  %p[%s%p]\n"

I'm thinking that %pS/%pN %p[/%pN] are more appropriate when
the styled string is constant, like:

   "this %p[text here%p] should be styled\n"

For the seemingly common case of printing a string variable
with a style, I'm thinking that a specific formatter would
be better.  I'll post a follow up patch for that.

From 8f1d8637839fc953d089f4816a12531fbf35202b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 5 Jun 2019 20:44:54 +0100
Subject: [PATCH 1/2] Support %pS / %pN for styling

---
 gdb/breakpoint.c |  6 +++---
 gdb/cli-out.c    | 50 ++++++++++++++++++++++++++------------------------
 gdb/cli-out.h    |  5 +++--
 gdb/mi/mi-out.c  |  3 ++-
 gdb/mi/mi-out.h  |  5 +++--
 gdb/symtab.c     | 26 ++++++++++++++------------
 gdb/ui-out.c     | 23 +++++++++++++----------
 gdb/ui-out.h     | 25 ++++++++++++++++++++++---
 gdb/utils.c      | 10 ++++++++++
 gdb/utils.h      |  6 ++++++
 10 files changed, 102 insertions(+), 57 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c4270064275..0c3c7df92fb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6693,9 +6693,9 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     (others > 1) ? "," 
 			     : ((others == 1) ? " and" : ""));
 	  }
-      printf_filtered (_("also set at pc "));
-      fputs_styled (paddress (gdbarch, pc), address_style.style (), gdb_stdout);
-      printf_filtered (".\n");
+      current_uiout->message (_("also set at pc %pS%s%pN.\n"),
+			      ptr (ui_out_style_kind::ADDRESS),
+			      paddress (gdbarch, pc), nullptr);
     }
 }
 
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 55c8d2b3b1b..71ed5c3cf33 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -118,6 +118,27 @@ cli_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
 		   ui_out_style_kind::DEFAULT);
 }
 
+static ui_file_style
+style_from_style_kind (ui_out_style_kind kind)
+{
+  switch (kind)
+    {
+    case ui_out_style_kind::DEFAULT:
+      /* Nothing.  */
+      return {};
+    case ui_out_style_kind::FILE:
+      return file_name_style.style ();
+    case ui_out_style_kind::FUNCTION:
+      return function_name_style.style ();
+    case ui_out_style_kind::VARIABLE:
+      return variable_name_style.style ();
+    case ui_out_style_kind::ADDRESS:
+      return address_style.style ();
+    default:
+      gdb_assert_not_reached ("missing case");
+    }
+}
+
 /* other specific cli_field_* end up here so alignment and field
    separators are both handled by cli_field_string */
 
@@ -160,28 +181,7 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
 
   if (string)
     {
-      ui_file_style fstyle;
-      switch (style)
-	{
-	case ui_out_style_kind::DEFAULT:
-	  /* Nothing.  */
-	  break;
-	case ui_out_style_kind::FILE:
-	  /* Nothing.  */
-	  fstyle = file_name_style.style ();
-	  break;
-	case ui_out_style_kind::FUNCTION:
-	  fstyle = function_name_style.style ();
-	  break;
-	case ui_out_style_kind::VARIABLE:
-	  fstyle = variable_name_style.style ();
-	  break;
-	case ui_out_style_kind::ADDRESS:
-	  fstyle = address_style.style ();
-	  break;
-	default:
-	  gdb_assert_not_reached ("missing case");
-	}
+      ui_file_style fstyle = style_from_style_kind (style);
       fputs_styled (string, fstyle, m_streams.back ());
     }
 
@@ -227,12 +227,14 @@ cli_ui_out::do_text (const char *string)
 }
 
 void
-cli_ui_out::do_message (const char *format, va_list args)
+cli_ui_out::do_message (ui_out_style_kind style,
+			const char *format, va_list args)
 {
   if (m_suppress_output)
     return;
 
-  vfprintf_unfiltered (m_streams.back (), format, args);
+  ui_file_style fstyle = style_from_style_kind (style);
+  vfprintf_styled (m_streams.back (), fstyle, format, args);
 }
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index eeb555fbbec..4ed7b396a57 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -59,8 +59,9 @@ protected:
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (const char *format, va_list args) override
-    ATTRIBUTE_PRINTF (2,0);
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args) override
+    ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index d8bee0f3927..bb52360e540 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -159,7 +159,8 @@ mi_ui_out::do_text (const char *string)
 }
 
 void
-mi_ui_out::do_message (const char *format, va_list args)
+mi_ui_out::do_message (ui_out_style_kind style,
+		       const char *format, va_list args)
 {
 }
 
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 82f77592da8..914a44194d4 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -64,8 +64,9 @@ protected:
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (const char *format, va_list args) override
-    ATTRIBUTE_PRINTF (2,0);
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args) override
+    ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 130d5cd48ff..5696d6fa890 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4603,9 +4603,10 @@ print_symbol_info (enum search_domain kind,
 
       if (filename_cmp (last, s_filename) != 0)
 	{
-	  fputs_filtered ("\nFile ", gdb_stdout);
-	  fputs_styled (s_filename, file_name_style.style (), gdb_stdout);
-	  fputs_filtered (":\n", gdb_stdout);
+	  current_uiout->message ("\nFile %pS%s%pN:\n",
+				  ptr (ui_out_style_kind::FILE),
+				  s_filename,
+				  nullptr);
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4651,15 +4652,16 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
   else
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
-  fputs_styled (tmp, address_style.style (), gdb_stdout);
-  fputs_filtered ("  ", gdb_stdout);
-  if (msymbol.minsym->text_p ())
-    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
-		  function_name_style.style (),
-		  gdb_stdout);
-  else
-    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
-  fputs_filtered ("\n", gdb_stdout);
+
+  current_uiout->message (_("%pS%s%pN  %pS%s%pN\n"),
+			  ptr (ui_out_style_kind::ADDRESS),
+			  tmp,
+			  nullptr,
+			  (msymbol.minsym->text_p ()
+			   ? ptr (ui_out_style_kind::FUNCTION)
+			   : ptr (ui_out_style_kind::DEFAULT)),
+			   MSYMBOL_PRINT_NAME (msymbol.minsym),
+			  nullptr);
 }
 
 /* This is the guts of the commands "info functions", "info types", and
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 9c987a6cfa6..6e1c87ab841 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -548,12 +548,12 @@ ui_out::text (const char *string)
 }
 
 void
-ui_out::call_do_message (const char *format, ...)
+ui_out::call_do_message (ui_out_style_kind style, const char *format, ...)
 {
   va_list args;
 
   va_start (args, format);
-  do_message (format, args);
+  do_message (style, format, args);
   va_end (args);
 }
 
@@ -562,6 +562,8 @@ ui_out::message (const char *format, ...)
 {
   format_pieces fpieces (&format, true);
 
+  ui_out_style_kind style = ui_out_style_kind::DEFAULT;
+
   va_list args;
   va_start (args, format);
 
@@ -572,7 +574,7 @@ ui_out::message (const char *format, ...)
       switch (piece.argclass)
 	{
 	case string_arg:
-	  call_do_message (current_substring, va_arg (args, const char *));
+	  call_do_message (style, current_substring, va_arg (args, const char *));
 	  break;
 	case wide_string_arg:
 	  /* FIXME */
@@ -582,16 +584,16 @@ ui_out::message (const char *format, ...)
 	  break;
 	case long_long_arg:
 #ifdef PRINTF_HAS_LONG_LONG
-	  call_do_message (current_substring, va_arg (args, long long));
+	  call_do_message (style, current_substring, va_arg (args, long long));
 	  break;
 #else
 	  error (_("long long not supported in ui_out::message"));
 #endif
 	case int_arg:
-	  call_do_message (current_substring, va_arg (args, int));
+	  call_do_message (style, current_substring, va_arg (args, int));
 	  break;
 	case long_arg:
-	  call_do_message (current_substring, va_arg (args, long));
+	  call_do_message (style, current_substring, va_arg (args, long));
 	  break;
 	  /* Handle floating-point values.  */
 	case double_arg:
@@ -611,13 +613,14 @@ ui_out::message (const char *format, ...)
 	      }
 	      break;
 	    case 'S':
-	      /* Push style on stack?  */
+	      style = *va_arg (args, ui_out_style_kind *);
 	      break;
 	    case 'N':
-	      /* Pop style from stack?  */
+	      va_arg (args, void *);
+	      style = ui_out_style_kind::DEFAULT;
 	      break;
 	    default:
-	      call_do_message (current_substring, va_arg (args, void *));
+	      call_do_message (style, current_substring, va_arg (args, void *));
 	      break;
 	    }
 	  break;
@@ -629,7 +632,7 @@ ui_out::message (const char *format, ...)
 	     pass a dummy argument because some platforms have
 	     modified GCC to include -Wformat-security by default,
 	     which will warn here if there is no argument.  */
-	  call_do_message (current_substring, 0);
+	  call_do_message (style, current_substring, 0);
 	  break;
 	default:
 	  internal_error (__FILE__, __LINE__,
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index b20d9508330..18af856313d 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -103,6 +103,24 @@ private:
   int m_val;
 };
 
+/* Wrap a ui_out_style_kind in a pointer to a temporary.  */
+
+/* XXX: Make a template?  */
+struct ptrS
+{
+  ptrS (ui_out_style_kind t) : m_t (t) {}
+
+  const ui_out_style_kind *ptr () const { return &m_t; }
+
+  ui_out_style_kind m_t;
+};
+
+static inline const ui_out_style_kind *
+ptr (ptrS &&t)
+{
+  return t.ptr ();
+}
+
 class ui_out
 {
  public:
@@ -188,8 +206,9 @@ class ui_out
     ATTRIBUTE_PRINTF (6,0) = 0;
   virtual void do_spaces (int numspaces) = 0;
   virtual void do_text (const char *string) = 0;
-  virtual void do_message (const char *format, va_list args)
-    ATTRIBUTE_PRINTF (2,0) = 0;
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args)
+    ATTRIBUTE_PRINTF (3,0) = 0;
   virtual void do_wrap_hint (const char *identstring) = 0;
   virtual void do_flush () = 0;
   virtual void do_redirect (struct ui_file *outstream) = 0;
@@ -201,7 +220,7 @@ class ui_out
   { return false; }
 
  private:
-  void call_do_message (const char *format, ...);
+  void call_do_message (ui_out_style_kind style, const char *format, ...);
 
   ui_out_flags m_flags;
 
diff --git a/gdb/utils.c b/gdb/utils.c
index f55661287eb..6ba4e41faa2 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2147,6 +2147,16 @@ fprintf_styled (struct ui_file *stream, const ui_file_style &style,
   set_output_style (stream, ui_file_style ());
 }
 
+/* See utils.h.  */
+
+void
+vfprintf_styled (struct ui_file *stream, const ui_file_style &style,
+		 const char *format, va_list args)
+{
+  set_output_style (stream, style);
+  vfprintf_filtered (stream, format, args);
+  set_output_style (stream, ui_file_style ());
+}
 
 void
 printf_filtered (const char *format, ...)
diff --git a/gdb/utils.h b/gdb/utils.h
index 58b4a283447..bb73d4a8e72 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -429,6 +429,12 @@ extern void fprintf_styled (struct ui_file *stream,
 			    ...)
   ATTRIBUTE_PRINTF (3, 4);
 
+extern void vfprintf_styled (struct ui_file *stream,
+			     const ui_file_style &style,
+			     const char *fmt,
+			     va_list args)
+  ATTRIBUTE_PRINTF (3, 0);
+
 /* Like fputs_filtered, but styles the output according to STYLE, when
    appropriate.  */
 
-- 
2.14.5
Pedro Alves June 5, 2019, 8:42 p.m. | #3
On 6/5/19 9:39 PM, Pedro Alves wrote:

> The format strings do get a bit ... wild:

> 

>    "%pS%s%pN  %pS%s%pN\n"

> 

> With your suggestion, it'd look like:

> 

>    "%p[%s%p]  %p[%s%p]\n"

> 

> I'm thinking that %pS/%pN %p[/%pN] are more appropriate when

> the styled string is constant, like:

> 

>    "this %p[text here%p] should be styled\n"

> 

> For the seemingly common case of printing a string variable

> with a style, I'm thinking that a specific formatter would

> be better.  I'll post a follow up patch for that.


Here it is.  I used %ps.

I pushed these two to the branch, in case you want to play with
them.  (I'm not likely to play with this any more today.)

From 52bf8432db926e2ce2ff8bdbececd242e93b4ca7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Wed, 5 Jun 2019 21:15:24 +0100
Subject: [PATCH 2/2] Introduce %ps / styled_string

---
 gdb/common/format.c |  1 +
 gdb/symtab.c        | 24 +++++++++++-------------
 gdb/ui-out.c        |  6 ++++++
 gdb/ui-out.h        | 20 ++++++++++++++++++++
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/gdb/common/format.c b/gdb/common/format.c
index d33eab2b2b0..177f79afee3 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -256,6 +256,7 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 	      {
 		switch (f[1])
 		  {
+		  case 's':
 		  case 'S':
 		  case 'F':
 		  case 'N':
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 5696d6fa890..7dbf24eb7c5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4603,10 +4603,9 @@ print_symbol_info (enum search_domain kind,
 
       if (filename_cmp (last, s_filename) != 0)
 	{
-	  current_uiout->message ("\nFile %pS%s%pN:\n",
-				  ptr (ui_out_style_kind::FILE),
-				  s_filename,
-				  nullptr);
+	  current_uiout->message
+	    (_("\nFile %ps:\n"),
+	     styled_string (ui_out_style_kind::FILE, s_filename).ptr ());
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4653,15 +4652,14 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
 
-  current_uiout->message (_("%pS%s%pN  %pS%s%pN\n"),
-			  ptr (ui_out_style_kind::ADDRESS),
-			  tmp,
-			  nullptr,
-			  (msymbol.minsym->text_p ()
-			   ? ptr (ui_out_style_kind::FUNCTION)
-			   : ptr (ui_out_style_kind::DEFAULT)),
-			   MSYMBOL_PRINT_NAME (msymbol.minsym),
-			  nullptr);
+  ui_out_style_kind sym_style = (msymbol.minsym->text_p ()
+				 ? ui_out_style_kind::FUNCTION
+				 : ui_out_style_kind::DEFAULT);
+
+  current_uiout->message
+    (_("%ps  %ps\n"),
+     styled_string (ui_out_style_kind::ADDRESS, tmp).ptr (),
+     styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ());
 }
 
 /* This is the guts of the commands "info functions", "info types", and
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6e1c87ab841..4169fdcb935 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -612,6 +612,12 @@ ui_out::message (const char *format, ...)
 		field_int (field->name (), field->val ());
 	      }
 	      break;
+	    case 's':
+	      {
+		styled_string *ss = va_arg (args, styled_string *);
+		call_do_message (ss->style (), "%s", ss->str ());
+	      }
+	      break;
 	    case 'S':
 	      style = *va_arg (args, ui_out_style_kind *);
 	      break;
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 18af856313d..059d1e376aa 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -103,6 +103,26 @@ private:
   int m_val;
 };
 
+struct styled_string
+{
+  styled_string (ui_out_style_kind style, const char *str)
+    : m_style (style),
+      m_str (str)
+  {
+  }
+
+  /* We need this because we can't pass a reference via
+     va_args.  */
+  const styled_string *ptr () const { return this; }
+
+  ui_out_style_kind style () const {return m_style; }
+  const char *str () const {return m_str; }
+
+private:
+  ui_out_style_kind m_style;
+  const char *m_str;
+};
+
 /* Wrap a ui_out_style_kind in a pointer to a temporary.  */
 
 /* XXX: Make a template?  */
-- 
2.14.5
Tom Tromey June 5, 2019, 8:47 p.m. | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Also, since gcc expects a pointer for %p, and we want to pass an
Pedro> enum for the style, I added a small pointer-wrapper hack -- see the
Pedro> ptr function.  Yay C++.

We could just remove the enum, move to passing pointers everywhere, and
convert the *_style objects to be pointers.  This would streamline
things a bit; and we could use nullptr to mean "keep the default".

Another option would be to just add a ptr method to the _style objects,
so instead of ui_out_style_kind::VARIABLE you'd write variable_style.ptr ().

Pedro> I've not been paying much attention to the styling patches, so I can't
Pedro> off hand tell which places would benefit the most from this.  So I just
Pedro> grepped for _styled and replaced a couple spots.  Likely there are better
Pedro> examples.

There's my new patch and I have one more along those lines as well, but
a good existing one that was already deconstructed is in symfile.c:

	  puts_filtered (_("Reading symbols from "));
	  fputs_styled (name, file_name_style.style (), gdb_stdout);
	  puts_filtered ("...\n");

Pedro> For the seemingly common case of printing a string variable
Pedro> with a style, I'm thinking that a specific formatter would
Pedro> be better.  I'll post a follow up patch for that.

I'm interested to see it.

Tom
Tom Tromey June 5, 2019, 8:49 p.m. | #5
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Here it is.  I used %ps.

Looks good to me.

Tom
Pedro Alves June 5, 2019, 9:25 p.m. | #6
On 6/5/19 9:47 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Also, since gcc expects a pointer for %p, and we want to pass an

> Pedro> enum for the style, I added a small pointer-wrapper hack -- see the

> Pedro> ptr function.  Yay C++.

> 

> We could just remove the enum, move to passing pointers everywhere, and

> convert the *_style objects to be pointers.  This would streamline

> things a bit; 


...

> and we could use nullptr to mean "keep the default".


Yeah, that would %pN / %p] redundant/unnecessary...

> 

> Another option would be to just add a ptr method to the _style objects,

> so instead of ui_out_style_kind::VARIABLE you'd write variable_style.ptr ().


... and could thus end up with something like:

  current_uiout->message (_("%pS%s%pS  %pS%s%pS\n"),
			  address_style.style ().ptr (),
			  tmp,
			  nullptr,
			  (msymbol.minsym->text_p ()
			   ? function_name_style.style ().ptr ()
			   : nullptr),
			   MSYMBOL_PRINT_NAME (msymbol.minsym),
			  nullptr);

If you'd like to give this a try, please do feel free to push to
the branch.

> 

> Pedro> I've not been paying much attention to the styling patches, so I can't

> Pedro> off hand tell which places would benefit the most from this.  So I just

> Pedro> grepped for _styled and replaced a couple spots.  Likely there are better

> Pedro> examples.

> 

> There's my new patch and I have one more along those lines as well, but

> a good existing one that was already deconstructed is in symfile.c:

> 

> 	  puts_filtered (_("Reading symbols from "));

> 	  fputs_styled (name, file_name_style.style (), gdb_stdout);

> 	  puts_filtered ("...\n");


So that could be either:

current_uiout->message (_("Reading symbols from %pS%s%pS...\n"),
                        file_name_style.style ().ptr (),
                        name,
                        nullptr);

or:

current_uiout->message (_("Reading symbols from %ps...\n"),
                        styled_string (ui_out_style_kind::FILENAME, name).ptr ());

or even:

current_uiout->message (_("Reading symbols from %ps...\n"),
                        styled_string (file_name_style.style (), name).ptr ());

> 

> Pedro> For the seemingly common case of printing a string variable

> Pedro> with a style, I'm thinking that a specific formatter would

> Pedro> be better.  I'll post a follow up patch for that.

> 

> I'm interested to see it.

> 

Thanks,
Pedro Alves
Tom Tromey June 5, 2019, 10:21 p.m. | #7
>> We could just remove the enum, move to passing pointers everywhere, and

>> convert the *_style objects to be pointers.  This would streamline

>> things a bit; 


Pedro> ...

>> and we could use nullptr to mean "keep the default".


Pedro> Yeah, that would %pN / %p] redundant/unnecessary...

I didn't go quite as far as making everything take pointers, partly
because it made it harder to preserve the distinction between the CLI
layer and the ui-file styling layer.  So instead I just removed the
enum.

My patch also removes (really just comments out) %pS and %pN in favor of
requiring %ps.  Those could be restored with a bit of effort I guess.

Pedro> If you'd like to give this a try, please do feel free to push to
Pedro> the branch.

I didn't push but my patch is appended.  Let me know what you think.  I
can push tomorrow if you want it.  I'm not sure now if what remains of
the patch is a good idea or not; or if moving fully to pointers would be
better.

Tom

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0c3c7df92fb..c5d3ccab1eb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5836,14 +5836,14 @@ print_breakpoint_location (struct breakpoint *b,
 	{
 	  uiout->text ("in ");
 	  uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	  uiout->text (" ");
 	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
 	  uiout->text ("at ");
 	}
       uiout->field_string ("file",
 			   symtab_to_filename_for_display (loc->symtab),
-			   ui_out_style_kind::FILE);
+			   file_name_style.style ());
       uiout->text (":");
 
       if (uiout->is_mi_like_p ())
@@ -6693,9 +6693,9 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     (others > 1) ? "," 
 			     : ((others == 1) ? " and" : ""));
 	  }
-      current_uiout->message (_("also set at pc %pS%s%pN.\n"),
-			      ptr (ui_out_style_kind::ADDRESS),
-			      paddress (gdbarch, pc), nullptr);
+      current_uiout->message (_("also set at pc %ps.\n"),
+			      styled_string (address_style.style (),
+					     paddress (gdbarch, pc)).ptr ());
     }
 }
 
@@ -13355,12 +13355,12 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 	  if (sym)
 	    {
 	      uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-				   ui_out_style_kind::FUNCTION);
+				   function_name_style.style ());
 	      uiout->text (" at ");
 	    }
 	  uiout->field_string ("file",
 			       symtab_to_filename_for_display (sal2.symtab),
-			       ui_out_style_kind::FILE);
+			       file_name_style.style ());
 	  uiout->text (":");
 
 	  if (uiout->is_mi_like_p ())
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 71ed5c3cf33..aaea20e1512 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -73,7 +73,7 @@ cli_ui_out::do_table_header (int width, ui_align alignment,
     return;
 
   do_field_string (0, width, alignment, 0, col_hdr.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* Mark beginning of a list */
@@ -102,7 +102,7 @@ cli_ui_out::do_field_int (int fldno, int width, ui_align alignment,
   std::string str = string_printf ("%d", value);
 
   do_field_string (fldno, width, alignment, fldname, str.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* used to omit a field */
@@ -115,28 +115,7 @@ cli_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
     return;
 
   do_field_string (fldno, width, alignment, fldname, "",
-		   ui_out_style_kind::DEFAULT);
-}
-
-static ui_file_style
-style_from_style_kind (ui_out_style_kind kind)
-{
-  switch (kind)
-    {
-    case ui_out_style_kind::DEFAULT:
-      /* Nothing.  */
-      return {};
-    case ui_out_style_kind::FILE:
-      return file_name_style.style ();
-    case ui_out_style_kind::FUNCTION:
-      return function_name_style.style ();
-    case ui_out_style_kind::VARIABLE:
-      return variable_name_style.style ();
-    case ui_out_style_kind::ADDRESS:
-      return address_style.style ();
-    default:
-      gdb_assert_not_reached ("missing case");
-    }
+		   ui_file_style ());
 }
 
 /* other specific cli_field_* end up here so alignment and field
@@ -145,7 +124,7 @@ style_from_style_kind (ui_out_style_kind kind)
 void
 cli_ui_out::do_field_string (int fldno, int width, ui_align align,
 			     const char *fldname, const char *string,
-			     ui_out_style_kind style)
+			     const ui_file_style &style)
 {
   int before = 0;
   int after = 0;
@@ -180,10 +159,7 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
     spaces (before);
 
   if (string)
-    {
-      ui_file_style fstyle = style_from_style_kind (style);
-      fputs_styled (string, fstyle, m_streams.back ());
-    }
+    fputs_styled (string, style, m_streams.back ());
 
   if (after)
     spaces (after);
@@ -205,7 +181,7 @@ cli_ui_out::do_field_fmt (int fldno, int width, ui_align align,
   std::string str = string_vprintf (format, args);
 
   do_field_string (fldno, width, align, fldname, str.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 void
@@ -227,14 +203,13 @@ cli_ui_out::do_text (const char *string)
 }
 
 void
-cli_ui_out::do_message (ui_out_style_kind style,
+cli_ui_out::do_message (const ui_file_style &style,
 			const char *format, va_list args)
 {
   if (m_suppress_output)
     return;
 
-  ui_file_style fstyle = style_from_style_kind (style);
-  vfprintf_styled (m_streams.back (), fstyle, format, args);
+  vfprintf_styled (m_streams.back (), style, format, args);
 }
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 4ed7b396a57..eb519bb4a8d 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -52,14 +52,14 @@ protected:
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname,
 				const char *string,
-				ui_out_style_kind style) override;
+				const ui_file_style &style) override;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			     const char *fldname, const char *format,
 			     va_list args)
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args) override
     ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
diff --git a/gdb/disasm.c b/gdb/disasm.c
index ed740c26e0f..54401a975b9 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -31,6 +31,7 @@
 #include <algorithm>
 #include "common/gdb_optional.h"
 #include "valprint.h"
+#include "cli/cli-style.h"
 
 /* Disassemble functions.
    FIXME: We should get rid of all the duplicate code in gdb that does
@@ -245,7 +246,7 @@ gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
 	uiout->text (" <");
 	if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
 	  uiout->field_string ("func-name", name.c_str (),
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	uiout->text ("+");
 	uiout->field_int ("offset", offset);
 	uiout->text (">:\t");
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index bb52360e540..b8ee8006018 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -70,9 +70,9 @@ mi_ui_out::do_table_header (int width, ui_align alignment,
   do_field_int (0, 0, ui_center, "width", width);
   do_field_int (0, 0, ui_center, "alignment", alignment);
   do_field_string (0, 0, ui_center, "col_name", col_name.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
   do_field_string (0, width, alignment, "colhdr", col_hdr.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
   close (ui_out_type_tuple);
 }
 
@@ -102,7 +102,7 @@ mi_ui_out::do_field_int (int fldno, int width, ui_align alignment,
 
   xsnprintf (buffer, sizeof (buffer), "%d", value);
   do_field_string (fldno, width, alignment, fldname, buffer,
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* Used to omit a field.  */
@@ -119,7 +119,7 @@ mi_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
 void
 mi_ui_out::do_field_string (int fldno, int width, ui_align align,
 			    const char *fldname, const char *string,
-			    ui_out_style_kind style)
+			    const ui_file_style &style)
 {
   ui_file *stream = m_streams.back ();
   field_separator ();
@@ -159,7 +159,7 @@ mi_ui_out::do_text (const char *string)
 }
 
 void
-mi_ui_out::do_message (ui_out_style_kind style,
+mi_ui_out::do_message (const ui_file_style &style,
 		       const char *format, va_list args)
 {
 }
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 914a44194d4..f01d62f9bf3 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -58,13 +58,13 @@ protected:
 			   const char *fldname) override;
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname, const char *string,
-				ui_out_style_kind style) override;
+				const ui_file_style &style) override;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			  const char *fldname, const char *format, va_list args)
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args) override
     ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 95ad410f23f..0d084564c20 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -31,6 +31,7 @@
 #include "mi/mi-cmds.h"
 #include "python-internal.h"
 #include "common/gdb_optional.h"
+#include "cli/cli-style.h"
 
 enum mi_print_types
 {
@@ -898,7 +899,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 	  if (function == NULL)
 	    out->field_skip ("func");
 	  else
-	    out->field_string ("func", function, ui_out_style_kind::FUNCTION);
+	    out->field_string ("func", function, function_name_style.style ());
 	}
     }
 
@@ -935,7 +936,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 	      out->text (" at ");
 	      annotate_frame_source_file ();
 	      out->field_string ("file", filename.get (),
-				 ui_out_style_kind::FILE);
+				 file_name_style.style ());
 	      annotate_frame_source_file_end ();
 	    }
 	}
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 21085d5c62c..02bc43810f0 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -41,6 +41,7 @@
 #include "common/vec.h"
 #include "inferior.h"
 #include <algorithm>
+#include "cli/cli-style.h"
 
 static const target_info record_btrace_target_info = {
   "record-btrace",
@@ -1091,7 +1092,7 @@ btrace_call_history_src_line (struct ui_out *uiout,
 
   uiout->field_string ("file",
 		       symtab_to_filename_for_display (symbol_symtab (sym)),
-		       ui_out_style_kind::FILE);
+		       file_name_style.style ());
 
   btrace_compute_src_line_range (bfun, &begin, &end);
   if (end < begin)
@@ -1183,13 +1184,13 @@ btrace_call_history (struct ui_out *uiout,
 
       if (sym != NULL)
 	uiout->field_string ("function", SYMBOL_PRINT_NAME (sym),
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
       else if (msym != NULL)
 	uiout->field_string ("function", MSYMBOL_PRINT_NAME (msym),
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
       else if (!uiout->is_mi_like_p ())
 	uiout->field_string ("function", "??",
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
 
       if ((flags & RECORD_PRINT_INSN_RANGE) != 0)
 	{
diff --git a/gdb/skip.c b/gdb/skip.c
index 127b11dc443..0578422b99c 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -36,6 +36,7 @@
 #include "gdb_regex.h"
 #include "common/gdb_optional.h"
 #include <list>
+#include "cli/cli-style.h"
 
 /* True if we want to print debug printouts related to file/function
    skipping. */
@@ -414,7 +415,7 @@ info_skip_command (const char *arg, int from_tty)
       current_uiout->field_string ("file",
 				   e.file ().empty () ? "<none>"
 				   : e.file ().c_str (),
-				   ui_out_style_kind::FILE); /* 4 */
+				   file_name_style.style ()); /* 4 */
       if (e.function_is_regexp ())
 	current_uiout->field_string ("regexp", "y"); /* 5 */
       else
@@ -423,7 +424,7 @@ info_skip_command (const char *arg, int from_tty)
       current_uiout->field_string ("function",
 				   e.function ().empty () ? "<none>"
 				   : e.function ().c_str (),
-				   ui_out_style_kind::FUNCTION); /* 6 */
+				   function_name_style.style ()); /* 6 */
 
       current_uiout->text ("\n");
     }
diff --git a/gdb/solib.c b/gdb/solib.c
index e0b1a921f82..662433d33d1 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -47,6 +47,7 @@
 #include "gdb_bfd.h"
 #include "common/filestuff.h"
 #include "source.h"
+#include "cli/cli-style.h"
 
 /* Architecture-specific operations.  */
 
@@ -1104,7 +1105,7 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
 	else
 	  uiout->field_string ("syms-read", so->symbols_loaded ? "Yes" : "No");
 
-	uiout->field_string ("name", so->so_name, ui_out_style_kind::FILE);
+	uiout->field_string ("name", so->so_name, file_name_style.style ());
 
 	uiout->text ("\n");
       }
diff --git a/gdb/source.c b/gdb/source.c
index 9a30209880b..3f6f64e1fdd 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -46,6 +46,7 @@
 #include <algorithm>
 #include "common/pathstuff.h"
 #include "source-cache.h"
+#include "cli/cli-style.h"
 
 #define OPEN_MODE (O_RDONLY | O_BINARY)
 #define FDOPEN_MODE FOPEN_RB
@@ -1306,7 +1307,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	     not for TUI.  */
 	  if (uiout->is_mi_like_p () || uiout->test_flags (ui_source_list))
 	    uiout->field_string ("file", symtab_to_filename_for_display (s),
-				 ui_out_style_kind::FILE);
+				 file_name_style.style ());
 	  if (uiout->is_mi_like_p () || !uiout->test_flags (ui_source_list))
  	    {
 	      const char *s_fullname = symtab_to_fullname (s);
diff --git a/gdb/stack.c b/gdb/stack.c
index 547e82bbfb2..d8c6fdf550b 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -53,6 +53,7 @@
 #include "observable.h"
 #include "common/def-vector.h"
 #include "cli/cli-option.h"
+#include "cli/cli-style.h"
 
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
@@ -341,7 +342,7 @@ print_frame_arg (const frame_print_options &fp_opts,
   if (arg->entry_kind == print_entry_values_only
       || arg->entry_kind == print_entry_values_compact)
     stb.puts ("@entry");
-  uiout->field_stream ("name", stb, ui_out_style_kind::VARIABLE);
+  uiout->field_stream ("name", stb, variable_name_style.style ());
   annotate_arg_name_end ();
   uiout->text ("=");
 
@@ -909,18 +910,18 @@ print_frame_info (const frame_print_options &fp_opts,
         {
           annotate_function_call ();
           uiout->field_string ("func", "<function called from gdb>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	}
       else if (get_frame_type (frame) == SIGTRAMP_FRAME)
         {
 	  annotate_signal_handler_caller ();
           uiout->field_string ("func", "<signal handler called>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
         }
       else if (get_frame_type (frame) == ARCH_FRAME)
         {
           uiout->field_string ("func", "<cross-architecture call>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	}
       uiout->text ("\n");
       annotate_frame_end ();
@@ -1262,7 +1263,7 @@ print_frame (const frame_print_options &fp_opts,
 	    uiout->field_core_addr ("addr", gdbarch, pc);
 	  else
 	    uiout->field_string ("addr", "<unavailable>",
-				 ui_out_style_kind::ADDRESS);
+				 address_style.style ());
 	  annotate_frame_address_end ();
 	  uiout->text (" in ");
 	}
@@ -1271,7 +1272,7 @@ print_frame (const frame_print_options &fp_opts,
     string_file stb;
     fprintf_symbol_filtered (&stb, funname ? funname.get () : "??",
 			     funlang, DMGL_ANSI);
-    uiout->field_stream ("func", stb, ui_out_style_kind::FUNCTION);
+    uiout->field_stream ("func", stb, function_name_style.style ());
     uiout->wrap_hint ("   ");
     annotate_frame_args ();
 
@@ -1313,7 +1314,8 @@ print_frame (const frame_print_options &fp_opts,
 	uiout->wrap_hint ("   ");
 	uiout->text (" at ");
 	annotate_frame_source_file ();
-	uiout->field_string ("file", filename_display, ui_out_style_kind::FILE);
+	uiout->field_string ("file", filename_display,
+			     file_name_style.style ());
 	if (uiout->is_mi_like_p ())
 	  {
 	    const char *fullname = symtab_to_fullname (sal.symtab);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 26762d289ca..903f2ff817b 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1115,11 +1115,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
       if (deprecated_pre_add_symbol_hook)
 	deprecated_pre_add_symbol_hook (name);
       else
-	{
-	  puts_filtered (_("Reading symbols from "));
-	  fputs_styled (name, file_name_style.style (), gdb_stdout);
-	  puts_filtered ("...\n");
-	}
+	current_uiout->message (_("Reading symbols from %ps...\n"),
+				styled_string (file_name_style.style (),
+					       name).ptr ());
     }
   syms_from_objfile (objfile, addrs, add_flags);
 
@@ -1131,7 +1129,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
   if ((flags & OBJF_READNOW))
     {
       if (should_print)
-	printf_filtered (_("Expanding full symbols from %s...\n"), name);
+	current_uiout->message (_("Expanding full symbols from %ps...\n"),
+				styled_string (file_name_style.style (),
+					       name).ptr ());
 
       if (objfile->sf)
 	objfile->sf->qf->expand_all_symtabs (objfile);
@@ -1143,7 +1143,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
      file, and so printing it twice is just redundant.  */
   if (should_print && !objfile_has_symbols (objfile)
       && objfile->separate_debug_objfile == nullptr)
-    printf_filtered (_("(No debugging symbols found in %s)\n"), name);
+    current_uiout->message (_("(No debugging symbols found in %ps)\n"),
+			    styled_string (file_name_style.style (),
+					   name).ptr ());
 
   if (should_print)
     {
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 7dbf24eb7c5..e5f702ab804 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4605,7 +4605,7 @@ print_symbol_info (enum search_domain kind,
 	{
 	  current_uiout->message
 	    (_("\nFile %ps:\n"),
-	     styled_string (ui_out_style_kind::FILE, s_filename).ptr ());
+	     styled_string (file_name_style.style (), s_filename).ptr ());
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4652,13 +4652,13 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
 
-  ui_out_style_kind sym_style = (msymbol.minsym->text_p ()
-				 ? ui_out_style_kind::FUNCTION
-				 : ui_out_style_kind::DEFAULT);
+  ui_file_style sym_style = (msymbol.minsym->text_p ()
+			     ? function_name_style.style ()
+			     : ui_file_style ());
 
   current_uiout->message
     (_("%ps  %ps\n"),
-     styled_string (ui_out_style_kind::ADDRESS, tmp).ptr (),
+     styled_string (address_style.style (), tmp).ptr (),
      styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ());
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index c7585c67839..170c40a3046 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -57,6 +57,7 @@
 #include "tracefile.h"
 #include "location.h"
 #include <algorithm>
+#include "cli/cli-style.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -3690,7 +3691,7 @@ print_one_static_tracepoint_marker (int count,
     {
       uiout->text ("in ");
       uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-			   ui_out_style_kind::FUNCTION);
+			   function_name_style.style ());
       uiout->wrap_hint (wrap_indent);
       uiout->text (" at ");
     }
@@ -3701,7 +3702,7 @@ print_one_static_tracepoint_marker (int count,
     {
       uiout->field_string ("file",
 			   symtab_to_filename_for_display (sal.symtab),
-			   ui_out_style_kind::FILE);
+			   file_name_style.style ());
       uiout->text (":");
 
       if (uiout->is_mi_like_p ())
diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index 5fabff2cf1c..521c519cb75 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -52,7 +52,7 @@ tui_ui_out::do_field_int (int fldno, int width, ui_align alignment,
 void
 tui_ui_out::do_field_string (int fldno, int width, ui_align align,
 			     const char *fldname, const char *string,
-			     ui_out_style_kind style)
+			     const ui_file_style &style)
 {
   if (suppress_output ())
     return;
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index edf0b912bef..173027df525 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -35,7 +35,7 @@ protected:
   void do_field_int (int fldno, int width, ui_align align, const char *fldname,
 		  int value) override;
   void do_field_string (int fldno, int width, ui_align align, const char *fldname,
-			const char *string, ui_out_style_kind style) override;
+			const char *string, const ui_file_style &style) override;
   void do_field_fmt (int fldno, int width, ui_align align, const char *fldname,
 		  const char *format, va_list args) override
     ATTRIBUTE_PRINTF (6,0);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 4169fdcb935..e74f685af02 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -25,6 +25,7 @@
 #include "language.h"
 #include "ui-out.h"
 #include "common/format.h"
+#include "cli/cli-style.h"
 
 #include <vector>
 #include <memory>
@@ -470,12 +471,12 @@ ui_out::field_core_addr (const char *fldname, struct gdbarch *gdbarch,
 			 CORE_ADDR address)
 {
   field_string (fldname, print_core_address (gdbarch, address),
-		ui_out_style_kind::ADDRESS);
+		address_style.style ());
 }
 
 void
 ui_out::field_stream (const char *fldname, string_file &stream,
-		      ui_out_style_kind style)
+		      const ui_file_style &style)
 {
   if (!stream.empty ())
     field_string (fldname, stream.c_str (), style);
@@ -500,7 +501,7 @@ ui_out::field_skip (const char *fldname)
 
 void
 ui_out::field_string (const char *fldname, const char *string,
-		      ui_out_style_kind style)
+		      const ui_file_style &style)
 {
   int fldno;
   int width;
@@ -548,7 +549,8 @@ ui_out::text (const char *string)
 }
 
 void
-ui_out::call_do_message (ui_out_style_kind style, const char *format, ...)
+ui_out::call_do_message (const ui_file_style &style, const char *format,
+			 ...)
 {
   va_list args;
 
@@ -562,7 +564,7 @@ ui_out::message (const char *format, ...)
 {
   format_pieces fpieces (&format, true);
 
-  ui_out_style_kind style = ui_out_style_kind::DEFAULT;
+  ui_file_style style;
 
   va_list args;
   va_start (args, format);
@@ -618,13 +620,13 @@ ui_out::message (const char *format, ...)
 		call_do_message (ss->style (), "%s", ss->str ());
 	      }
 	      break;
-	    case 'S':
-	      style = *va_arg (args, ui_out_style_kind *);
-	      break;
-	    case 'N':
-	      va_arg (args, void *);
-	      style = ui_out_style_kind::DEFAULT;
-	      break;
+	    /* case 'S': */
+	    /*   style = *va_arg (args, const ui_file_style *); */
+	    /*   break; */
+	    /* case 'N': */
+	    /*   va_arg (args, void *); */
+	    /*   style = nullptr; */
+	    /*   break; */
 	    default:
 	      call_do_message (style, current_substring, va_arg (args, void *));
 	      break;
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 059d1e376aa..f4c3857fe2d 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -26,6 +26,7 @@
 #include <vector>
 
 #include "common/enum-flags.h"
+#include "ui-style.h"
 
 class ui_out_level;
 class ui_out_table;
@@ -67,22 +68,6 @@ enum ui_out_type
     ui_out_type_list
   };
 
-/* Possible kinds of styling.  */
-
-enum class ui_out_style_kind
-{
-  /* The default (plain) style.  */
-  DEFAULT,
-  /* File name.  */
-  FILE,
-  /* Function name.  */
-  FUNCTION,
-  /* Variable name.  */
-  VARIABLE,
-  /* Address.  */
-  ADDRESS
-};
-
 struct int_field
 {
   int_field (const char *name, int val)
@@ -105,7 +90,7 @@ private:
 
 struct styled_string
 {
-  styled_string (ui_out_style_kind style, const char *str)
+  styled_string (const ui_file_style &style, const char *str)
     : m_style (style),
       m_str (str)
   {
@@ -115,32 +100,14 @@ struct styled_string
      va_args.  */
   const styled_string *ptr () const { return this; }
 
-  ui_out_style_kind style () const {return m_style; }
+  const ui_file_style &style () const { return m_style; }
   const char *str () const {return m_str; }
 
 private:
-  ui_out_style_kind m_style;
+  ui_file_style m_style;
   const char *m_str;
 };
 
-/* Wrap a ui_out_style_kind in a pointer to a temporary.  */
-
-/* XXX: Make a template?  */
-struct ptrS
-{
-  ptrS (ui_out_style_kind t) : m_t (t) {}
-
-  const ui_out_style_kind *ptr () const { return &m_t; }
-
-  ui_out_style_kind m_t;
-};
-
-static inline const ui_out_style_kind *
-ptr (ptrS &&t)
-{
-  return t.ptr ();
-}
-
 class ui_out
 {
  public:
@@ -171,10 +138,10 @@ class ui_out
   void field_core_addr (const char *fldname, struct gdbarch *gdbarch,
 			CORE_ADDR address);
   void field_string (const char *fldname, const char *string,
-		     ui_out_style_kind style = ui_out_style_kind::DEFAULT);
+		     const ui_file_style &style = ui_file_style ());
   void field_string (const char *fldname, const std::string &string);
   void field_stream (const char *fldname, string_file &stream,
-		     ui_out_style_kind style = ui_out_style_kind::DEFAULT);
+		     const ui_file_style &style = ui_file_style ());
   void field_skip (const char *fldname);
   void field_fmt (const char *fldname, const char *format, ...)
     ATTRIBUTE_PRINTF (3, 4);
@@ -219,14 +186,14 @@ class ui_out
 			      const char *fldname) = 0;
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname, const char *string,
-				ui_out_style_kind style) = 0;
+				const ui_file_style &style) = 0;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			     const char *fldname, const char *format,
 			     va_list args)
     ATTRIBUTE_PRINTF (6,0) = 0;
   virtual void do_spaces (int numspaces) = 0;
   virtual void do_text (const char *string) = 0;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args)
     ATTRIBUTE_PRINTF (3,0) = 0;
   virtual void do_wrap_hint (const char *identstring) = 0;
@@ -240,7 +207,8 @@ class ui_out
   { return false; }
 
  private:
-  void call_do_message (ui_out_style_kind style, const char *format, ...);
+  void call_do_message (const ui_file_style &style, const char *format,
+			...);
 
   ui_out_flags m_flags;
Pedro Alves June 6, 2019, 3:49 p.m. | #8
On 6/5/19 11:21 PM, Tom Tromey wrote:
>>> We could just remove the enum, move to passing pointers everywhere, and

>>> convert the *_style objects to be pointers.  This would streamline

>>> things a bit; 

> 

> Pedro> ...

> 

>>> and we could use nullptr to mean "keep the default".

> 

> Pedro> Yeah, that would %pN / %p] redundant/unnecessary...

> 

> I didn't go quite as far as making everything take pointers, partly

> because it made it harder to preserve the distinction between the CLI

> layer and the ui-file styling layer.  So instead I just removed the

> enum.

> 

> My patch also removes (really just comments out) %pS and %pN in favor of

> requiring %ps.  Those could be restored with a bit of effort I guess.


Yes, I think so.  I think the main issue is that cli_style_option::style()
returns a new temporary ui_file_style, so we can't take its address, like in:

       current_uiout->message (_("Reading symbols from %pS%s%pS...\n"),
                               &file_name_style.style (), name, nullptr);

But that could be fixed by adding a m_style field to cli_style_option,
and making cli_style_option::style() return a const reference to that.
We'd either need to make cli_style_option register set command hooks
to update that m_size variable whenever the users changes the corresponding
foreground/background setting, or always update m_style when
cli_style_option::style() is called.

> 

> Pedro> If you'd like to give this a try, please do feel free to push to

> Pedro> the branch.

> 

> I didn't push but my patch is appended.  Let me know what you think.  I

> can push tomorrow if you want it.  I'm not sure now if what remains of

> the patch is a good idea or not; or if moving fully to pointers would be

> better.

> 


Please push.  I don't know either, but undoing a patch on a branch is
simple enough if we find that we want to do that.  :-)

Thanks,
Pedro Alves
Tom Tromey June 6, 2019, 11:55 p.m. | #9
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Yes, I think so.  I think the main issue is that cli_style_option::style()
Pedro> returns a new temporary ui_file_style, so we can't take its address, like in:

If it had a 'ptr ()' method, wouldn't the temporary live long enough in
the callee for it to work?

Pedro> Please push.  I don't know either, but undoing a patch on a branch is
Pedro> simple enough if we find that we want to do that.  :-)

I did it.

Tom
Tom Tromey June 7, 2019, 6:27 p.m. | #10
Tom> My patch also removes (really just comments out) %pS and %pN in favor of
Tom> requiring %ps.  Those could be restored with a bit of effort I guess.

I think we do need these.

I have been trying a patch to style gdb's meta-data output, stuff like
<unavailable>.  There are cases where the <...> contains some other
value, like:

	  fprintf_filtered (stream, " <repeats %u times>", reps);

Here we want to rewrite as:

	  fprintf_filtered (stream, " %pS<repeats %u times>%pN",
                            metadata_style.style ().ptr (), reps, nullptr);

Tom
Tom Tromey June 7, 2019, 7:20 p.m. | #11
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


Tom> My patch also removes (really just comments out) %pS and %pN in favor of
Tom> requiring %ps.  Those could be restored with a bit of effort I guess.

Tom> I think we do need these.

I pushed a couple more patches to the branch, including one intended to
fix this.

What's left to do on that branch in order to merge it?

Tom

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 08083645e1e..c4270064275 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4927,10 +4927,10 @@  watchpoint_check (bpstat bs)
 	  if (uiout->is_mi_like_p ())
 	    uiout->field_string
 	      ("reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
-	  uiout->text ("\nWatchpoint ");
-	  uiout->field_int ("wpnum", b->number);
-	  uiout->text (" deleted because the program has left the block in\n"
-		       "which its expression is valid.\n");
+	  uiout->message ("\nWatchpoint %pF deleted because the program has "
+			  "left the block in\n"
+			  "which its expression is valid.\n",
+			  int_field ("wpnum", b->number).ptr ());
 	}
 
       /* Make sure the watchpoint's commands aren't executed.  */
@@ -6245,9 +6245,8 @@  print_one_breakpoint_location (struct breakpoint *b,
   if (!part_of_multiple && b->ignore_count)
     {
       annotate_field (8);
-      uiout->text ("\tignore next ");
-      uiout->field_int ("ignore", b->ignore_count);
-      uiout->text (" hits\n");
+      uiout->message ("\tignore next %pF hits\n",
+		      int_field ("ignore", b->ignore_count).ptr ());
     }
 
   /* Note that an enable count of 1 corresponds to "enable once"
@@ -12428,18 +12427,18 @@  bkpt_print_it (bpstat bs)
   annotate_breakpoint (b->number);
   maybe_print_thread_hit_breakpoint (uiout);
 
-  if (bp_temp)
-    uiout->text ("Temporary breakpoint ");
-  else
-    uiout->text ("Breakpoint ");
   if (uiout->is_mi_like_p ())
     {
       uiout->field_string ("reason",
 			   async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
       uiout->field_string ("disp", bpdisp_text (b->disposition));
     }
-  uiout->field_int ("bkptno", b->number);
-  uiout->text (", ");
+  if (bp_temp)
+    uiout->message ("Temporary breakpoint %pF, ",
+		    int_field ("bkptno", b->number).ptr ());
+  else
+    uiout->message ("Breakpoint %pF, ",
+		    int_field ("bkptno", b->number).ptr ());
 
   return PRINT_SRC_AND_LOC;
 }
diff --git a/gdb/common/format.c b/gdb/common/format.c
index fb3421e62bf..d33eab2b2b0 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -20,7 +20,7 @@ 
 #include "common-defs.h"
 #include "format.h"
 
-format_pieces::format_pieces (const char **arg)
+format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 {
   const char *s;
   char *f, *string;
@@ -251,6 +251,19 @@  format_pieces::format_pieces (const char **arg)
 	      bad = 1;
 	    if (seen_hash || seen_zero || seen_space || seen_plus)
 	      bad = 1;
+
+	    if (gdb_extensions)
+	      {
+		switch (f[1])
+		  {
+		  case 'S':
+		  case 'F':
+		  case 'N':
+		    f++;
+		    break;
+		  }
+	      }
+
 	    break;
 
 	  case 's':
diff --git a/gdb/common/format.h b/gdb/common/format.h
index 058844cec7b..52c0b152bcd 100644
--- a/gdb/common/format.h
+++ b/gdb/common/format.h
@@ -70,7 +70,7 @@  class format_pieces
 {
 public:
 
-  format_pieces (const char **arg);
+  format_pieces (const char **arg, bool gdb_extensions = false);
   ~format_pieces () = default;
 
   DISABLE_COPY_AND_ASSIGN (format_pieces);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6851fd29c6a..9c987a6cfa6 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -24,6 +24,7 @@ 
 #include "expression.h"		/* For language.h */
 #include "language.h"
 #include "ui-out.h"
+#include "common/format.h"
 
 #include <vector>
 #include <memory>
@@ -547,7 +548,7 @@  ui_out::text (const char *string)
 }
 
 void
-ui_out::message (const char *format, ...)
+ui_out::call_do_message (const char *format, ...)
 {
   va_list args;
 
@@ -556,6 +557,89 @@  ui_out::message (const char *format, ...)
   va_end (args);
 }
 
+void
+ui_out::message (const char *format, ...)
+{
+  format_pieces fpieces (&format, true);
+
+  va_list args;
+  va_start (args, format);
+
+  for (auto &&piece : fpieces)
+    {
+      const char *current_substring = piece.string;
+
+      switch (piece.argclass)
+	{
+	case string_arg:
+	  call_do_message (current_substring, va_arg (args, const char *));
+	  break;
+	case wide_string_arg:
+	  /* FIXME */
+	  break;
+	case wide_char_arg:
+	  /* FIXME */
+	  break;
+	case long_long_arg:
+#ifdef PRINTF_HAS_LONG_LONG
+	  call_do_message (current_substring, va_arg (args, long long));
+	  break;
+#else
+	  error (_("long long not supported in ui_out::message"));
+#endif
+	case int_arg:
+	  call_do_message (current_substring, va_arg (args, int));
+	  break;
+	case long_arg:
+	  call_do_message (current_substring, va_arg (args, long));
+	  break;
+	  /* Handle floating-point values.  */
+	case double_arg:
+	case long_double_arg:
+	case dec32float_arg:
+	case dec64float_arg:
+	case dec128float_arg:
+	  /* FIXME */
+	  break;
+	case ptr_arg:
+	  switch (current_substring[2])
+	    {
+	    case 'F':
+	      {
+		int_field *field = va_arg (args, int_field *);
+		field_int (field->name (), field->val ());
+	      }
+	      break;
+	    case 'S':
+	      /* Push style on stack?  */
+	      break;
+	    case 'N':
+	      /* Pop style from stack?  */
+	      break;
+	    default:
+	      call_do_message (current_substring, va_arg (args, void *));
+	      break;
+	    }
+	  break;
+	case literal_piece:
+	  /* Print a portion of the format string that has no
+	     directives.  Note that this will not include any ordinary
+	     %-specs, but it might include "%%".  That is why we use
+	     printf_filtered and not puts_filtered here.  Also, we
+	     pass a dummy argument because some platforms have
+	     modified GCC to include -Wformat-security by default,
+	     which will warn here if there is no argument.  */
+	  call_do_message (current_substring, 0);
+	  break;
+	default:
+	  internal_error (__FILE__, __LINE__,
+			  _("failed internal consistency check"));
+	}
+    }
+
+  va_end (args);
+}
+
 void
 ui_out::wrap_hint (const char *identstring)
 {
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9eba70eedac..b20d9508330 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -83,6 +83,26 @@  enum class ui_out_style_kind
   ADDRESS
 };
 
+struct int_field
+{
+  int_field (const char *name, int val)
+    : m_name (name),
+      m_val (val)
+  {
+  }
+
+  /* We need this because we can't pass a reference via
+     va_args.  */
+  const int_field *ptr () const { return this; }
+
+  const char *name () const {return m_name; }
+  int val () const {return m_val; }
+
+private:
+  const char *m_name;
+  int m_val;
+};
+
 class ui_out
 {
  public:
@@ -181,6 +201,7 @@  class ui_out
   { return false; }
 
  private:
+  void call_do_message (const char *format, ...);
 
   ui_out_flags m_flags;