Patchwork C++-ify parse_format_string

login
register
mail settings
Submitter Simon Marchi
Date Dec. 2, 2017, 8:31 p.m.
Message ID <20171202203133.10770-1-simon.marchi@polymtl.ca>
Download mbox | patch
Permalink /patch/24693/
State New
Headers show

Comments

Simon Marchi - Dec. 2, 2017, 8:31 p.m.
From: Simon Marchi <simon.marchi@ericsson.com>

I finally got around to polish this patch.  Let me know what you think.

This patch C++ifies the parse_format_string function and its return
value, allowing to get rid of free_format_pieces_cleanup.  Currently,
format_piece::string points into a big buffer shared by all format
pieces resulting of the format string parse.  To make the code and
allocation scheme simpler to understand, this patch makes each piece own
its string with an std::string.

I've tried to measure the impact of this change on the time taken by
parse_format_string in an optimized build.  I ran

  $ perf record -g -- ./gdb -nx -batch -x test.gdb

with test.gdb:

  set $a = 0
  while $a < 20000
    printf "%daaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa<16 times>\n", 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16
    set $a = $a + 1
  end

The purpose of the long string of a's is to have some format pieces for
which the substring doesn't fit in the std::string local buffer.  The
command "perf report" shows this for current master:

  1.81%     1.81%  gdb  gdb  [.] parse_format_string

and this with the patch:

  1.69%     1.69%  gdb  gdb  [.] parse_format_string

So it doesn't seem like it makes much of a difference.  If you have tips
on how to use perf to get more precise measurements, I would be glad to
hear them.

gdb/ChangeLog:

	* common/format.h (struct format_piece): Add constructor.
	<string>: Change type to std::string.
	(parse_format_string): Return an std::vector of format_piece.
	(free_format_pieces): Remove.
	(free_format_pieces_cleanup): Remove.
	* common/format.c (parse_format_string): Return an std::vector of
	format_piece.  Don't allocate a big buffer, adjust to those
	changes.
	(free_format_pieces): Remove.
	(free_format_pieces_cleanup): Remove.
	* ax-gdb.c (gen_printf): Remove format_piece * parameter.
	(maint_agent_printf_command): Adjust to parse_format_string
	changes.
	* ax-gdb.h (struct format_piece): Don't forward-declare.
	(gen_printf): Remove format_piece * parameter.
	* breakpoint.c (parse_cmd_to_aexpr): Adjust to
	parse_format_string chagnes.
	* printcmd.c (ui_printf): Likewise.

gdb/gdbserver/ChangeLog:

	* ax.c (ax_printf): Adjust to parse_format_string changes.
---
 gdb/ax-gdb.c        | 22 ++++--------
 gdb/ax-gdb.h        |  2 --
 gdb/breakpoint.c    | 24 ++++---------
 gdb/common/format.c | 99 +++++++----------------------------------------------
 gdb/common/format.h | 19 ++++------
 gdb/gdbserver/ax.c  | 34 +++++++++---------
 gdb/printcmd.c      | 41 ++++++++++------------
 7 files changed, 67 insertions(+), 174 deletions(-)
Pedro Alves - Dec. 3, 2017, 2:12 p.m.
On 12/02/2017 08:31 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@ericsson.com>
> 
> I finally got around to polish this patch.  Let me know what you think.
> 
> This patch C++ifies the parse_format_string function and its return
> value, allowing to get rid of free_format_pieces_cleanup.  Currently,
> format_piece::string points into a big buffer shared by all format
> pieces resulting of the format string parse.  To make the code and
> allocation scheme simpler to understand, this patch makes each piece own
> its string with an std::string.
> 
> I've tried to measure the impact of this change on the time taken by
> parse_format_string in an optimized build.  I ran
> 
>   $ perf record -g -- ./gdb -nx -batch -x test.gdb

I like using:

 $ perf record --call-graph dwarf,65528 -- ....

so that perf report has more accurate callchain info.

Otherwise, compile with -fno-omit-frame-pointer (but that's no
longer testing what you normally compile to).

> 
> with test.gdb:
> 
>   set $a = 0
>   while $a < 20000
>     printf "%daaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa<16 times>\n", 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16
>     set $a = $a + 1
>   end
> 
> The purpose of the long string of a's is to have some format pieces for
> which the substring doesn't fit in the std::string local buffer.  The
> command "perf report" shows this for current master:
> 
>   1.81%     1.81%  gdb  gdb  [.] parse_format_string
> 
> and this with the patch:
> 
>   1.69%     1.69%  gdb  gdb  [.] parse_format_string
> 
> So it doesn't seem like it makes much of a difference.  If you have tips
> on how to use perf to get more precise measurements, I would be glad to
> hear them.

Use plain "time" first.  (You may have shifted work
out of parse_format_string.)

With your test script as is, I get around:

 real    0m1.053s
 user    0m0.985s
 sys     0m0.068s

I like a testcase that runs a bit longer in order to have a
better signal/noise ratio.  IMO 1 second is too short.  Bumping the
number of iterations 10x (to 200000), I get (representative
of a few runs),

before patch:

 real    0m9.432s
 user    0m8.978s
 sys     0m0.444s

after patch:

 real    0m10.322s
 user    0m9.854s
 sys     0m0.454s

there's some variation across runs, but after-patch compared
to before-patch is consistently around 7%-11% slower, for me.

Looking at "perf report -g", we see we're very inefficient
while handling this specific use case, spending a lot of time
in the C parser and in fprintf_filtered.  If those paths
were tuned a bit the relative slowdown of parse_format_string
would probably be more pronounced.

Note that parse_format_string is used to implement target-side
dprintf, in gdbserver/ax.c:ax_printf.  This means that a slow
down here affects inferior execution time too, which may be
a less-contrived scenario.

My point is still the same as ever -- it's not possible
upfront to envision all the use cases users throw at us,
given gdb scripting, etc.  So if easy, I think it's better
to avoid premature pessimization:

 https://sourceware.org/ml/gdb-patches/2017-06/msg00506.html

(Note: I have not really looked at the patch in any detail.)

Thanks,
Pedro Alves
Simon Marchi - Dec. 3, 2017, 5:49 p.m.
On 2017-12-03 09:12, Pedro Alves wrote:
> Use plain "time" first.  (You may have shifted work
> out of parse_format_string.)
> 
> With your test script as is, I get around:
> 
>  real    0m1.053s
>  user    0m0.985s
>  sys     0m0.068s
> 
> I like a testcase that runs a bit longer in order to have a
> better signal/noise ratio.  IMO 1 second is too short.  Bumping the
> number of iterations 10x (to 200000), I get (representative
> of a few runs),
> 
> before patch:
> 
>  real    0m9.432s
>  user    0m8.978s
>  sys     0m0.444s
> 
> after patch:
> 
>  real    0m10.322s
>  user    0m9.854s
>  sys     0m0.454s
> 
> there's some variation across runs, but after-patch compared
> to before-patch is consistently around 7%-11% slower, for me.

Indeed, I see similar results (except 200000 iterations take 30 seconds 
for me with my 10 years old CPU :)).

> Looking at "perf report -g", we see we're very inefficient
> while handling this specific use case, spending a lot of time
> in the C parser and in fprintf_filtered.  If those paths
> were tuned a bit the relative slowdown of parse_format_string
> would probably be more pronounced.
> 
> Note that parse_format_string is used to implement target-side
> dprintf, in gdbserver/ax.c:ax_printf.  This means that a slow
> down here affects inferior execution time too, which may be
> a less-contrived scenario.

I agree that we should not make parse_format_string slow just for fun, 
but in this particular case I think we should avoid parsing the format 
string on the fast path.  It should be possible to chew it to pieces 
earlier (when installing the breakpoint) and just access the pieces when 
we hit the dprintf.

> My point is still the same as ever -- it's not possible
> upfront to envision all the use cases users throw at us,
> given gdb scripting, etc.  So if easy, I think it's better
> to avoid premature pessimization:
> 
>  https://sourceware.org/ml/gdb-patches/2017-06/msg00506.html

The article you link defines premature pessimization as "when 
equivalently complex code would be faster".  In this case, the code is 
more complex (though maybe not by much) with a shared buffer, and I was 
wondering if that complexity was really worth the speedup, especially 
for something not expected to be on a fast path.  But since it's already 
implemented like this and we know it's faster, I agree it feels like a 
step backwards to remove it.  So I have no problem going with Tom's 
patch.

Simon
Tom Tromey - Dec. 8, 2017, 4:22 p.m.
Simon> So I have no problem going with Tom's patch.

Thanks for following up on this.
I'll push it soon.

Tom

Patch

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 5027f6a464..6c0d0f29f9 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -2541,7 +2541,6 @@  agent_expr_up
 gen_printf (CORE_ADDR scope, struct gdbarch *gdbarch,
 	    CORE_ADDR function, LONGEST channel,
 	    const char *format, int fmtlen,
-	    struct format_piece *frags,
 	    int nargs, struct expression **exprs)
 {
   agent_expr_up ax (new agent_expr (gdbarch, scope));
@@ -2681,12 +2680,7 @@  agent_eval_command (const char *exp, int from_tty)
 static void
 maint_agent_printf_command (const char *cmdrest, int from_tty)
 {
-  struct cleanup *old_chain = 0;
-  struct expression *argvec[100];
   struct frame_info *fi = get_current_frame ();	/* need current scope */
-  const char *format_start, *format_end;
-  struct format_piece *fpieces;
-  int nargs;
 
   /* We don't deal with overlay debugging at the moment.  We need to
      think more carefully about this.  If you copy this code into
@@ -2703,13 +2697,11 @@  maint_agent_printf_command (const char *cmdrest, int from_tty)
   if (*cmdrest++ != '"')
     error (_("Must start with a format string."));
 
-  format_start = cmdrest;
+  const char *format_start = cmdrest;
 
-  fpieces = parse_format_string (&cmdrest);
+  parse_format_string (&cmdrest);
 
-  old_chain = make_cleanup (free_format_pieces_cleanup, &fpieces);
-
-  format_end = cmdrest;
+  const char *format_end = cmdrest;
 
   if (*cmdrest++ != '"')
     error (_("Bad format string, non-terminated '\"'."));
@@ -2723,15 +2715,14 @@  maint_agent_printf_command (const char *cmdrest, int from_tty)
     cmdrest++;
   cmdrest = skip_spaces (cmdrest);
 
-  nargs = 0;
+  std::vector<struct expression *> argvec;
   while (*cmdrest != '\0')
     {
       const char *cmd1;
 
       cmd1 = cmdrest;
       expression_up expr = parse_exp_1 (&cmd1, 0, (struct block *) 0, 1);
-      argvec[nargs] = expr.release ();
-      ++nargs;
+      argvec.push_back (expr.release ());
       cmdrest = cmd1;
       if (*cmdrest == ',')
 	++cmdrest;
@@ -2742,14 +2733,13 @@  maint_agent_printf_command (const char *cmdrest, int from_tty)
   agent_expr_up agent = gen_printf (get_frame_pc (fi), get_current_arch (),
 				    0, 0,
 				    format_start, format_end - format_start,
-				    fpieces, nargs, argvec);
+				    argvec.size (), argvec.data ());
   ax_reqs (agent.get ());
   ax_print (gdb_stdout, agent.get ());
 
   /* It would be nice to call ax_reqs here to gather some general info
      about the expression, and then print out the result.  */
 
-  do_cleanups (old_chain);
   dont_repeat ();
 }
 
diff --git a/gdb/ax-gdb.h b/gdb/ax-gdb.h
index 8b5ab46c66..834ddffe05 100644
--- a/gdb/ax-gdb.h
+++ b/gdb/ax-gdb.h
@@ -120,10 +120,8 @@  extern void gen_expr (struct expression *exp, union exp_element **pc,
 
 extern void require_rvalue (struct agent_expr *ax, struct axs_value *value);
 
-struct format_piece;
 extern agent_expr_up gen_printf (CORE_ADDR, struct gdbarch *,
 				 CORE_ADDR, LONGEST, const char *, int,
-				 struct format_piece *,
 				 int, struct expression **);
 
 #endif /* AX_GDB_H */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d4d095d87e..065ac002f7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2230,12 +2230,7 @@  build_target_condition_list (struct bp_location *bl)
 static agent_expr_up
 parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
 {
-  struct cleanup *old_cleanups = 0;
-  struct expression **argvec;
   const char *cmdrest;
-  const char *format_start, *format_end;
-  struct format_piece *fpieces;
-  int nargs;
   struct gdbarch *gdbarch = get_current_arch ();
 
   if (cmd == NULL)
@@ -2250,13 +2245,11 @@  parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
   if (*cmdrest++ != '"')
     error (_("No format string following the location"));
 
-  format_start = cmdrest;
+  const char *format_start = cmdrest;
 
-  fpieces = parse_format_string (&cmdrest);
+  parse_format_string (&cmdrest);
 
-  old_cleanups = make_cleanup (free_format_pieces_cleanup, &fpieces);
-
-  format_end = cmdrest;
+  const char *format_end = cmdrest;
 
   if (*cmdrest++ != '"')
     error (_("Bad format string, non-terminated '\"'."));
@@ -2272,17 +2265,14 @@  parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
 
   /* For each argument, make an expression.  */
 
-  argvec = (struct expression **) alloca (strlen (cmd)
-					 * sizeof (struct expression *));
-
-  nargs = 0;
+  std::vector<struct expression *> argvec;
   while (*cmdrest != '\0')
     {
       const char *cmd1;
 
       cmd1 = cmdrest;
       expression_up expr = parse_exp_1 (&cmd1, scope, block_for_pc (scope), 1);
-      argvec[nargs++] = expr.release ();
+      argvec.push_back (expr.release ());
       cmdrest = cmd1;
       if (*cmdrest == ',')
 	++cmdrest;
@@ -2296,7 +2286,7 @@  parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
     {
       aexpr = gen_printf (scope, gdbarch, 0, 0,
 			  format_start, format_end - format_start,
-			  fpieces, nargs, argvec);
+			  argvec.size (), argvec.data ());
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
@@ -2306,8 +2296,6 @@  parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
     }
   END_CATCH
 
-  do_cleanups (old_cleanups);
-
   /* We have a valid agent expression, return it.  */
   return aexpr;
 }
diff --git a/gdb/common/format.c b/gdb/common/format.c
index 8cb15511fa..b0c7268c8f 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -20,17 +20,14 @@ 
 #include "common-defs.h"
 #include "format.h"
 
-struct format_piece *
+std::vector<format_piece>
 parse_format_string (const char **arg)
 {
   const char *s;
   char *f, *string;
   const char *prev_start;
   const char *percent_loc;
-  char *sub_start, *current_substring;
-  struct format_piece *pieces;
-  int next_frag;
-  int max_pieces;
+  std::vector<format_piece> pieces;
   enum argclass this_argclass;
 
   s = *arg;
@@ -97,16 +94,6 @@  parse_format_string (const char **arg)
      done with it; it's up to callers to complain about syntax.  */
   *arg = s;
 
-  /* Need extra space for the '\0's.  Doubling the size is sufficient.  */
-
-  current_substring = (char *) xmalloc (strlen (string) * 2 + 1000);
-
-  max_pieces = strlen (string) + 2;
-
-  pieces = XNEWVEC (struct format_piece, max_pieces);
-
-  next_frag = 0;
-
   /* Now scan the string for %-specs and see what kinds of args they want.
      argclass classifies the %-specs so we can give printf-type functions
      something of the right size.  */
@@ -129,15 +116,8 @@  parse_format_string (const char **arg)
 	    continue;
 	  }
 
-	sub_start = current_substring;
-
-	strncpy (current_substring, prev_start, f - 1 - prev_start);
-	current_substring += f - 1 - prev_start;
-	*current_substring++ = '\0';
-
-	pieces[next_frag].string = sub_start;
-	pieces[next_frag].argclass = literal_piece;
-	next_frag++;
+	pieces.emplace_back (std::string (prev_start, f - 1 - prev_start),
+			     literal_piece);
 
 	percent_loc = f - 1;
 
@@ -309,7 +289,7 @@  parse_format_string (const char **arg)
 
 	f++;
 
-	sub_start = current_substring;
+	std::string piece_string;
 
 	if (lcount > 1 && USE_PRINTF_I64)
 	  {
@@ -317,11 +297,9 @@  parse_format_string (const char **arg)
 	       Convert %lld to %I64d.  */
 	    int length_before_ll = f - percent_loc - 1 - lcount;
 
-	    strncpy (current_substring, percent_loc, length_before_ll);
-	    strcpy (current_substring + length_before_ll, "I64");
-	    current_substring[length_before_ll + 3] =
-	      percent_loc[length_before_ll + lcount];
-	    current_substring += length_before_ll + 4;
+	    piece_string.assign (percent_loc, length_before_ll);
+	    piece_string += "I64";
+	    piece_string += percent_loc[length_before_ll + lcount];
 	  }
 	else if (this_argclass == wide_string_arg
 		 || this_argclass == wide_char_arg)
@@ -329,71 +307,20 @@  parse_format_string (const char **arg)
 	    /* Convert %ls or %lc to %s.  */
 	    int length_before_ls = f - percent_loc - 2;
 
-	    strncpy (current_substring, percent_loc, length_before_ls);
-	    strcpy (current_substring + length_before_ls, "s");
-	    current_substring += length_before_ls + 2;
+	    piece_string.assign (percent_loc, length_before_ls);
+	    piece_string += 's';
 	  }
 	else
-	  {
-	    strncpy (current_substring, percent_loc, f - percent_loc);
-	    current_substring += f - percent_loc;
-	  }
-
-	*current_substring++ = '\0';
+	  piece_string.assign (percent_loc, f - percent_loc);
 
 	prev_start = f;
 
-	pieces[next_frag].string = sub_start;
-	pieces[next_frag].argclass = this_argclass;
-	next_frag++;
+	pieces.emplace_back (std::move (piece_string), this_argclass);
       }
 
   /* Record the remainder of the string.  */
 
-  sub_start = current_substring;
-
-  strncpy (current_substring, prev_start, f - prev_start);
-  current_substring += f - prev_start;
-  *current_substring++ = '\0';
-
-  pieces[next_frag].string = sub_start;
-  pieces[next_frag].argclass = literal_piece;
-  next_frag++;
-
-  /* Record an end-of-array marker.  */
-
-  pieces[next_frag].string = NULL;
-  pieces[next_frag].argclass = literal_piece;
+  pieces.emplace_back (std::string (prev_start, f - prev_start), literal_piece);
 
   return pieces;
 }
-
-void
-free_format_pieces (struct format_piece *pieces)
-{
-  if (!pieces)
-    return;
-
-  /* We happen to know that all the string pieces are in the block
-     pointed to by the first string piece.  */
-  if (pieces[0].string)
-    xfree (pieces[0].string);
-
-  xfree (pieces);
-}
-
-void
-free_format_pieces_cleanup (void *ptr)
-{
-  struct format_piece **location = (struct format_piece **) ptr;
-
-  if (location == NULL)
-    return;
-
-  if (*location != NULL)
-    {
-      free_format_pieces (*location);
-      *location = NULL;
-    }
-}
-
diff --git a/gdb/common/format.h b/gdb/common/format.h
index f3a94b8bbb..558f719efe 100644
--- a/gdb/common/format.h
+++ b/gdb/common/format.h
@@ -48,22 +48,17 @@  enum argclass
 
 struct format_piece
 {
-  char *string;
+  format_piece (std::string &&string_, enum argclass argclass_)
+  : string (std::move (string_)), argclass (argclass_)
+  {}
+
+  std::string string;
   enum argclass argclass;
 };
 
-/* Return an array of printf fragments found at the given string, and
+/* Return a vector of printf fragments found at the given string, and
    rewrite ARG with a pointer to the end of the format string.  */
 
-extern struct format_piece *parse_format_string (const char **arg);
-
-/* Given a pointer to an array of format pieces, free any memory that
-   would have been allocated by parse_format_string.  */
-
-extern void free_format_pieces (struct format_piece *frags);
-
-/* Freeing, cast as a cleanup.  */
-
-extern void free_format_pieces_cleanup (void *);
+extern std::vector<format_piece> parse_format_string (const char **arg);
 
 #endif /* COMMON_FORMAT_H */
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 35ed2c69ad..46a1399e0b 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -816,30 +816,29 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	   int nargs, ULONGEST *args)
 {
   const char *f = format;
-  struct format_piece *fpieces;
-  int i, fp;
-  char *current_substring;
+  int i;
   int nargs_wanted;
 
   ax_debug ("Printf of \"%s\" with %d args", format, nargs);
 
-  fpieces = parse_format_string (&f);
+  std::vector<format_piece> fpieces = parse_format_string (&f);
 
   nargs_wanted = 0;
-  for (fp = 0; fpieces[fp].string != NULL; fp++)
-    if (fpieces[fp].argclass != literal_piece)
+  for (const format_piece &piece : fpieces)
+    if (piece.argclass != literal_piece)
       ++nargs_wanted;
 
   if (nargs != nargs_wanted)
     error (_("Wrong number of arguments for specified format-string"));
 
   i = 0;
-  for (fp = 0; fpieces[fp].string != NULL; fp++)
+  for (const format_piece &piece : fpieces)
     {
-      current_substring = fpieces[fp].string;
+      const char *fmt = piece.string.c_str ();
+
       ax_debug ("current substring is '%s', class is %d",
-		current_substring, fpieces[fp].argclass);
-      switch (fpieces[fp].argclass)
+		fmt, piece.argclass);
+      switch (piece.argclass)
 	{
 	case string_arg:
 	  {
@@ -865,7 +864,7 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 		read_inferior_memory (tem, str, j);
 	      str[j] = 0;
 
-              printf (current_substring, (char *) str);
+              printf (fmt, (char *) str);
 	    }
 	    break;
 
@@ -874,7 +873,7 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	    {
 	      long long val = args[i];
 
-              printf (current_substring, val);
+              printf (fmt, val);
 	      break;
 	    }
 #else
@@ -884,7 +883,7 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	  {
 	    int val = args[i];
 
-	    printf (current_substring, val);
+	    printf (fmt, val);
 	    break;
 	  }
 
@@ -892,7 +891,7 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	  {
 	    long val = args[i];
 
-	    printf (current_substring, val);
+	    printf (fmt, val);
 	    break;
 	  }
 
@@ -905,20 +904,19 @@  ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	     have modified GCC to include -Wformat-security by
 	     default, which will warn here if there is no
 	     argument.  */
-	  printf (current_substring, 0);
+	  printf (fmt, 0);
 	  break;
 
 	default:
 	  error (_("Format directive in '%s' not supported in agent printf"),
-		 current_substring);
+		 fmt);
 	}
 
       /* Maybe advance to the next argument.  */
-      if (fpieces[fp].argclass != literal_piece)
+      if (piece.argclass != literal_piece)
 	++i;
     }
 
-  free_format_pieces (fpieces);
   fflush (stdout);
 }
 
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 5d4417fa06..850d5ec9de 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2427,7 +2427,6 @@  printf_pointer (struct ui_file *stream, const char *format,
 static void
 ui_printf (const char *arg, struct ui_file *stream)
 {
-  struct format_piece *fpieces;
   const char *s = arg;
   struct value **val_args;
   int allocated_args = 20;
@@ -2447,9 +2446,7 @@  ui_printf (const char *arg, struct ui_file *stream)
   if (*s++ != '"')
     error (_("Bad format string, missing '\"'."));
 
-  fpieces = parse_format_string (&s);
-
-  make_cleanup (free_format_pieces_cleanup, &fpieces);
+  std::vector<format_piece> fpieces = parse_format_string (&s);
 
   if (*s++ != '"')
     error (_("Bad format string, non-terminated '\"'."));
@@ -2466,12 +2463,11 @@  ui_printf (const char *arg, struct ui_file *stream)
   {
     int nargs = 0;
     int nargs_wanted;
-    int i, fr;
-    char *current_substring;
+    int i;
 
     nargs_wanted = 0;
-    for (fr = 0; fpieces[fr].string != NULL; fr++)
-      if (fpieces[fr].argclass != literal_piece)
+    for (const format_piece &piece : fpieces)
+      if (piece.argclass != literal_piece)
 	++nargs_wanted;
 
     /* Now, parse all arguments and evaluate them.
@@ -2499,16 +2495,17 @@  ui_printf (const char *arg, struct ui_file *stream)
 
     /* Now actually print them.  */
     i = 0;
-    for (fr = 0; fpieces[fr].string != NULL; fr++)
+    for (const format_piece &piece : fpieces)
       {
-	current_substring = fpieces[fr].string;
-	switch (fpieces[fr].argclass)
+	const char *fmt = piece.string.c_str ();
+
+	switch (piece.argclass)
 	  {
 	  case string_arg:
-	    printf_c_string (stream, current_substring, val_args[i]);
+	    printf_c_string (stream, fmt, val_args[i]);
 	    break;
 	  case wide_string_arg:
-	    printf_wide_c_string (stream, current_substring, val_args[i]);
+	    printf_wide_c_string (stream, fmt, val_args[i]);
 	    break;
 	  case wide_char_arg:
 	    {
@@ -2535,7 +2532,7 @@  ui_printf (const char *arg, struct ui_file *stream)
 					 &output, translit_char);
 	      obstack_grow_str0 (&output, "");
 
-	      fprintf_filtered (stream, current_substring,
+	      fprintf_filtered (stream, fmt,
                                 obstack_base (&output));
 	    }
 	    break;
@@ -2544,7 +2541,7 @@  ui_printf (const char *arg, struct ui_file *stream)
 	    {
 	      long long val = value_as_long (val_args[i]);
 
-              fprintf_filtered (stream, current_substring, val);
+              fprintf_filtered (stream, fmt, val);
 	      break;
 	    }
 #else
@@ -2554,14 +2551,14 @@  ui_printf (const char *arg, struct ui_file *stream)
 	    {
 	      int val = value_as_long (val_args[i]);
 
-              fprintf_filtered (stream, current_substring, val);
+              fprintf_filtered (stream, fmt, val);
 	      break;
 	    }
 	  case long_arg:
 	    {
 	      long val = value_as_long (val_args[i]);
 
-              fprintf_filtered (stream, current_substring, val);
+              fprintf_filtered (stream, fmt, val);
 	      break;
 	    }
 	  /* Handles floating-point values.  */
@@ -2570,11 +2567,11 @@  ui_printf (const char *arg, struct ui_file *stream)
 	  case dec32float_arg:
 	  case dec64float_arg:
 	  case dec128float_arg:
-	    printf_floating (stream, current_substring, val_args[i],
-			     fpieces[fr].argclass);
+	    printf_floating (stream, fmt, val_args[i],
+			     piece.argclass);
 	    break;
 	  case ptr_arg:
-	    printf_pointer (stream, current_substring, val_args[i]);
+	    printf_pointer (stream, fmt, val_args[i]);
 	    break;
 	  case literal_piece:
 	    /* Print a portion of the format string that has no
@@ -2585,14 +2582,14 @@  ui_printf (const char *arg, struct ui_file *stream)
 	       have modified GCC to include -Wformat-security by
 	       default, which will warn here if there is no
 	       argument.  */
-	    fprintf_filtered (stream, current_substring, 0);
+	    fprintf_filtered (stream, fmt, 0);
 	    break;
 	  default:
 	    internal_error (__FILE__, __LINE__,
 			    _("failed internal consistency check"));
 	  }
 	/* Maybe advance to the next argument.  */
-	if (fpieces[fr].argclass != literal_piece)
+	if (piece.argclass != literal_piece)
 	  ++i;
       }
   }