Re: [PATCH v2 01/31] Introduce string_printf

[Pedro Alves <palves@redhat.com>]

On 10/19/2016 06:17 PM, Simon Marchi wrote:

>> +/* See documentation in common-utils.h.  */
>> +
>> +std::string
>> +string_printf (const char* fmt, ...)
>> +{
>> +  std::string str;
>> +  va_list vp;
>> +
>> +  /* Start by assuming some reasonable size will be sufficient.  */
>> +  str.resize (1024);
>> +
>> +  while (1)
>> +    {
>> +      size_t size;
>> +      int result;
>> +
>> +      va_start (vp, fmt);
>> +      size = str.size ();
>> +      result = vsnprintf (&str[0], size, fmt, vp);
>> +      va_end (vp);
>> +
>> +      str.resize (result);
> 
> I think you have an off-by-one here, which causes an infinite loop if
> the 1024 bytes buffer is not large enough.  vsnprintf returns the size
> needed without the terminating \0, so the resize here should be of
> "result + 1".  To reproduce/test easily, just change your str.resize
> (1024) to something small.

Whoops...  Good catch.  I should have been more careful.  It's the sort
of code that screams for that sort of bug.  :-)  Brown paper bag on me.

resizing to "result + 1" would be incorrect though.  A
std::string's size does not include the terminating \0.
My idea above was to include the size for the \0 in the
initial resize(1024), and then the last resize fixed
it down.  Probably should have added comments.

The version prior to the one I posted had this:

+      /* While only C++11 or later guarantee std::string has contiguous
+        memory, no known C++03 compiler lays memory non-contiguously.
+        This is a _very_ common hack.  */
+      size = str.size () + 1;
+      result = vsnprintf (const_cast<char*> (str.c_str ()), size, fmt, vp);
+      va_end (vp);
+
+      str.resize (result);


And then thinking that people might dislike the hack, I changed
it to consider the \0 as part of the initial string (std::string
can contain embedded NULLs).

But I shouldn't have.  The "hack" is really not a problem:

  https://herbsutter.com/2008/04/07/cringe-not-vectors-are-guaranteed-to-be-contiguous/#comment-483

I.e., "everyone" does it like that.

> I thought the use of a while loop for this a bit strange, but I
> understand it's to avoid duplicating the code.

Yeah.

> 
> I think you should try writing a unit test for this :).

That's a good point.  :-)  I did that now.

Here's a new version.  Passes testing along with the
rest of the series, and, switches to "pre-compute buffer size",
as I mentioned in the reply to Trevor.

From 32670757bd2647c04d02d01849e417c5a07a6cf5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 19 Oct 2016 20:25:23 +0100
Subject: [PATCH] Introduce string_printf

This introduces the string_printf function.  Like asprintf, but
returns a std::string.

gdb/ChangeLog:
yyyy-mm-yy  Pedro Alves  <palves@redhat.com>

	* Makefile.in (COMMON_OBS): Add utils-selftests.o.
	* common/common-utils.c (string_printf): New function.
	* common/common-utils.h: Include <string>.
	(string_printf): Declare.
	* utils-selftests.c: New file.
---
 gdb/Makefile.in           |  2 +-
 gdb/common/common-utils.c | 25 +++++++++++++++++++++++
 gdb/common/common-utils.h |  6 ++++++
 gdb/utils-selftests.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 gdb/utils-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 2c88434..757fd05 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1064,7 +1064,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	ada-typeprint.o c-typeprint.o f-typeprint.o m2-typeprint.o \
 	ada-valprint.o c-valprint.o cp-valprint.o d-valprint.o f-valprint.o \
 	m2-valprint.o \
-	ser-event.o serial.o mdebugread.o top.o utils.o \
+	ser-event.o serial.o mdebugread.o top.o utils.o utils-selftests.o \
 	ui-file.o \
 	user-regs.o \
 	frame.o frame-unwind.o doublest.o \
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 5a346ec..a6035ad 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -150,6 +150,31 @@ xsnprintf (char *str, size_t size, const char *format, ...)
   return ret;
 }
 
+/* See documentation in common-utils.h.  */
+
+std::string
+string_printf (const char* fmt, ...)
+{
+  va_list vp;
+  int size;
+
+  va_start (vp, fmt);
+  size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  std::string str (size, '\0');
+
+  va_start (vp, fmt);
+  /* While only C++11 or later guarantee std::string uses contiguous
+     memory, including the terminating '\0' and that c_str() returns a
+     pointer to the string's mutable data, in practice that's true
+     with all C++03 compilers.  */
+  vsprintf (const_cast<char *> (str.c_str ()), fmt, vp);
+  va_end (vp);
+
+  return str;
+}
+
 char *
 savestring (const char *ptr, size_t len)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 47def11..a9053ff 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -20,6 +20,8 @@
 #ifndef COMMON_UTILS_H
 #define COMMON_UTILS_H
 
+#include <string>
+
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
    defined, all uses must be protected by appropriate macro definition
@@ -56,6 +58,10 @@ char *xstrvprintf (const char *format, va_list ap)
 int xsnprintf (char *str, size_t size, const char *format, ...)
      ATTRIBUTE_PRINTF (3, 4);
 
+/* Returns a std::string built from a printf-style format string.  */
+std::string string_printf (const char* fmt, ...)
+  ATTRIBUTE_PRINTF (1, 2);
+
 /* Make a copy of the string at PTR with LEN characters
    (and add a null character at the end in the copy).
    Uses malloc to get the space.  Returns the address of the copy.  */
diff --git a/gdb/utils-selftests.c b/gdb/utils-selftests.c
new file mode 100644
index 0000000..482b698
--- /dev/null
+++ b/gdb/utils-selftests.c
@@ -0,0 +1,52 @@
+/* Self tests for general utility routines for GDB, the GNU debugger.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+
+/* common-utils self tests.  Defined here instead of in
+   common/common-utils.c because that file is shared with
+   gdbserver.  */
+
+static void
+common_utils_tests (void)
+{
+  SELF_CHECK (string_printf ("%s", "") == "");
+  SELF_CHECK (string_printf ("%d comes before 2", 1) == "1 comes before 2");
+  SELF_CHECK (string_printf ("hello %s", "world") == "hello world");
+
+#define X10 "xxxxxxxxxx"
+#define X100 X10 X10 X10 X10 X10 X10 X10 X10 X10 X10
+#define X1000 X100 X100 X100 X100 X100 X100 X100 X100 X100 X100
+#define X10000 X1000 X1000 X1000 X1000 X1000 X1000 X1000 X1000 X1000 X1000
+#define X100000 X10000 X10000 X10000 X10000 X10000 X10000 X10000 X10000 X10000 X10000
+  SELF_CHECK (string_printf ("%s", X10).size () == 10);
+  SELF_CHECK (string_printf ("%s", X100).size () == 100);
+  SELF_CHECK (string_printf ("%s", X1000).size () == 1000);
+  SELF_CHECK (string_printf ("%s", X10000).size () == 10000);
+  SELF_CHECK (string_printf ("%s", X100000).size () == 100000);
+}
+
+void
+_initialize_utils_selftests (void)
+{
+#if GDB_SELF_TEST
+  register_self_test (common_utils_tests);
+#endif
+}
-- 
2.5.5