Re: [PATCH] gdb: add support for %V to printf command

[Andrew Burgess <aburgess@redhat.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Thu, 30 Mar 2023 13:22:10 +0100
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>> 
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -63,6 +63,15 @@
>>      break foo thread 1 task 1
>>      watch var thread 2 task 3
>>  
>> +* The printf command now accepts a '%V' output format which will
>> +  format an expression just as the 'print' command would.  Print
>> +  options can be placed withing '[...]' after the '%V' to modify how
>> +  the value is printed.  E.g:
>> +    printf "%V", some_array
>> +    printf "%V[-array-indexes on]", some_array
>> +  Will print the array without, or with array indexes included, just
>      ^^^^
> That "Will" should not be capitalized.
>
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -13131,6 +13131,10 @@
>>  
>>  @findex $_as_string@r{, convenience function}
>>  @item $_as_string(@var{value})
>> +This convenience function is considered deprecated, and could be
>> +removed from future versions of @value{GDBN}.  Use the @kbd{%V} format
>> +specifier instead (@pxref{%V Format Specifier}).       ^^^^^^^^
>
> Please use @samp, not @kbd there, as we are not describing commands
> typed by the user.
>
>> +@anchor{%V Format Specifier}
>> +Additionally, @code{printf} supports a special @kbd{%V} output format.
>                                                   ^^^^^^^^
> Likewise.
>
>> +Additionally, it is possible to include print options with the
>> +@kbd{%V} format by placing them in @kbd{[...]} immediately after the
>> +@kbd{%V}, like this:               ^^^^^^^^^^^
>    ^^^^^^^^
> @samp again there.
>
> Thanks.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks for the feedback.  I fixed all the issues you pointed out.

The updated patch is below, but there's no need to reread it unless you
want to.  The fixes all seemed pretty straight forward.

Thanks,
Andrew

---

commit 58458800a40bd066a9621b4a19b63630bc087c1f
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Mar 23 12:12:38 2023 +0000

    gdb: add support for %V to printf command
    
    This commit adds a new format for the printf and dprintf commands:
    '%V'.  This new format takes any GDB expression and formats it as a
    string, just as GDB would for a 'print' command, e.g.:
    
      (gdb) print a1
      $a = {2, 4, 6, 8, 10, 12, 14, 16, 18, 20}
      (gdb) printf "%V\n", a1
      {2, 4, 6, 8, 10, 12, 14, 16, 18, 20}
      (gdb)
    
    It is also possible to pass the same options to %V as you might pass
    to the print command, e.g.:
    
      (gdb) print -elements 3 -- a1
      $4 = {2, 4, 6...}
      (gdb) printf "%V[-elements 3]\n", a1
      {2, 4, 6...}
      (gdb)
    
    This new feature would effectively replace an existing feature of GDB,
    the $_as_string builtin convenience function.  However, the
    $_as_string function has a few problems which this new feature solves:
    
    1. $_as_string doesn't currently work when the inferior is not
    running, e.g:
    
      (gdb) printf "%s", $_as_string(a1)
      You can't do that without a process to debug.
      (gdb)
    
    The reason for this is that $_as_string returns a value object with
    string type.  When we try to print this we call value_as_address,
    which ends up trying to push the string into the inferior's address
    space.
    
    Clearly we could solve this problem, the string data exists in GDB, so
    there's no reason why we have to push it into the inferior, but this
    is an existing problem that would need solving.
    
    2. $_as_string suffers from the fact that C degrades arrays to
    pointers, e.g.:
    
      (gdb) printf "%s\n", $_as_string(a1)
      0x404260 <a1>
      (gdb)
    
    The implementation of $_as_string is passed a gdb.Value object that is
    a pointer, it doesn't understand that it's actually an array.  Solving
    this would be harder than issue #1 I think.  The whole array to
    pointer transformation is part of our expression evaluation.  And in
    most cases this is exactly what we want.  It's not clear to me how
    we'd (easily) tell GDB that we didn't want this reduction in _some_
    cases.  But I'm sure this is solvable if we really wanted to.
    
    3. $_as_string is a gdb.Function sub-class, and as such is passed
    gdb.Value objects.  There's no super convenient way to pass formatting
    options to $_as_string.  By this I mean that the new %V feature
    supports print formatting options.  Ideally, we might want to add this
    feature to $_as_string, we might imagine it working something like:
    
      (gdb) printf "%s\n", $_as_string(a1,
                                       elements = 3,
                                       array_indexes = True)
    
    where the first item is the value to print, while the remaining
    options are the print formatting options.  However, this relies on
    Python calling syntax, which isn't something that convenience
    functions handle.  We could possibly rely on strictly positional
    arguments, like:
    
      (gdb) printf "%s\n", $_as_string(a1, 3, 1)
    
    But that's clearly terrible as there's far more print formatting
    options, and if you needed to set the 9th option you'd need to fill in
    all the previous options.
    
    And right now, the only way to pass these options to a gdb.Function is
    to have GDB first convert them all into gdb.Value objects, which is
    really overkill for what we want.
    
    The new %V format solves all these problems: the string is computed
    and printed entirely on the GDB side, we are able to print arrays as
    actual arrays rather than pointers, and we can pass named format
    arguments.
    
    Finally, the $_as_string is sold in the manual as allowing users to
    print the string representation of flag enums, so given:
    
      enum flags
        {
          FLAG_A = (1 << 0),
          FLAG_B = (1 << 1),
          FLAG_C = (1 << 1)
        };
    
      enum flags ff = FLAG_B;
    
    We can:
    
      (gdb) printf "%s\n", $_as_string(ff)
      FLAG_B
    
    This works just fine with %V too:
    
      (gdb) printf "%V\n", ff
      FLAG_B
    
    So all functionality of $_as_string is replaced by %V.  I'm not
    proposing to remove $_as_string, there might be users currently
    depending on it, but I am proposing that we don't push $_as_string in
    the documentation.
    
    Reviewed-By: Eli Zaretskii <eliz@gnu.org>

diff --git a/gdb/NEWS b/gdb/NEWS
index 10a1a70fa52..d84f04b9a88 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -63,6 +63,15 @@
     break foo thread 1 task 1
     watch var thread 2 task 3
 
+* The printf command now accepts a '%V' output format which will
+  format an expression just as the 'print' command would.  Print
+  options can be placed withing '[...]' after the '%V' to modify how
+  the value is printed.  E.g:
+    printf "%V", some_array
+    printf "%V[-array-indexes on]", some_array
+  will print the array without, or with array indexes included, just
+  as the array would be printed by the 'print' command.
+
 * New commands
 
 maintenance print record-instruction [ N ]
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2d5358a792b..b0621b297f1 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -13131,6 +13131,10 @@
 
 @findex $_as_string@r{, convenience function}
 @item $_as_string(@var{value})
+This convenience function is considered deprecated, and could be
+removed from future versions of @value{GDBN}.  Use the @samp{%V} format
+specifier instead (@pxref{%V Format Specifier}).
+
 Return the string representation of @var{value}.
 
 This function is useful to obtain the textual label (enumerator) of an
@@ -29031,6 +29035,36 @@
 printf "D32: %Hf - D64: %Df - D128: %DDf\n",1.2345df,1.2E10dd,1.2E1dl
 @end smallexample
 
+@anchor{%V Format Specifier}
+Additionally, @code{printf} supports a special @samp{%V} output format.
+This format prints the string representation of an expression just as
+@value{GDBN} would produce with the standard @kbd{print} command
+(@pxref{Data, ,Examining Data}):
+
+@smallexample
+(@value{GDBP}) print array
+$1 = @{0, 1, 2, 3, 4, 5@}
+(@value{GDBP}) printf "Array is: %V\n", array
+Array is: @{0, 1, 2, 3, 4, 5@}
+@end smallexample
+
+Additionally, it is possible to include print options with the
+@samp{%V} format by placing them in @samp{[...]} immediately after the
+@samp{%V}, like this:
+
+@smallexample
+(@value{GDBP}) printf "Array is: %V[-array-indexes on]\n", array
+Array is: @{[0] = 0, [1] = 1, [2] = 2, [3] = 3, [4] = 4, [5] = 5@}
+@end smallexample
+
+If you need to print a literal @samp{[} directly after a @samp{%V} then
+just include an empty print options list:
+
+@smallexample
+(@value{GDBP}) printf "Array is: %V[][Hello]\n", array
+Array is: @{0, 1, 2, 3, 4, 5@}[Hello]
+@end smallexample
+
 @anchor{eval}
 @kindex eval
 @item eval @var{template}, @var{expressions}@dots{}
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index dd92e31d31b..7783606fd9a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2733,7 +2733,7 @@ ui_printf (const char *arg, struct ui_file *stream)
   if (*s++ != '"')
     error (_("Bad format string, missing '\"'."));
 
-  format_pieces fpieces (&s);
+  format_pieces fpieces (&s, false, true);
 
   if (*s++ != '"')
     error (_("Bad format string, non-terminated '\"'."));
@@ -2875,6 +2875,36 @@ ui_printf (const char *arg, struct ui_file *stream)
 	  case ptr_arg:
 	    printf_pointer (stream, current_substring, val_args[i]);
 	    break;
+	  case value_arg:
+	    {
+	      value_print_options print_opts;
+	      get_user_print_options (&print_opts);
+
+	      if (current_substring[2] == '[')
+		{
+		  std::string args (&current_substring[3],
+				    strlen (&current_substring[3]) - 1);
+
+		  const char *args_ptr = args.c_str ();
+
+		  /* Override global settings with explicit options, if
+		     any.  */
+		  auto group
+		    = make_value_print_options_def_group (&print_opts);
+		  gdb::option::process_options
+		    (&args_ptr, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR,
+		     group);
+
+		  if (*args_ptr != '\0')
+		    error (_("unexpected content in print options: %s"),
+			     args_ptr);
+		}
+
+	      string_file buffer;
+	      print_formatted (val_args[i], 0, &print_opts, &buffer);
+	      gdb_puts (buffer.string ().c_str ());
+	    }
+	    break;
 	  case literal_piece:
 	    /* Print a portion of the format string that has no
 	       directives.  Note that this will not include any
diff --git a/gdb/testsuite/gdb.base/printcmds.c b/gdb/testsuite/gdb.base/printcmds.c
index 78291a2803c..fa3a62d6cdd 100644
--- a/gdb/testsuite/gdb.base/printcmds.c
+++ b/gdb/testsuite/gdb.base/printcmds.c
@@ -108,6 +108,7 @@ enum flag_enum
   FE_TWO_LEGACY = 0x02,
 };
 
+enum flag_enum one = FE_ONE;
 enum flag_enum three = (enum flag_enum) (FE_ONE | FE_TWO);
 
 /* Another enum considered as a "flag enum", but with no enumerator with value
@@ -152,6 +153,18 @@ struct some_struct
   }
 };
 
+/* This is used in the printf test.  */
+struct small_struct
+{
+  int a;
+  int b;
+  int c;
+} a_small_struct = {
+  1,
+  2,
+  3
+};
+
 /* The following variables are used for testing byte repeat sequences.
    The variable names are encoded: invalid_XYZ where:
    X = start
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 21a2cad458c..10275720e8f 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -957,6 +957,32 @@ proc test_printf_with_dfp {} {
     gdb_test "printf \"%Hf %Hf\\n\",1.2df,1.3df" "1.2 1.3"
 }
 
+# Test the printf '%V' format.
+proc test_printf_V_format {} {
+    # Enums.
+    gdb_test {printf "%V\n", one} "FE_ONE"
+    gdb_test {printf "%V\n", three} "\\(FE_ONE \\| FE_TWO\\)"
+    gdb_test {printf "%V\n", flag_enum_without_zero} "0"
+    gdb_test {printf "%V\n", three_not_flag} "3"
+
+    # Arrays.
+    gdb_test {printf "%V\n", a1} "\\{2, 4, 6, 8, 10, 12, 14, 16, 18, 20\\}"
+    gdb_test {printf "%V[]\n", a1} "\\{2, 4, 6, 8, 10, 12, 14, 16, 18, 20\\}"
+    gdb_test {printf "%V[][]\n", a1} \
+	"\\{2, 4, 6, 8, 10, 12, 14, 16, 18, 20\\}\\\[\\\]"
+    gdb_test {printf "%V[-elements 3]\n", a1} "\\{2, 4, 6\\.\\.\\.\\}"
+    gdb_test {printf "%V[-elements 3][]\n", a1} \
+	"\\{2, 4, 6\\.\\.\\.\\}\\\[\\\]"
+    gdb_test {printf "%V[-elements 3 -array-indexes on]\n", a1} \
+	"\\{\\\[0\\\] = 2, \\\[1\\\] = 4, \\\[2\\\] = 6\\.\\.\\.\\}"
+
+    # Structures.
+    gdb_test {printf "%V\n", a_small_struct} \
+	"\\{a = 1, b = 2, c = 3\\}"
+    gdb_test {printf "%V[-pretty on]\n", a_small_struct} \
+	"\\{\r\n  a = 1,\r\n  b = 2,\r\n  c = 3\r\n\\}"
+}
+
 proc test_print_symbol {} {
     gdb_test_no_output "set print symbol on"
 
@@ -1086,7 +1112,6 @@ proc test_printf_convenience_var {prefix} {
     }
 }
 
-
 clean_restart
 
 gdb_test "print \$pc" "No registers\\."
@@ -1171,6 +1196,7 @@ test_print_array_constants
 test_print_enums
 test_printf
 test_printf_with_dfp
+test_printf_V_format
 test_print_symbol
 test_repeat_bytes
 test_radices
diff --git a/gdbsupport/format.cc b/gdbsupport/format.cc
index 19f37ec8e0c..2edeb5033be 100644
--- a/gdbsupport/format.cc
+++ b/gdbsupport/format.cc
@@ -20,7 +20,8 @@
 #include "common-defs.h"
 #include "format.h"
 
-format_pieces::format_pieces (const char **arg, bool gdb_extensions)
+format_pieces::format_pieces (const char **arg, bool gdb_extensions,
+			      bool value_extension)
 {
   const char *s;
   const char *string;
@@ -44,7 +45,7 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
       char *f = (char *) alloca (strlen (s) + 1);
       string = f;
 
-      while ((gdb_extensions || *s != '"') && *s != '\0')
+      while (*s != '"' && *s != '\0')
 	{
 	  int c = *s++;
 	  switch (c)
@@ -340,6 +341,27 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 	      bad = 1;
 	    break;
 
+	  case 'V':
+	    if (!value_extension)
+	      error (_("Unrecognized format specifier '%c' in printf"), *f);
+
+	    if (lcount > 1 || seen_h || seen_big_h || seen_big_h
+		|| seen_big_d || seen_double_big_d || seen_size_t
+		|| seen_prec || seen_zero || seen_space || seen_plus)
+	      bad = 1;
+
+	    this_argclass = value_arg;
+
+	    if (f[1] == '[')
+	      {
+		const char *tmp;
+		for (tmp = f; *tmp != ']' && *tmp != '\0'; ++tmp)
+		  ;
+		if (*tmp == ']')
+		  f = tmp;
+	      }
+	    break;
+
 	  case '*':
 	    error (_("`*' not supported for precision or width in printf"));
 
diff --git a/gdbsupport/format.h b/gdbsupport/format.h
index 342b473c3ed..2af34ab9450 100644
--- a/gdbsupport/format.h
+++ b/gdbsupport/format.h
@@ -41,7 +41,8 @@ enum argclass
     int_arg, long_arg, long_long_arg, size_t_arg, ptr_arg,
     string_arg, wide_string_arg, wide_char_arg,
     double_arg, long_double_arg,
-    dec32float_arg, dec64float_arg, dec128float_arg
+    dec32float_arg, dec64float_arg, dec128float_arg,
+    value_arg
   };
 
 /* A format piece is a section of the format string that may include a
@@ -75,7 +76,8 @@ class format_pieces
 {
 public:
 
-  format_pieces (const char **arg, bool gdb_extensions = false);
+  format_pieces (const char **arg, bool gdb_extensions = false,
+		 bool value_extension = false);
   ~format_pieces () = default;
 
   DISABLE_COPY_AND_ASSIGN (format_pieces);