[RFA 00/12] some minor cleanup removals

[RFA 00/12] some minor cleanup removals

[Tom Tromey]

This removes a number of cleanups, generally string related.

I introduced string_vprintf to allow the use of std::string in some of
these patches.

The final patch just removes a couple of unused declarations; I would
have checked it in as obvious, but it was more convenient to just
leave it in this series.

Tom

[RFA 06/12] Remove cleanups from cp-support.c

[Tom Tromey]

This removes some cleanups from cp-support.c, using std::string.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* cp-support.c (gdb_demangle): Use std::string.
---
 gdb/ChangeLog    |  4 ++++
 gdb/cp-support.c | 25 ++++++++++---------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 6b5f2a3..d88bdaa 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1546,17 +1546,14 @@ gdb_demangle (const char *name, int options)
 
 	  if (!error_reported)
 	    {
-	      char *short_msg, *long_msg;
-	      struct cleanup *back_to;
+	      std::string short_msg
+		= string_printf (_("unable to demangle '%s' "
+				   "(demangler failed with signal %d)"),
+				 name, crash_signal);
 
-	      short_msg = xstrprintf (_("unable to demangle '%s' "
-				      "(demangler failed with signal %d)"),
-				    name, crash_signal);
-	      back_to = make_cleanup (xfree, short_msg);
-
-	      long_msg = xstrprintf ("%s:%d: %s: %s", __FILE__, __LINE__,
-				    "demangler-warning", short_msg);
-	      make_cleanup (xfree, long_msg);
+	      std::string long_msg
+		= string_printf ("%s:%d: %s: %s", __FILE__, __LINE__,
+				 "demangler-warning", short_msg.c_str ());
 
 	      target_terminal::scoped_restore_terminal_state term_state;
 	      target_terminal::ours_for_output ();
@@ -1565,13 +1562,11 @@ gdb_demangle (const char *name, int options)
 	      if (core_dump_allowed)
 		fprintf_unfiltered (gdb_stderr,
 				    _("%s\nAttempting to dump core.\n"),
-				    long_msg);
+				    long_msg.c_str ());
 	      else
-		warn_cant_dump_core (long_msg);
-
-	      demangler_warning (__FILE__, __LINE__, "%s", short_msg);
+		warn_cant_dump_core (long_msg.c_str ());
 
-	      do_cleanups (back_to);
+	      demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ());
 
 	      error_reported = 1;
 	    }
-- 
2.9.5

[RFA 05/12] Remove some cleanups from stack.c

[Tom Tromey]

This removes some cleanups from stack.c by using std::string or
gdb::unique_xmalloc_ptr.  One cleanup remains in this file; I did not
remove it here because it is handled in another patch series that has
yet to be resolved.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* stack.c (parse_frame_specification): Use std::string
	(info_frame_command): Use gdb::unique_xmalloc_ptr.
---
 gdb/ChangeLog |  5 +++++
 gdb/stack.c   | 20 +++++---------------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index a00e0c5..53dc829 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1277,8 +1277,6 @@ parse_frame_specification (const char *frame_exp, int *selected_frame_p)
       numargs = 0;
       while (1)
 	{
-	  char *addr_string;
-	  struct cleanup *cleanup;
 	  const char *p;
 
 	  /* Skip leading white space, bail of EOL.  */
@@ -1290,9 +1288,8 @@ parse_frame_specification (const char *frame_exp, int *selected_frame_p)
 	  for (p = frame_exp;
 	       *p && !ISSPACE (*p);
 	       p++);
-	  addr_string = savestring (frame_exp, p - frame_exp);
+	  std::string addr_string (frame_exp, p - frame_exp);
 	  frame_exp = p;
-	  cleanup = make_cleanup (xfree, addr_string);
 	  
 	  /* NOTE: Parse and evaluate expression, but do not use
 	     functions such as parse_and_eval_long or
@@ -1302,9 +1299,7 @@ parse_frame_specification (const char *frame_exp, int *selected_frame_p)
 	     side-effects.  */
 	  if (numargs >= ARRAY_SIZE (args))
 	    error (_("Too many args in frame specification"));
-	  args[numargs++] = parse_and_eval (addr_string);
-
-	  do_cleanups (cleanup);
+	  args[numargs++] = parse_and_eval (addr_string.c_str ());
 	}
     }
 
@@ -1400,7 +1395,6 @@ info_frame_command (char *addr_exp, int from_tty)
   const char *pc_regname;
   int selected_frame_p;
   struct gdbarch *gdbarch;
-  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
   CORE_ADDR frame_pc;
   int frame_pc_p;
   /* Initialize it to avoid "may be used uninitialized" warning.  */
@@ -1428,6 +1422,7 @@ info_frame_command (char *addr_exp, int from_tty)
   func = get_frame_function (fi);
   symtab_and_line sal = find_frame_sal (fi);
   s = sal.symtab;
+  gdb::unique_xmalloc_ptr<char> func_only;
   if (func)
     {
       funname = SYMBOL_PRINT_NAME (func);
@@ -1439,13 +1434,10 @@ info_frame_command (char *addr_exp, int from_tty)
 	     stored in the symbol table, but we stored a version
 	     with DMGL_PARAMS turned on, and here we don't want to
 	     display parameters.  So remove the parameters.  */
-	  char *func_only = cp_remove_params (funname);
+	  func_only.reset (cp_remove_params (funname));
 
 	  if (func_only)
-	    {
-	      funname = func_only;
-	      make_cleanup (xfree, func_only);
-	    }
+	    funname = func_only.get ();
 	}
     }
   else if (frame_pc_p)
@@ -1697,8 +1689,6 @@ info_frame_command (char *addr_exp, int from_tty)
     if (count || need_nl)
       puts_filtered ("\n");
   }
-
-  do_cleanups (back_to);
 }
 
 /* Print briefly all stack frames or just the innermost COUNT_EXP
-- 
2.9.5

Re: [RFA 05/12] Remove some cleanups from stack.c

[Pedro Alves]

On 09/28/2017 08:50 PM, Tom Tromey wrote:
> This removes some cleanups from stack.c by using std::string or
> gdb::unique_xmalloc_ptr.  One cleanup remains in this file; I did not
> remove it here because it is handled in another patch series that has
> yet to be resolved.
> 
> gdb/ChangeLog
> 2017-09-28  Tom Tromey  <tom@tromey.com>
> 
> 	* stack.c (parse_frame_specification): Use std::string
> 	(info_frame_command): Use gdb::unique_xmalloc_ptr.

OK.

The cp_remove_param-related changes reminded me of:
  https://sourceware.org/ml/gdb-patches/2017-06/msg00029.html
(the last hunk)

I think I'll apply that soon, once yours is in.

Thanks,
Pedro Alves

[RFA 08/12] Remove cleanup from complaints.c

[Tom Tromey]

This removes a cleanup from complaints.c by using std::string.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* complaints.c (vcomplaint): Use std::string.
---
 gdb/ChangeLog    | 4 ++++
 gdb/complaints.c | 9 +++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index aba305c..6456cae 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -192,16 +192,14 @@ vcomplaint (struct complaints **c, const char *file,
 	vwarning (fmt, args);
       else
 	{
-	  char *msg;
-	  struct cleanup *cleanups;
-	  msg = xstrvprintf (fmt, args);
-	  cleanups = make_cleanup (xfree, msg);
+	  std::string msg (string_vprintf (fmt, args));
 	  wrap_here ("");
 	  if (series != SUBSEQUENT_MESSAGE)
 	    begin_line ();
 	  /* XXX: i18n */
 	  fprintf_filtered (gdb_stderr, "%s%s%s",
-			    complaints->explanation[series].prefix, msg,
+			    complaints->explanation[series].prefix,
+			    msg.c_str (),
 			    complaints->explanation[series].postfix);
 	  /* Force a line-break after any isolated message.  For the
              other cases, clear_complaints() takes care of any missing
@@ -214,7 +212,6 @@ vcomplaint (struct complaints **c, const char *file,
 	    fputs_filtered ("\n", gdb_stderr);
 	  else
 	    wrap_here ("");
-	  do_cleanups (cleanups);
 	}
     }
 
-- 
2.9.5

Re: [RFA 08/12] Remove cleanup from complaints.c

[Pedro Alves]

On 09/28/2017 08:50 PM, Tom Tromey wrote:

> -	  char *msg;
> -	  struct cleanup *cleanups;
> -	  msg = xstrvprintf (fmt, args);
> -	  cleanups = make_cleanup (xfree, msg);
> +	  std::string msg (string_vprintf (fmt, args));

Please use copy-init.  Otherwise OK.

Thanks,
Pedro Alves

[RFA 03/12] Remove cleanups from utils.c

[Tom Tromey]

This removes a couple of cleanups from utils.c through the use of
std::string.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* utils.c (vfprintf_maybe_filtered): Use std::string.
	(vfprintf_unfiltered): Likewise.
---
 gdb/ChangeLog |  5 +++++
 gdb/utils.c   | 23 +++++++----------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 24294be..dbd56b2 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2036,13 +2036,8 @@ static void
 vfprintf_maybe_filtered (struct ui_file *stream, const char *format,
 			 va_list args, int filter)
 {
-  char *linebuffer;
-  struct cleanup *old_cleanups;
-
-  linebuffer = xstrvprintf (format, args);
-  old_cleanups = make_cleanup (xfree, linebuffer);
-  fputs_maybe_filtered (linebuffer, stream, filter);
-  do_cleanups (old_cleanups);
+  std::string linebuffer (string_vprintf (format, args));
+  fputs_maybe_filtered (linebuffer.c_str (), stream, filter);
 }
 
 
@@ -2055,11 +2050,7 @@ vfprintf_filtered (struct ui_file *stream, const char *format, va_list args)
 void
 vfprintf_unfiltered (struct ui_file *stream, const char *format, va_list args)
 {
-  char *linebuffer;
-  struct cleanup *old_cleanups;
-
-  linebuffer = xstrvprintf (format, args);
-  old_cleanups = make_cleanup (xfree, linebuffer);
+  std::string linebuffer (string_vprintf (format, args));
   if (debug_timestamp && stream == gdb_stdlog)
     {
       using namespace std::chrono;
@@ -2069,18 +2060,18 @@ vfprintf_unfiltered (struct ui_file *stream, const char *format, va_list args)
       seconds s = duration_cast<seconds> (now.time_since_epoch ());
       microseconds us = duration_cast<microseconds> (now.time_since_epoch () - s);
 
-      len = strlen (linebuffer);
+      len = linebuffer.size ();
       need_nl = (len > 0 && linebuffer[len - 1] != '\n');
 
       std::string timestamp = string_printf ("%ld.%06ld %s%s",
 					     (long) s.count (),
 					     (long) us.count (),
-					     linebuffer, need_nl ? "\n": "");
+					     linebuffer.c_str (),
+					     need_nl ? "\n": "");
       fputs_unfiltered (timestamp.c_str (), stream);
     }
   else
-    fputs_unfiltered (linebuffer, stream);
-  do_cleanups (old_cleanups);
+    fputs_unfiltered (linebuffer.c_str (), stream);
 }
 
 void
-- 
2.9.5

Re: [RFA 03/12] Remove cleanups from utils.c

[Pedro Alves]

This is OK, with nits below.

On 09/28/2017 08:50 PM, Tom Tromey wrote:

> -
> -  linebuffer = xstrvprintf (format, args);
> -  old_cleanups = make_cleanup (xfree, linebuffer);
> -  fputs_maybe_filtered (linebuffer, stream, filter);
> -  do_cleanups (old_cleanups);
> +  std::string linebuffer (string_vprintf (format, args));

Please use copy initialization:

  std::string linebuffer = string_vprintf (format, args);

... to spare readers from wondering what explicit/converting
constructor this is trying to call.

> -  linebuffer = xstrvprintf (format, args);
> -  old_cleanups = make_cleanup (xfree, linebuffer);
> +  std::string linebuffer (string_vprintf (format, args));

Ditto.

Thanks,
Pedro Alves

[RFA 04/12] Remove cleanup from tilegx-tdep.c

[Tom Tromey]

This removes a cleanup from tilegx-tdep.c, by using gdb::byte_vector.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* tilegx-tdep.c (tilegx_push_dummy_call): Use gdb::byte_vector.
---
 gdb/ChangeLog     |  4 ++++
 gdb/tilegx-tdep.c | 13 +++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index b26622d..5291e10 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -42,6 +42,7 @@
 #include "tilegx-tdep.h"
 #include "opcode/tilegx.h"
 #include <algorithm>
+#include "common/byte-vector.h"
 
 struct tilegx_frame_cache
 {
@@ -328,21 +329,17 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
      the stack, word aligned.  */
   for (j = nargs - 1; j >= i; j--)
     {
-      gdb_byte *val;
-      struct cleanup *back_to;
       const gdb_byte *contents = value_contents (args[j]);
 
       typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
       slacklen = align_up (typelen, 8) - typelen;
-      val = (gdb_byte *) xmalloc (typelen + slacklen);
-      back_to = make_cleanup (xfree, val);
-      memcpy (val, contents, typelen);
-      memset (val + typelen, 0, slacklen);
+      gdb::byte_vector val (typelen + slacklen);
+      memcpy (val.data (), contents, typelen);
+      memset (val.data () + typelen, 0, slacklen);
 
       /* Now write data to the stack.  The stack grows downwards.  */
       stack_dest -= typelen + slacklen;
-      write_memory (stack_dest, val, typelen + slacklen);
-      do_cleanups (back_to);
+      write_memory (stack_dest, val.data (), typelen + slacklen);
     }
 
   /* Add 16 bytes for linkage space to the stack.  */
-- 
2.9.5

Re: [RFA 04/12] Remove cleanup from tilegx-tdep.c

[Pedro Alves]

On 09/28/2017 08:50 PM, Tom Tromey wrote:
> This removes a cleanup from tilegx-tdep.c, by using gdb::byte_vector.
> 
> gdb/ChangeLog
> 2017-09-28  Tom Tromey  <tom@tromey.com>
> 
> 	* tilegx-tdep.c (tilegx_push_dummy_call): Use gdb::byte_vector.

OK.

Thanks,
Pedro Alves

[RFA 01/12] Introduce string_vprintf

[Tom Tromey]

This adds string_vprintf, a va_list variant of string_printf.  This
will be used in later patches.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* common/common-utils.c (string_vprintf): New function.
	* common/common-utils.h (string_vprintf): Declare.
---
 gdb/ChangeLog             |  5 +++++
 gdb/common/common-utils.c | 21 +++++++++++++++++++++
 gdb/common/common-utils.h |  4 ++++
 3 files changed, 30 insertions(+)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 6b10d11..d8c546a 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -174,6 +174,27 @@ string_printf (const char* fmt, ...)
   return str;
 }
 
+/* See documentation in common-utils.h.  */
+
+std::string
+string_vprintf (const char* fmt, va_list args)
+{
+  va_list vp;
+  size_t size;
+
+  va_copy (vp, args);
+  size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  std::string str (size, '\0');
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  vsprintf (&str[0], fmt, args);
+
+  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 6475c28..19724f9 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -63,6 +63,10 @@ int xsnprintf (char *str, size_t size, const char *format, ...)
 std::string string_printf (const char* fmt, ...)
   ATTRIBUTE_PRINTF (1, 2);
 
+/* Like string_printf, but takes a va_list.  */
+std::string string_vprintf (const char* fmt, va_list args)
+  ATTRIBUTE_PRINTF (1, 0);
+
 /* 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.  */
-- 
2.9.5

Re: [RFA 01/12] Introduce string_vprintf

[Pedro Alves]

On 09/28/2017 08:50 PM, Tom Tromey wrote:
> This adds string_vprintf, a va_list variant of string_printf.  This
> will be used in later patches.

Thanks.

This is OK, but IWBN to add some unittest to
gdb/unittests/utils-selftests.c.

Thanks,
Pedro Alves

Re: [RFA 01/12] Introduce string_vprintf

[Tom Tromey]

Pedro> This is OK, but IWBN to add some unittest to
Pedro> gdb/unittests/utils-selftests.c.

I didn't see that so I just made a new file.
There isn't much there; I didn't think it made sense to write a
comprehensive printf test suite for this function.  I can add more if
you think this isn't enough.

Tom

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2120df6..9c0f6b6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-09-28  Tom Tromey  <tom@tromey.com>
+
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add new file.
+	(SUBDIR_UNITTESTS_OBS): Add new file.
+	* unittests/string-vprintf-selftests.c: New file.
+	* common/common-utils.c (string_vprintf): New function.
+	* common/common-utils.h (string_vprintf): Declare.
+
 2017-09-28  Alexander Shaposhnikov <alexander.v.shaposhnikov@gmail.com> (tiny patch)
 
 	* dwarf2read.c (open_and_init_dwp_file): Protect against dwp_file
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 9004b35..ff86ce9 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -533,6 +533,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/ptid-selftests.c \
 	unittests/scoped_restore-selftests.c \
+	unittests/string-vprintf-selftests.c \
 	unittests/xml-utils-selftests.c
 
 SUBDIR_UNITTESTS_OBS = \
@@ -543,6 +544,7 @@ SUBDIR_UNITTESTS_OBS = \
 	optional-selftests.o \
 	ptid-selftests.o \
 	scoped_restore-selftests.o \
+	string-vprintf-selftests.o \
 	xml-utils-selftests.o
 
 # Opcodes currently live in one of two places.  Either they are in the
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 6b10d11..d8c546a 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -174,6 +174,27 @@ string_printf (const char* fmt, ...)
   return str;
 }
 
+/* See documentation in common-utils.h.  */
+
+std::string
+string_vprintf (const char* fmt, va_list args)
+{
+  va_list vp;
+  size_t size;
+
+  va_copy (vp, args);
+  size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  std::string str (size, '\0');
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  vsprintf (&str[0], fmt, args);
+
+  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 6475c28..19724f9 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -63,6 +63,10 @@ int xsnprintf (char *str, size_t size, const char *format, ...)
 std::string string_printf (const char* fmt, ...)
   ATTRIBUTE_PRINTF (1, 2);
 
+/* Like string_printf, but takes a va_list.  */
+std::string string_vprintf (const char* fmt, va_list args)
+  ATTRIBUTE_PRINTF (1, 0);
+
 /* 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/unittests/string-vprintf-selftests.c b/gdb/unittests/string-vprintf-selftests.c
new file mode 100644
index 0000000..b84d0a5
--- /dev/null
+++ b/gdb/unittests/string-vprintf-selftests.c
@@ -0,0 +1,57 @@
+/* Self tests for string_vprintf for GDB, the GNU debugger.
+
+   Copyright (C) 2017 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"
+#include "common/common-utils.h"
+
+namespace selftests {
+namespace string_vprintf {
+
+std::string
+format (const char *fmt, ...)
+{
+  va_list vp;
+
+  va_start (vp, fmt);
+  std::string result = ::string_vprintf (fmt, vp);
+  va_end (vp);
+  return result;
+}
+
+void
+run_tests ()
+{
+  /* Basic smoke tests.  */
+  SELF_CHECK (format ("%s", "test") == "test");
+  SELF_CHECK (format ("%d", 23) == "23");
+  SELF_CHECK (format ("%s %d %s", "test", 23, "done")
+	      == "test 23 done");
+  SELF_CHECK (format ("nothing") == "nothing");
+}
+
+}
+}
+
+void
+_initialize_string_vprintf_selftests ()
+{
+  selftests::register_test ("string_vprintf",
+			    selftests::string_vprintf::run_tests);
+}

Re: [RFA 01/12] Introduce string_vprintf

[Tom Tromey]

Tom> I didn't see that so I just made a new file.

I pulled and saw the file, so I moved the new tests there.

Tom

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ab9250c..4cdae07 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-09-28  Tom Tromey  <tom@tromey.com>
+
+	* unittests/common-utils-selftests.c (format): New function.
+	(string_vprintf_tests): New function.
+	(_initialize_common_utils_selftests): Register new tests.
+	* common/common-utils.c (string_vprintf): New function.
+	* common/common-utils.h (string_vprintf): Declare.
+
 2017-09-28  Pedro Alves  <palves@redhat.com>
 
 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 6b10d11..d8c546a 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -174,6 +174,27 @@ string_printf (const char* fmt, ...)
   return str;
 }
 
+/* See documentation in common-utils.h.  */
+
+std::string
+string_vprintf (const char* fmt, va_list args)
+{
+  va_list vp;
+  size_t size;
+
+  va_copy (vp, args);
+  size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  std::string str (size, '\0');
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  vsprintf (&str[0], fmt, args);
+
+  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 6475c28..19724f9 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -63,6 +63,10 @@ int xsnprintf (char *str, size_t size, const char *format, ...)
 std::string string_printf (const char* fmt, ...)
   ATTRIBUTE_PRINTF (1, 2);
 
+/* Like string_printf, but takes a va_list.  */
+std::string string_vprintf (const char* fmt, va_list args)
+  ATTRIBUTE_PRINTF (1, 0);
+
 /* 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/unittests/common-utils-selftests.c b/gdb/unittests/common-utils-selftests.c
index 71bc2df..0f26b21 100644
--- a/gdb/unittests/common-utils-selftests.c
+++ b/gdb/unittests/common-utils-selftests.c
@@ -41,10 +41,33 @@ string_printf_tests ()
   SELF_CHECK (string_printf ("%s", X100000) == X100000);
 }
 
+static std::string
+format (const char *fmt, ...)
+{
+  va_list vp;
+
+  va_start (vp, fmt);
+  std::string result = string_vprintf (fmt, vp);
+  va_end (vp);
+  return result;
+}
+
+static void
+string_vprintf_tests ()
+{
+  /* Basic smoke tests.  */
+  SELF_CHECK (format ("%s", "test") == "test");
+  SELF_CHECK (format ("%d", 23) == "23");
+  SELF_CHECK (format ("%s %d %s", "test", 23, "done")
+	      == "test 23 done");
+  SELF_CHECK (format ("nothing") == "nothing");
+}
+
 } /* namespace selftests */
 
 void
 _initialize_common_utils_selftests ()
 {
   selftests::register_test ("string_printf", selftests::string_printf_tests);
+  selftests::register_test ("string_vprintf", selftests::string_vprintf_tests);
 }

Re: [RFA 01/12] Introduce string_vprintf

[Pedro Alves]

On 09/29/2017 03:59 AM, Tom Tromey wrote:
> Tom> I didn't see that so I just made a new file.
> 
> I pulled and saw the file, so I moved the new tests there.

Sorry, I shouldn't have assumed you'd see it.

This version is OK.

Thanks,
Pedro Alves

[RFA 02/12] Remove cleanup from display_gdb_prompt

[Tom Tromey]

This removes a cleanup from display_gdb_prompt by using std::string.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* event-top.c (top_level_prompt): Return std::string.
	(display_gdb_prompt): Update.
---
 gdb/ChangeLog   |  5 +++++
 gdb/event-top.c | 22 ++++++++--------------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index b1cc8b9..17367ea 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -48,7 +48,7 @@
 /* readline defines this.  */
 #undef savestring
 
-static char *top_level_prompt (void);
+static std::string top_level_prompt ();
 
 /* Signal handlers.  */
 #ifdef SIGQUIT
@@ -352,16 +352,13 @@ gdb_rl_callback_handler_reinstall (void)
 void
 display_gdb_prompt (const char *new_prompt)
 {
-  char *actual_gdb_prompt = NULL;
-  struct cleanup *old_chain;
+  std::string actual_gdb_prompt;
 
   annotate_display_prompt ();
 
   /* Reset the nesting depth used when trace-commands is set.  */
   reset_command_nest_depth ();
 
-  old_chain = make_cleanup (free_current_contents, &actual_gdb_prompt);
-
   /* Do not call the python hook on an explicit prompt change as
      passed to this function, as this forms a secondary/local prompt,
      IE, displayed but not set.  */
@@ -391,7 +388,6 @@ display_gdb_prompt (const char *new_prompt)
 
 	  if (current_ui->command_editing)
 	    gdb_rl_callback_handler_remove ();
-	  do_cleanups (old_chain);
 	  return;
 	}
       else if (ui->prompt_state == PROMPT_NEEDED)
@@ -402,12 +398,12 @@ display_gdb_prompt (const char *new_prompt)
 	}
     }
   else
-    actual_gdb_prompt = xstrdup (new_prompt);
+    actual_gdb_prompt = new_prompt;
 
   if (current_ui->command_editing)
     {
       gdb_rl_callback_handler_remove ();
-      gdb_rl_callback_handler_install (actual_gdb_prompt);
+      gdb_rl_callback_handler_install (actual_gdb_prompt.c_str ());
     }
   /* new_prompt at this point can be the top of the stack or the one
      passed in.  It can't be NULL.  */
@@ -416,11 +412,9 @@ display_gdb_prompt (const char *new_prompt)
       /* Don't use a _filtered function here.  It causes the assumed
          character position to be off, since the newline we read from
          the user is not accounted for.  */
-      fputs_unfiltered (actual_gdb_prompt, gdb_stdout);
+      fputs_unfiltered (actual_gdb_prompt.c_str (), gdb_stdout);
       gdb_flush (gdb_stdout);
     }
-
-  do_cleanups (old_chain);
 }
 
 /* Return the top level prompt, as specified by "set prompt", possibly
@@ -428,7 +422,7 @@ display_gdb_prompt (const char *new_prompt)
    with the prompt prefix and suffix (annotations).  The caller is
    responsible for freeing the returned string.  */
 
-static char *
+static std::string
 top_level_prompt (void)
 {
   char *prompt;
@@ -448,10 +442,10 @@ top_level_prompt (void)
 	 beginning.  */
       const char suffix[] = "\n\032\032prompt\n";
 
-      return concat (prefix, prompt, suffix, (char *) NULL);
+      return std::string (prefix) + prompt + suffix;
     }
 
-  return xstrdup (prompt);
+  return prompt;
 }
 
 /* See top.h.  */
-- 
2.9.5

Re: [RFA 02/12] Remove cleanup from display_gdb_prompt

[Pedro Alves]

Looks good, but ...

On 09/28/2017 08:50 PM, Tom Tromey wrote:

>  /* Return the top level prompt, as specified by "set prompt", possibly
> @@ -428,7 +422,7 @@ display_gdb_prompt (const char *new_prompt)
>     with the prompt prefix and suffix (annotations).  The caller is
>     responsible for freeing the returned string.  */

... please remove that last sentence from the comment above.

>  
> -static char *
> +static std::string
>  top_level_prompt (void)
>  {

Thanks,
Pedro Alves

[RFA 11/12] Remove a cleanup from symtab.c

[Tom Tromey]

This removes an unused outer cleanup from symtab.c, and an unused
cleanup declaration as well.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* symtab.c (search_symbols): Remove unused outer cleanup.
	(make_source_files_completion_list): Remove unused declaration.
---
 gdb/ChangeLog | 5 +++++
 gdb/symtab.c  | 9 +--------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2da37e8..47385df 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4241,11 +4241,6 @@ search_symbols (const char *regexp, enum search_domain kind,
   int nfound;
   gdb::optional<compiled_regex> preg;
 
-  /* OLD_CHAIN .. RETVAL_CHAIN is always freed, RETVAL_CHAIN .. current
-     CLEANUP_CHAIN is freed only in the case of an error.  */
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
-  struct cleanup *retval_chain;
-
   gdb_assert (kind <= TYPES_DOMAIN);
 
   ourtype = types[kind];
@@ -4365,7 +4360,7 @@ search_symbols (const char *regexp, enum search_domain kind,
   found = NULL;
   tail = NULL;
   nfound = 0;
-  retval_chain = make_cleanup_free_search_symbols (&found);
+  struct cleanup *retval_chain = make_cleanup_free_search_symbols (&found);
 
   ALL_COMPUNITS (objfile, cust)
   {
@@ -4478,7 +4473,6 @@ search_symbols (const char *regexp, enum search_domain kind,
     }
 
   discard_cleanups (retval_chain);
-  do_cleanups (old_chain);
   *matches = found;
 }
 
@@ -5439,7 +5433,6 @@ make_source_files_completion_list (const char *text, const char *word)
   completion_list list;
   const char *base_name;
   struct add_partial_filename_data datum;
-  struct cleanup *back_to;
 
   if (!have_full_symbols () && !have_partial_symbols ())
     return list;
-- 
2.9.5

[RFA 09/12] Remove cleanup from xstormy16-tdep.c

[Tom Tromey]

This removes a cleanup from xstormy16-tdep.c, using gdb::byte_vector.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* xstormy16-tdep.c (xstormy16_push_dummy_call): Use
	gdb::byte_vector.
---
 gdb/ChangeLog        |  5 +++++
 gdb/xstormy16-tdep.c | 13 +++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdb/xstormy16-tdep.c b/gdb/xstormy16-tdep.c
index 40a6d98..bed4305 100644
--- a/gdb/xstormy16-tdep.c
+++ b/gdb/xstormy16-tdep.c
@@ -35,6 +35,7 @@
 #include "doublest.h"
 #include "osabi.h"
 #include "objfiles.h"
+#include "common/byte-vector.h"
 
 enum gdb_regnum
 {
@@ -276,21 +277,17 @@ xstormy16_push_dummy_call (struct gdbarch *gdbarch,
      wordaligned.  */
   for (j = nargs - 1; j >= i; j--)
     {
-      gdb_byte *val;
-      struct cleanup *back_to;
       const gdb_byte *bytes = value_contents (args[j]);
 
       typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
       slacklen = typelen & 1;
-      val = (gdb_byte *) xmalloc (typelen + slacklen);
-      back_to = make_cleanup (xfree, val);
-      memcpy (val, bytes, typelen);
-      memset (val + typelen, 0, slacklen);
+      gdb::byte_vector val (typelen + slacklen);
+      memcpy (val.data (), bytes, typelen);
+      memset (val.data () + typelen, 0, slacklen);
 
       /* Now write this data to the stack.  The stack grows upwards.  */
-      write_memory (stack_dest, val, typelen + slacklen);
+      write_memory (stack_dest, val.data (), typelen + slacklen);
       stack_dest += typelen + slacklen;
-      do_cleanups (back_to);
     }
 
   store_unsigned_integer (buf, xstormy16_pc_size, byte_order, bp_addr);
-- 
2.9.5

[RFA 12/12] Remove some unused declarations

[Tom Tromey]

This removes a couple of unused cleanup-related declarations.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* main.c (captured_main_1): Remove unused declaration.
	* spu-multiarch.c (parse_spufs_run): Remove unused declaration.
---
 gdb/ChangeLog       | 5 +++++
 gdb/main.c          | 2 --
 gdb/spu-multiarch.c | 1 -
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index 66ba75b..f174a24 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -498,8 +498,6 @@ captured_main_1 (struct captured_main_args *context)
   int save_auto_load;
   struct objfile *objfile;
 
-  struct cleanup *chain;
-
 #ifdef HAVE_SBRK
   /* Set this before constructing scoped_command_stats.  */
   lim_at_start = (char *) sbrk (0);
diff --git a/gdb/spu-multiarch.c b/gdb/spu-multiarch.c
index ebca082..a935a72 100644
--- a/gdb/spu-multiarch.c
+++ b/gdb/spu-multiarch.c
@@ -56,7 +56,6 @@ static int
 parse_spufs_run (ptid_t ptid, int *fd, CORE_ADDR *addr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
-  struct cleanup *old_chain;
   struct gdbarch_tdep *tdep;
   struct regcache *regcache;
   gdb_byte buf[4];
-- 
2.9.5

[RFA 10/12] Remove cleanup from mt-tdep.c

[Tom Tromey]

Remove a cleanup from mt-tdep.c, using gdb::byte_vector.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* mt-tdep.c (mt_push_dummy_call): Use gdb::byte_vector.
---
 gdb/ChangeLog |  4 ++++
 gdb/mt-tdep.c | 13 +++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/mt-tdep.c b/gdb/mt-tdep.c
index e3ea322..7992394 100644
--- a/gdb/mt-tdep.c
+++ b/gdb/mt-tdep.c
@@ -36,6 +36,7 @@
 #include "infcall.h"
 #include "language.h"
 #include "valprint.h"
+#include "common/byte-vector.h"
 
 enum mt_arch_constants
 {
@@ -849,21 +850,17 @@ mt_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   /* Next, the rest of the arguments go onto the stack, in reverse order.  */
   for (j = nargs - 1; j >= i; j--)
     {
-      gdb_byte *val;
-      struct cleanup *back_to;
       const gdb_byte *contents = value_contents (args[j]);
       
       /* Right-justify the value in an aligned-length buffer.  */
       typelen = TYPE_LENGTH (value_type (args[j]));
       slacklen = (wordsize - (typelen % wordsize)) % wordsize;
-      val = (gdb_byte *) xmalloc (typelen + slacklen);
-      back_to = make_cleanup (xfree, val);
-      memcpy (val, contents, typelen);
-      memset (val + typelen, 0, slacklen);
+      gdb::byte_vector val (typelen + slacklen);
+      memcpy (val.data (), contents, typelen);
+      memset (val.data () + typelen, 0, slacklen);
       /* Now write this data to the stack.  */
       stack_dest -= typelen + slacklen;
-      write_memory (stack_dest, val, typelen + slacklen);
-      do_cleanups (back_to);
+      write_memory (stack_dest, val.data (), typelen + slacklen);
     }
 
   /* Finally, if a param needs to be split between registers and stack, 
-- 
2.9.5

[RFA 07/12] Remove some cleanups from tracepoint.c

[Tom Tromey]

This removes some cleanups from tracepoint.c by using std::string.  It
also removes some unused cleanup declarations.

gdb/ChangeLog
2017-09-28  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (trace_variable_command): Use std::string.
	(encode_actions_1): Remove unused declarations.
	(create_tsv_from_upload): Use std::string.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/tracepoint.c | 42 ++++++++++++------------------------------
 2 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 30e3a3a..daa55f6 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -371,10 +371,9 @@ validate_trace_state_variable_name (const char *name)
 static void
 trace_variable_command (char *args, int from_tty)
 {
-  struct cleanup *old_chain;
   LONGEST initval = 0;
   struct trace_state_variable *tsv;
-  char *name, *p;
+  char *name_start, *p;
 
   if (!args || !*args)
     error_no_arg (_("Syntax is $NAME [ = EXPR ]"));
@@ -385,23 +384,22 @@ trace_variable_command (char *args, int from_tty)
   if (*p++ != '$')
     error (_("Name of trace variable should start with '$'"));
 
-  name = p;
+  name_start = p;
   while (isalnum (*p) || *p == '_')
     p++;
-  name = savestring (name, p - name);
-  old_chain = make_cleanup (xfree, name);
+  std::string name (name_start, p - name_start);
 
   p = skip_spaces (p);
   if (*p != '=' && *p != '\0')
     error (_("Syntax must be $NAME [ = EXPR ]"));
 
-  validate_trace_state_variable_name (name);
+  validate_trace_state_variable_name (name.c_str ());
 
   if (*p == '=')
     initval = value_as_long (parse_and_eval (++p));
 
   /* If the variable already exists, just change its initial value.  */
-  tsv = find_trace_state_variable (name);
+  tsv = find_trace_state_variable (name.c_str ());
   if (tsv)
     {
       if (tsv->initial_value != initval)
@@ -412,12 +410,11 @@ trace_variable_command (char *args, int from_tty)
       printf_filtered (_("Trace state variable $%s "
 			 "now has initial value %s.\n"),
 		       tsv->name, plongest (tsv->initial_value));
-      do_cleanups (old_chain);
       return;
     }
 
   /* Create a new variable.  */
-  tsv = create_trace_state_variable (name);
+  tsv = create_trace_state_variable (name.c_str ());
   tsv->initial_value = initval;
 
   observer_notify_tsv_created (tsv);
@@ -425,8 +422,6 @@ trace_variable_command (char *args, int from_tty)
   printf_filtered (_("Trace state variable $%s "
 		     "created, with initial value %s.\n"),
 		   tsv->name, plongest (tsv->initial_value));
-
-  do_cleanups (old_chain);
 }
 
 static void
@@ -663,7 +658,6 @@ void
 validate_actionline (const char *line, struct breakpoint *b)
 {
   struct cmd_list_element *c;
-  struct cleanup *old_chain = NULL;
   const char *tmp_p;
   const char *p;
   struct bp_location *loc;
@@ -999,8 +993,6 @@ collection_list::collect_symbol (struct symbol *sym,
   /* Expressions are the most general case.  */
   if (treat_as_expr)
     {
-      struct cleanup *old_chain1 = NULL;
-
       agent_expr_up aexpr = gen_trace_for_var (scope, gdbarch,
 					       sym, trace_string);
 
@@ -1383,7 +1375,6 @@ encode_actions_1 (struct command_line *action,
 	      else
 		{
 		  unsigned long addr;
-		  struct cleanup *old_chain1 = NULL;
 
 		  expression_up exp = parse_exp_1 (&action_exp, tloc->address,
 						   block_for_pc (tloc->address),
@@ -1478,8 +1469,6 @@ encode_actions_1 (struct command_line *action,
 	      action_exp = skip_spaces (action_exp);
 
 		{
-		  struct cleanup *old_chain1 = NULL;
-
 		  expression_up exp = parse_exp_1 (&action_exp, tloc->address,
 						   block_for_pc (tloc->address),
 						   1);
@@ -3310,7 +3299,7 @@ static struct trace_state_variable *
 create_tsv_from_upload (struct uploaded_tsv *utsv)
 {
   const char *namebase;
-  char *buf;
+  std::string buf;
   int try_num = 0;
   struct trace_state_variable *tsv;
   struct cleanup *old_chain;
@@ -3318,33 +3307,26 @@ create_tsv_from_upload (struct uploaded_tsv *utsv)
   if (utsv->name)
     {
       namebase = utsv->name;
-      buf = xstrprintf ("%s", namebase);
+      buf = namebase;
     }
   else
     {
       namebase = "__tsv";
-      buf = xstrprintf ("%s_%d", namebase, try_num++);
+      buf = string_printf ("%s_%d", namebase, try_num++);
     }
 
   /* Fish for a name that is not in use.  */
   /* (should check against all internal vars?)  */
-  while (find_trace_state_variable (buf))
-    {
-      xfree (buf);
-      buf = xstrprintf ("%s_%d", namebase, try_num++);
-    }
-
-  old_chain = make_cleanup (xfree, buf);
+  while (find_trace_state_variable (buf.c_str ()))
+    buf = string_printf ("%s_%d", namebase, try_num++);
 
   /* We have an available name, create the variable.  */
-  tsv = create_trace_state_variable (buf);
+  tsv = create_trace_state_variable (buf.c_str ());
   tsv->initial_value = utsv->initial_value;
   tsv->builtin = utsv->builtin;
 
   observer_notify_tsv_created (tsv);
 
-  do_cleanups (old_chain);
-
   return tsv;
 }
 
-- 
2.9.5

Re: [RFA 00/12] some minor cleanup removals

[Sergio Durigan Junior]

On Thursday, September 28 2017, Tom Tromey wrote:

> This removes a number of cleanups, generally string related.
>
> I introduced string_vprintf to allow the use of std::string in some of
> these patches.
>
> The final patch just removes a couple of unused declarations; I would
> have checked it in as obvious, but it was more convenient to just
> leave it in this series.

FWIW, I looked at the patches and they seem OK to me.

Thanks for doing all these cleanups!

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFA 00/12] some minor cleanup removals

[Pedro Alves]

On 09/28/2017 10:04 PM, Sergio Durigan Junior wrote:
> On Thursday, September 28 2017, Tom Tromey wrote:
> 
>> This removes a number of cleanups, generally string related.
>>
>> I introduced string_vprintf to allow the use of std::string in some of
>> these patches.
>>
>> The final patch just removes a couple of unused declarations; I would
>> have checked it in as obvious, but it was more convenient to just
>> leave it in this series.
> 
> FWIW, I looked at the patches and they seem OK to me.
> 
> Thanks for doing all these cleanups!

Seconded!

I sent a few comments, but overall it looks good to me too.

Thanks,
Pedro Alves