gdbsupport: remove xmalloc in format_pieces

Remove the use of xmalloc (and the arbitrary allocation size) in
format_pieces.  This turned out a bit more involved than expected, but
not too bad.

format_pieces::m_storage is a buffer with multiple concatenated
null-terminated strings, referenced by format_piece::string.  Change
this to an std::string, while keeping its purpose (use the std::string
as a buffer with embedded null characters).

However, because the std::string's internal buffer can be reallocated as
it grows, and I do not want to hardcode a big reserved size like we have
now, it's not possible to store the direct pointer to the string in
format_piece::string.  Those pointers would become stale as the buffer
gets reallocated.  Therefore, change format_piece to hold an index into
the storage instead.  Add format_pieces::piece_str for the callers to be
able to access the piece's string.  This requires changing the few
callers, but in a trivial way.

The selftest also needs to be updated.  I want to keep the test cases
as-is, where the expected pieces contain the expected string, and not
hard-code an expected index.  To achieve this, add the
expected_format_piece structure.  Note that the previous
format_piece::operator== didn't compare the n_int_args fields, while the
test provides expected values for that field.  I guess that was a
mistake.  The new code checks it, and the test still passes.

Change-Id: I80630ff60e01c8caaa800ae22f69a9a7660bc9e9
Reviewed-By: Keith Seitz <keiths@redhat.com>

gdb/printcmd.c

@@ -2703,7 +2703,6 @@ ui_printf (const char *arg, struct ui_file *stream)
   {
     int nargs_wanted;
     int i;
-    const char *current_substring;
 
     nargs_wanted = 0;
     for (auto &&piece : fpieces)
@@ -2732,7 +2731,8 @@ ui_printf (const char *arg, struct ui_file *stream)
     i = 0;
     for (auto &&piece : fpieces)
       {
-	current_substring = piece.string;
+	const char *current_substring = fpieces.piece_str (piece);
+
 	switch (piece.argclass)
 	  {
 	  case string_arg:

gdb/ui-out.c

@@ -586,7 +586,7 @@ ui_out::vmessage (const ui_file_style &in_style, const char *format,
 
   for (auto &&piece : fpieces)
     {
-      const char *current_substring = piece.string;
+      const char *current_substring = fpieces.piece_str (piece);
 
       gdb_assert (piece.n_int_args >= 0 && piece.n_int_args <= 2);
       int intvals[2] = { 0, 0 };

gdb/unittests/format_pieces-selftests.c

@@ -29,17 +29,35 @@
 namespace selftests {
 namespace format_pieces {
 
+/* Like format_piece, but with the expected string hardcoded instead of an
+   index.  */
+
+struct expected_format_piece
+{
+  std::string_view str;
+  enum argclass argclass;
+  int n_int_args;
+};
+
 /* Verify that parsing STR gives pieces equal to EXPECTED_PIECES.  */
 
 static void
-check (const char *str, const std::vector<format_piece> &expected_pieces,
+check (const char *str,
+       const std::vector<expected_format_piece> &expected_pieces,
        bool gdb_format = false)
 {
   ::format_pieces pieces (&str, gdb_format);
 
   SELF_CHECK ((pieces.end () - pieces.begin ()) == expected_pieces.size ());
-  SELF_CHECK (std::equal (pieces.begin (), pieces.end (),
+
-			  expected_pieces.begin ()));
+  for (auto it = pieces.begin (); it != pieces.end (); ++it)
+    {
+      auto &expected = expected_pieces[it - pieces.begin ()];
+
+      SELF_CHECK (expected.str == pieces.piece_str (*it));
+      SELF_CHECK (expected.argclass == it->argclass);
+      SELF_CHECK (expected.n_int_args == it->n_int_args);
+    }
 }
 
 static void
@@ -47,7 +65,7 @@ test_escape_sequences ()
 {
   check ("This is an escape sequence: \\e",
     {
-      format_piece ("This is an escape sequence: \e", literal_piece, 0),
+      {"This is an escape sequence: \e", literal_piece, 0},
     });
 }
 
@@ -58,11 +76,11 @@ test_format_specifier ()
      see a trailing empty literal piece.  */
   check ("Hello\\t %d%llx%%d%d", /* ARI: %ll */
     {
-      format_piece ("Hello\t ", literal_piece, 0),
+      {"Hello\t ", literal_piece, 0},
-      format_piece ("%d", int_arg, 0),
+      {"%d", int_arg, 0},
-      format_piece ("%" LL "x", long_long_arg, 0),
+      {"%" LL "x", long_long_arg, 0},
-      format_piece ("%%d", literal_piece, 0),
+      {"%%d", literal_piece, 0},
-      format_piece ("%d", int_arg, 0),
+      {"%d", int_arg, 0},
     });
 }
 
@@ -71,13 +89,13 @@ test_gdb_formats ()
 {
   check ("Hello\\t \"%p[%pF%ps%*.*d%p]\"",
     {
-      format_piece ("Hello\\t \"", literal_piece, 0),
+      {"Hello\\t \"", literal_piece, 0},
-      format_piece ("%p[", ptr_arg, 0),
+      {"%p[", ptr_arg, 0},
-      format_piece ("%pF", ptr_arg, 0),
+      {"%pF", ptr_arg, 0},
-      format_piece ("%ps", ptr_arg, 0),
+      {"%ps", ptr_arg, 0},
-      format_piece ("%*.*d", int_arg, 2),
+      {"%*.*d", int_arg, 2},
-      format_piece ("%p]", ptr_arg, 0),
+      {"%p]", ptr_arg, 0},
-      format_piece ("\"", literal_piece, 0),
+      {"\"", literal_piece, 0},
     }, true);
 }
 
@@ -89,38 +107,38 @@ test_format_int_sizes ()
 {
   check ("Hello\\t %hu%lu%llu%zu", /* ARI: %ll */
     {
-      format_piece ("Hello\t ", literal_piece, 0),
+      {"Hello\t ", literal_piece, 0},
-      format_piece ("%hu", int_arg, 0),
+      {"%hu", int_arg, 0},
-      format_piece ("%lu", long_arg, 0),
+      {"%lu", long_arg, 0},
-      format_piece ("%" LL "u", long_long_arg, 0),
+      {"%" LL "u", long_long_arg, 0},
-      format_piece ("%zu", size_t_arg, 0)
+      {"%zu", size_t_arg, 0},
     });
 
   check ("Hello\\t %hx%lx%llx%zx", /* ARI: %ll */
     {
-      format_piece ("Hello\t ", literal_piece, 0),
+      {"Hello\t ", literal_piece, 0},
-      format_piece ("%hx", int_arg, 0),
+      {"%hx", int_arg, 0},
-      format_piece ("%lx", long_arg, 0),
+      {"%lx", long_arg, 0},
-      format_piece ("%" LL "x", long_long_arg, 0),
+      {"%" LL "x", long_long_arg, 0},
-      format_piece ("%zx", size_t_arg, 0)
+      {"%zx", size_t_arg, 0},
     });
 
   check ("Hello\\t %ho%lo%llo%zo", /* ARI: %ll */
     {
-      format_piece ("Hello\t ", literal_piece, 0),
+      {"Hello\t ", literal_piece, 0},
-      format_piece ("%ho", int_arg, 0),
+      {"%ho", int_arg, 0},
-      format_piece ("%lo", long_arg, 0),
+      {"%lo", long_arg, 0},
-      format_piece ("%" LL "o", long_long_arg, 0),
+      {"%" LL "o", long_long_arg, 0},
-      format_piece ("%zo", size_t_arg, 0)
+      {"%zo", size_t_arg, 0},
     });
 
   check ("Hello\\t %hd%ld%lld%zd", /* ARI: %ll */
     {
-      format_piece ("Hello\t ", literal_piece, 0),
+      {"Hello\t ", literal_piece, 0},
-      format_piece ("%hd", int_arg, 0),
+      {"%hd", int_arg, 0},
-      format_piece ("%ld", long_arg, 0),
+      {"%ld", long_arg, 0},
-      format_piece ("%" LL "d", long_long_arg, 0),
+      {"%" LL "d", long_long_arg, 0},
-      format_piece ("%zd", size_t_arg, 0)
+      {"%zd", size_t_arg, 0},
     });
 }
 
@@ -129,8 +147,8 @@ test_windows_formats ()
 {
   check ("rc%I64d",
     {
-     format_piece ("rc", literal_piece, 0),
+     {"rc", literal_piece, 0},
-     format_piece ("%I64d", long_long_arg, 0),
+     {"%I64d", long_long_arg, 0},
     });
 }
 

gdbserver/ax.cc

@@ -817,7 +817,6 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 {
   const char *f = format;
   int i;
-  const char *current_substring;
   int nargs_wanted;
 
   ax_debug ("Printf of \"%s\" with %d args", format, nargs);
@@ -835,7 +834,8 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
   i = 0;
   for (auto &&piece : fpieces)
     {
-      current_substring = piece.string;
+      const char *current_substring = fpieces.piece_str (piece);
+
       ax_debug ("current substring is '%s', class is %d",
 		current_substring, piece.argclass);
       switch (piece.argclass)

gdbsupport/format.cc

@@ -97,10 +97,6 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
       *arg = s;
     }
 
-  /* Need extra space for the '\0's.  Doubling the size is sufficient.  */
-  char *current_substring = (char *) xmalloc (strlen (string) * 2 + 1000);
-  m_storage.reset (current_substring);
-
   /* 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.  */
@@ -126,13 +122,12 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
 	    continue;
 	  }
 
-	const char *sub_start = current_substring;
+	std::string::size_type sub_start = m_storage.size ();
 
-	strncpy (current_substring, prev_start, f - 1 - prev_start);
+	m_storage.append (prev_start, f - 1 - prev_start);
-	current_substring += f - 1 - prev_start;
+	m_storage += '\0';
-	*current_substring++ = '\0';
 
-	if (*sub_start != '\0')
+	if (m_storage[sub_start] != '\0')
 	  m_pieces.emplace_back (sub_start, literal_piece, 0);
 
 	const char *percent_loc = f - 1;
@@ -374,7 +369,7 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
 
 	f++;
 
-	sub_start = current_substring;
+	sub_start = m_storage.size ();
 
 	if (lcount > 1 && !seen_i64 && USE_PRINTF_I64)
 	  {
@@ -382,11 +377,9 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
 	       Convert %lld to %I64d.  */
 	    int length_before_ll = f - percent_loc - 1 - lcount;
 
-	    strncpy (current_substring, percent_loc, length_before_ll);
+	    m_storage.append (percent_loc, length_before_ll);
-	    strcpy (current_substring + length_before_ll, "I64");
+	    m_storage += "I64";
-	    current_substring[length_before_ll + 3] =
+	    m_storage += percent_loc[length_before_ll + lcount];
-	      percent_loc[length_before_ll + lcount];
-	    current_substring += length_before_ll + 4;
 	  }
 	else if (this_argclass == wide_string_arg
 		 || this_argclass == wide_char_arg)
@@ -394,18 +387,13 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
 	    /* Convert %ls or %lc to %s.  */
 	    int length_before_ls = f - percent_loc - 2;
 
-	    strncpy (current_substring, percent_loc, length_before_ls);
+	    m_storage.append (percent_loc, length_before_ls);
-	    strcpy (current_substring + length_before_ls, "s");
+	    m_storage += "s";
-	    current_substring += length_before_ls + 2;
 	  }
 	else
-	  {
+	  m_storage.append (percent_loc, f - percent_loc);
-	    strncpy (current_substring, percent_loc, f - percent_loc);
-	    current_substring += f - percent_loc;
-	  }
-
-	*current_substring++ = '\0';
 
+	m_storage += '\0';
 	prev_start = f;
 
 	m_pieces.emplace_back (sub_start, this_argclass, n_int_args);
@@ -415,11 +403,9 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions,
 
   if (f > prev_start)
     {
-      const char *sub_start = current_substring;
+      std::string::size_type sub_start = m_storage.size ();
-
+      m_storage.append (prev_start, f - prev_start);
-      strncpy (current_substring, prev_start, f - prev_start);
+      /* No need for a final '\0', std::string already has one.  */
-      current_substring += f - prev_start;
-      *current_substring++ = '\0';
 
       m_pieces.emplace_back (sub_start, literal_piece, 0);
     }

gdbsupport/format.h

@@ -20,8 +20,6 @@
 #ifndef GDBSUPPORT_FORMAT_H
 #define GDBSUPPORT_FORMAT_H
 
-#include <string_view>
-
 #if defined(__MINGW32__) && !defined(PRINTF_HAS_LONG_LONG)
 # define USE_PRINTF_I64 1
 # define PRINTF_HAS_LONG_LONG
@@ -51,21 +49,15 @@ enum argclass
 
 struct format_piece
 {
-  format_piece (const char *str, enum argclass argc, int n)
+  format_piece (std::string::size_type start, enum argclass argc, int n)
-    : string (str),
+    : start (start),
       argclass (argc),
       n_int_args (n)
-  {
+  {}
-    gdb_assert (str != nullptr);
-  }
 
-  bool operator== (const format_piece &other) const
+  /* Where this piece starts, within FORMAT_PIECES::M_STORAGE.  */
-  {
+  std::string::size_type start;
-    return (this->argclass == other.argclass
-	    && std::string_view (this->string) == other.string);
-  }
 
-  const char *string;
   enum argclass argclass;
   /* Count the number of preceding 'int' arguments that must be passed
      along.  This is used for a width or precision of '*'.  Note that
@@ -95,10 +87,17 @@ public:
     return m_pieces.end ();
   }
 
+  /* Return the string associated to PIECE.  */
+  const char *piece_str (const format_piece &piece)
+  { return &m_storage[piece.start]; }
+
 private:
 
   std::vector<format_piece> m_pieces;
-  gdb::unique_xmalloc_ptr<char> m_storage;
+
+  /* This is used as a buffer of concatenated null-terminated strings.  The
+     individual strings are referenced by FORMAT_PIECE::START.  */
+  std::string m_storage;
 };
 
 #endif /* GDBSUPPORT_FORMAT_H */