Re: [RFA 4/6] Simple cleanup removals in remote.c

[Pedro Alves <palves@redhat.com>]

On 10/16/2017 10:36 PM, Simon Marchi wrote:
> On 2017-10-16 05:14 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>>
>> Simon> For this one, wouldn't it be easier to just go with a string?  Something like:
>>
>> It's easier but less efficient.
>> I wasn't sure if it mattered so I erred on the side of efficiency.
>>
>> Tom
>>
> 
> I suppose you are talking about the += string_printf ()?  It's true that it's
> not the most efficient, because the formatting is done in a separate buffer
> (an std::string) and then copied, instead of directly in-place.  At least,
> there's likely no dynamic memory allocation, because the string fits in
> std::string's local buffer.

This suggests to me that we're missing a string_printf variant that
appends to a preexisting string:

  void string_appendf (std::string &dest, const char* fmt, ...);

See (untested) patch below.

> 
> So since there's likely no real-world impact, I would've erred on the other
> side.
> 
> In any case, I don't really mind, your version is good too and eliminates the
> clean up, which is the main point.

I agree.  Tom, please go ahead.

From e9602dc6ee8eac1f0dc5b267e961f9a206ce649b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 16 Oct 2017 23:22:27 +0100
Subject: [PATCH] string_appendf

---
 gdb/common/common-utils.c | 22 ++++++++++++++++++++++
 gdb/common/common-utils.h |  5 +++++
 gdb/remote.c              | 22 +++++++---------------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index d8c546a..8ca62f3 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -153,6 +153,28 @@ xsnprintf (char *str, size_t size, const char *format, ...)
 
 /* See documentation in common-utils.h.  */
 
+void
+string_appendf (std::string &str, const char* fmt, ...)
+{
+  va_list vp;
+  int grow_size;
+
+  va_start (vp, fmt);
+  grow_size = vsnprintf (NULL, 0, fmt, vp);
+  va_end (vp);
+
+  size_t curr_size = str.size ();
+  str.resize (curr_size + grow_size);
+
+  /* C++11 and later guarantee std::string uses contiguous memory and
+     always includes the terminating '\0'.  */
+  va_start (vp, fmt);
+  vsprintf (&str[curr_size], fmt, vp);
+  va_end (vp);
+}
+
+/* See documentation in common-utils.h.  */
+
 std::string
 string_printf (const char* fmt, ...)
 {
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 19724f9..bf1b444 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -67,6 +67,11 @@ std::string string_printf (const char* fmt, ...)
 std::string string_vprintf (const char* fmt, va_list args)
   ATTRIBUTE_PRINTF (1, 0);
 
+/* Like string_printf, but appends to dest instead of returning a new
+   std::string.  */
+void string_appendf (std::string &dest, const char* fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
 /* 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/remote.c b/gdb/remote.c
index 6b77a9f..a6cb724 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2086,40 +2086,32 @@ remote_set_syscall_catchpoint (struct target_ops *self,
 			  pid, needed, any_count, n_sysno);
     }
 
-  gdb::unique_xmalloc_ptr<char> built_packet;
+  std::string built_packet;
   if (needed)
     {
       /* Prepare a packet with the sysno list, assuming max 8+1
 	 characters for a sysno.  If the resulting packet size is too
 	 big, fallback on the non-selective packet.  */
       const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
-
-      built_packet.reset ((char *) xmalloc (maxpktsz));
-      strcpy (built_packet.get (), "QCatchSyscalls:1");
+      built_packet.reserve (maxpktsz);
+      built_packet = "QCatchSyscalls:1";
       if (!any_count)
 	{
-	  int i;
-	  char *p;
-
-	  p = built_packet.get ();
-	  p += strlen (p);
-
 	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
-	  for (i = 0; i < table_size; i++)
+	  for (int i = 0; i < table_size; i++)
 	    {
 	      if (table[i] != 0)
-		p += xsnprintf (p, built_packet.get () + maxpktsz - p,
-				";%x", i);
+		string_appendf (built_packet, ";%x", i);
 	    }
 	}
-      if (strlen (built_packet.get ()) > get_remote_packet_size ())
+      if (built_packet.size () > get_remote_packet_size ())
 	{
 	  /* catch_packet too big.  Fallback to less efficient
 	     non selective mode, with GDB doing the filtering.  */
 	  catch_packet = "QCatchSyscalls:1";
 	}
       else
-	catch_packet = built_packet.get ();
+	catch_packet = built_packet.c_str ();
     }
   else
     catch_packet = "QCatchSyscalls:0";
-- 
2.5.5