From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-156651-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 22372 invoked by alias); 5 Jun 2019 18:12:28 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Received: (qmail 22316 invoked by uid 89); 5 Jun 2019 18:12:28 -0000
Authentication-Results: sourceware.org; auth=none
X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=2516, time!, hooked, time
X-HELO: mail-wr1-f66.google.com
Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Jun 2019 18:12:25 +0000
Received: by mail-wr1-f66.google.com with SMTP id d18so20434751wrs.5        for <gdb-patches@sourceware.org>; Wed, 05 Jun 2019 11:12:24 -0700 (PDT)
Return-Path: <palves@redhat.com>
Received: from ?IPv6:2001:8a0:f913:f700:4eeb:42ff:feef:f164? ([2001:8a0:f913:f700:4eeb:42ff:feef:f164])        by smtp.gmail.com with ESMTPSA id 32sm56345427wra.35.2019.06.05.11.12.21        (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128);        Wed, 05 Jun 2019 11:12:22 -0700 (PDT)
Subject: ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output)
To: Tom Tromey <tom@tromey.com>
References: <20190605020116.1550-1-tom@tromey.com> <1ee4bd6b-4cdf-f3a9-74af-0843bf123a8b@redhat.com> <87lfygi1x0.fsf@tromey.com> <32872d6a-15d6-9718-59ae-957694e114c9@redhat.com>
Cc: gdb-patches@sourceware.org
From: Pedro Alves <palves@redhat.com>
Message-ID: <b0299d9c-22d8-de30-72b5-99de4605d5dd@redhat.com>
Date: Wed, 05 Jun 2019 18:12:00 -0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <32872d6a-15d6-9718-59ae-957694e114c9@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
X-SW-Source: 2019-06/txt/msg00125.txt.bz2

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

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;
 
-- 
2.14.5