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

login
register
mail settings
Submitter Pedro Alves
Date June 5, 2019, 8:39 p.m.
Message ID <625cd0ba-058d-d4bf-8ba3-8676f335b0f3@redhat.com>
Download mbox | patch
Permalink /patch/33031/
State New
Headers show

Comments

Pedro Alves - June 5, 2019, 8:39 p.m.
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(-)
Tom Tromey - June 5, 2019, 8:47 p.m.
>>>>> "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
Pedro Alves - June 5, 2019, 9:25 p.m.
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

Patch

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