[PATCH] fork-child.c: Avoid unnecessary heap-allocation / string copying (Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior)

[Pedro Alves <palves at redhat dot com>]

On 04/12/2017 06:04 AM, Sergio Durigan Junior wrote:

>>>>  static void
>>>> -breakup_args (char *scratch, char **argv)
>>>> +breakup_args (const std::string &scratch, std::vector<char *> &argv)
>>>>  {
>>>
>>> ...
>>>
>>>> +
>>>> +      std::string arg = scratch.substr (cur_pos, next_sep - cur_pos);
>>>> +
>>>
>>> This creates a temporary string (heap allocates) ...
>>>
>>>> +      argv.push_back (xstrdup (arg.c_str ()));
>>>
>>> ... and here you create yet another copy.
>>>
>>> You should be able to avoid it by using e.g., savestring:
>>>
>>>     char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos);
>>>     argv.push_back (arg);
>>
>> Fair enough.  I had my mind on "C++-only mode" when writing this code.

Yup, in C++, it's good to keep unnecessary temporaries
and hidden heap allocations in mind.  Actually, now that I look a bit
deeper, I think we can avoid a "premature pessimization" here, keeping
the same level of clarity.

I think it'd be good to push this patch below.  WDYT?

>From 64d0035762344ed33858a3b8930a83e7071ea4a8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 12 Apr 2017 10:55:36 +0100
Subject: [PATCH] fork-child.c: Avoid unnecessary heap-allocation / string
 copying

The previous patch converted the argv building from an
alloca-allocated array of non-owning arg pointers, to a std::vector of
owning pointers, which results in N string dups, with N being the
number of arguments in the vector, and then requires manually
releasing the pointers owned by the vector.

This patch makes the vector hold non-owning pointers, and avoids the
string dups, by doing one single string copy of the arguments upfront,
and replacing separators with NULL terminators in place, like we used
to.

With this, there's no need to remember to call free_vector_argv either.

Tested on x86_64 Fedora 23.

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

	* fork-child.c (breakup_args): Make 'scratch' non-const.
	Replace separators with NULL terminators in place.  Change
	type of vector.  (fork_inferior): The argument vector now
	holds non-owning pointers.  Don't strdup strings into the
	vector.  Remove free_vector_argv call.
---
 gdb/fork-child.c | 57 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 6b7386e..8a6f9e6 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -46,10 +46,12 @@ static char *exec_wrapper;
 /* Break up SCRATCH into an argument vector suitable for passing to
    execvp and store it in ARGV.  E.g., on "run a b c d" this routine
    would get as input the string "a b c d", and as output it would
-   fill in ARGV with the four arguments "a", "b", "c", "d".  */
+   fill in ARGV with the four arguments "a", "b", "c", "d".  The
+   pointers pushed to ARGV point directly into SCRATCH, which is
+   modified in place with the necessary NULL terminators.  */
 
 static void
-breakup_args (const std::string &scratch, std::vector<char *> &argv)
+breakup_args (std::string &scratch, std::vector<const char *> &argv)
 {
   for (size_t cur_pos = 0; cur_pos < scratch.size ();)
     {
@@ -62,13 +64,19 @@ breakup_args (const std::string &scratch, std::vector<char *> &argv)
       /* Find the position of the next separator.  */
       std::size_t next_sep = scratch.find_first_of (" \t\n", cur_pos);
 
-      /* No separator found, which means this is the last
-	 argument.  */
       if (next_sep == std::string::npos)
-	next_sep = scratch.size ();
+	{
+	  /* No separator found, which means this is the last
+	     argument.  */
+	  next_sep = scratch.size ();
+	}
+      else
+	{
+	  /* Replace the separator with a terminator.  */
+	  scratch[next_sep++] = '\0';
+	}
 
-      char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos);
-      argv.push_back (arg);
+      argv.push_back (&scratch[cur_pos]);
 
       cur_pos = next_sep;
     }
@@ -155,7 +163,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
   static const char *exec_file;
   char **save_our_env;
   int shell = 0;
-  std::vector<char *> argv;
   const char *inferior_io_terminal = get_inferior_io_terminal ();
   struct inferior *inf;
   int i;
@@ -183,21 +190,29 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
       shell = 1;
     }
 
+  /* The argument vector.  Holds non-owning pointers.  */
+  std::vector<const char *> c_argv;
+
+  /* These must be live up to the exec call, because the arguments in
+     C_ARGV point inside them.  */
+  std::string broken_up_args;
+  std::string shell_command;
+
   if (!shell)
     {
       /* We're going to call execvp.  Create argument vector.  */
-      argv.push_back (xstrdup (exec_file));
-      breakup_args (allargs, argv);
+      c_argv.push_back (exec_file);
+      broken_up_args = allargs;
+      breakup_args (broken_up_args, c_argv);
     }
   else
     {
       /* We're going to call a shell.  */
-      std::string shell_command;
       const char *p;
       int need_to_quote;
       const int escape_bang = escape_bang_in_quoted_argument (shell_file);
 
-      shell_command = std::string ("exec ");
+      shell_command = "exec ";
 
       /* Add any exec wrapper.  That may be a program name with arguments, so
 	 the user must handle quoting.  */
@@ -265,12 +280,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
       /* If we decided above to start up with a shell, we exec the
 	 shell, "-c" says to interpret the next arg as a shell command
 	 to execute, and this command is "exec <target-program>
-	 <args>".  We xstrdup all the strings here because they will
-	 be free'd later in the code.  */
-      argv.push_back (xstrdup (shell_file));
-      argv.push_back (xstrdup ("-c"));
-      argv.push_back (xstrdup (shell_command.c_str ()));
-      argv.push_back (NULL);
+	 <args>".  */
+      c_argv.reserve (4);
+      c_argv.push_back (shell_file);
+      c_argv.push_back ("-c");
+      c_argv.push_back (shell_command.c_str ());
+      c_argv.push_back (NULL);
     }
 
   /* Retain a copy of our environment variables, since the child will
@@ -376,6 +391,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
          path to find $SHELL.  Rich Pixley says so, and I agree.  */
       environ = env;
 
+      /* It is guaranteed that the exec functions do not modify the
+	 arguments, but they nevertheless expect "char **".  */
+      char **argv = const_cast<char **> (&c_argv[0]);
+
       if (exec_fun != NULL)
         (*exec_fun) (argv[0], &argv[0], env);
       else
@@ -393,8 +412,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
       _exit (0177);
     }
 
-  free_vector_argv (argv);
-
   /* Restore our environment in case a vforked child clob'd it.  */
   environ = save_our_env;
 
-- 
2.5.5