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, 6:12 p.m.
Message ID <b0299d9c-22d8-de30-72b5-99de4605d5dd@redhat.com>
Download mbox | patch
Permalink /patch/33030/
State New
Headers show

Comments

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(-)
Tom Tromey - June 5, 2019, 8:27 p.m.
>>>>> "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

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;