[14/16] gdb/gdbserver: remove some uses of free_vector_argv

Message ID 530cf2c6789513d079c3a3817c00e9625ba18c49.1704809585.git.aburgess@redhat.com
State New
Headers
Series Inferior argument (inc for remote targets) changes |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed

Commit Message

Andrew Burgess Jan. 9, 2024, 2:26 p.m. UTC
  This commit removes some of the uses of free_vector_argv and instead
makes use of gdb::unique_xmalloc_ptr<char> to manage memory.

Most of this patch is about changing various APIs to accept the
gdb::unique_xmalloc_ptr<char> vectors instead of raw 'char *'.

However, there are a couple of places, one in gdbserver, and one in
GDB, where retaining the old API will remove the need to malloc copies
of all the incoming arguments -- this is for the initial set of
arguments delivered from the OS.  To support these I've added an
overload of construct_inferior_arguments that retains the old
API (taking a gdb::array_view<char *>), and we make use of this to
avoid unnecessary malloc/free calls.

There is a place in gdb/nat/fork-inferior.c where it seems unavoidable
that free_vector_argv is needed.  In this case we need to build an
array of 'char *' to pass to an exec call, so using
gdb::unique_xmalloc_ptr<char> is out of the question I think.

There should be no user visible changes after this commit.
---
 gdb/inferior.c                       |  9 -----
 gdb/inferior.h                       |  8 +++--
 gdb/python/py-inferior.c             |  7 ++--
 gdb/unittests/remote-arg-selftests.c | 33 +++----------------
 gdbserver/server.cc                  | 22 ++++---------
 gdbsupport/common-inferior.cc        | 49 ++++++++++++++++++++++++----
 gdbsupport/common-inferior.h         |  7 ++++
 gdbsupport/remote-args.cc            |  2 +-
 gdbsupport/remote-args.h             |  2 +-
 9 files changed, 71 insertions(+), 68 deletions(-)
  

Patch

diff --git a/gdb/inferior.c b/gdb/inferior.c
index ed138888024..d2cdf31551a 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -165,15 +165,6 @@  inferior::tty ()
   return m_terminal;
 }
 
-/* See inferior.h.  */
-
-void
-inferior::set_args (gdb::array_view<char * const> args,
-		    escape_args_func escape_func)
-{
-  set_args (construct_inferior_arguments (args, escape_func));
-}
-
 void
 inferior::set_arch (gdbarch *arch)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index d3824236ef6..3dad403b244 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -526,8 +526,12 @@  class inferior : public refcounted_object,
   };
 
   /* Set the argument string from some strings.  */
-  void set_args (gdb::array_view<char * const> args,
-		 escape_args_func escape_func);
+  template<typename T>
+  void set_args (gdb::array_view<T const> args,
+		 escape_args_func escape_func)
+  {
+    this->set_args (construct_inferior_arguments (args, escape_func));
+  }
 
   /* Get the argument string to use when running this inferior.
 
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 5b7c7fb9365..c7a47e92ded 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -915,11 +915,8 @@  infpy_set_args (PyObject *self, PyObject *value, void *closure)
 	    return -1;
 	  args.push_back (std::move (str));
 	}
-      std::vector<char *> argvec;
-      for (const auto &arg : args)
-	argvec.push_back (arg.get ());
-      gdb::array_view<char * const> view (argvec.data (), argvec.size ());
-      inf->inferior->set_args (view, escape_quotes_and_white_space);
+      gdb::array_view<gdb::unique_xmalloc_ptr<char> const> arg_view (args);
+      inf->inferior->set_args (arg_view, escape_quotes_and_white_space);
     }
   else
     {
diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
index 3240ab9aeea..b732d94792d 100644
--- a/gdb/unittests/remote-arg-selftests.c
+++ b/gdb/unittests/remote-arg-selftests.c
@@ -59,32 +59,6 @@  arg_test_desc desc[] = {
   { "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
 };
 
-/* Convert a std::vector<std::string> into std::vector<char *>.  This
-   requires copying all of the string content.  This class takes care of
-   freeing the memory once we are done with it.  */
-
-struct args_as_c_strings
-{
-  args_as_c_strings (std::vector<std::string> args)
-  {
-    for (const auto & a : args)
-      m_data.push_back (xstrdup (a.c_str ()));
-  }
-
-  ~args_as_c_strings ()
-  {
-    free_vector_argv (m_data);
-  }
-
-  std::vector<char *> &get ()
-  {
-    return m_data;
-  }
-
-private:
-  std::vector<char *> m_data;
-};
-
 /* Run the remote argument passing self tests.  */
 
 static void
@@ -132,9 +106,10 @@  self_test ()
 	}
 
       /* Now join the arguments.  */
-      args_as_c_strings split_args_c_str (split_args);
-      std::string joined_args
-	= gdb::remote_args::join (split_args_c_str.get ());
+      std::vector<gdb::unique_xmalloc_ptr<char>> temp_args;
+      for (const auto & a : split_args)
+	temp_args.push_back (make_unique_xstrdup (a.c_str ()));
+      std::string joined_args = gdb::remote_args::join (temp_args);
 
       if (run_verbose ())
 	debug_printf ("Joined (%s), expected (%s)\n",
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0445fa0237f..9c5d8ee4f54 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3385,7 +3385,7 @@  handle_v_run (char *own_buf)
 {
   client_state &cs = get_client_state ();
   char *p, *next_p;
-  std::vector<char *> new_argv;
+  std::vector<gdb::unique_xmalloc_ptr<char>> new_argv;
   gdb::unique_xmalloc_ptr<char> new_program_name;
   int i;
 
@@ -3405,7 +3405,7 @@  handle_v_run (char *own_buf)
       else if (p == next_p)
 	{
 	  /* Empty argument.  */
-	  new_argv.push_back (xstrdup (""));
+	  new_argv.push_back (make_unique_xstrdup (""));
 	}
       else
 	{
@@ -3416,14 +3416,13 @@  handle_v_run (char *own_buf)
 	  if (arg == nullptr)
 	    {
 	      write_enn (own_buf);
-	      free_vector_argv (new_argv);
 	      return;
 	    }
 
 	  if (i == 0)
 	    new_program_name = std::move (arg);
 	  else
-	    new_argv.push_back (arg.release ());
+	    new_argv.push_back (std::move (arg));
 	}
       if (*next_p == '\0')
 	break;
@@ -3436,7 +3435,6 @@  handle_v_run (char *own_buf)
       if (program_path.get () == nullptr)
 	{
 	  write_enn (own_buf);
-	  free_vector_argv (new_argv);
 	  return;
 	}
     }
@@ -3451,15 +3449,13 @@  handle_v_run (char *own_buf)
 	  return;
 	}
       else if (new_argv.size () == 1)
-	program_args = std::string (new_argv[0]);
+	program_args = std::string (new_argv[0].get ());
       else
 	program_args.clear ();
     }
   else
     program_args = gdb::remote_args::join (new_argv);
 
-  free_vector_argv (new_argv);
-
   target_create_inferior (program_path.get (), program_args);
 
   if (cs.last_status.kind () == TARGET_WAITKIND_STOPPED)
@@ -4345,16 +4341,12 @@  captured_main (int argc, char *argv[])
 
   if (pid == 0 && *next_arg != NULL)
     {
-      int i, n;
-
-      n = argc - (next_arg - argv);
       program_path.set (next_arg[0]);
-      std::vector<char *> temp_arg_vector;
-      for (i = 1; i < n; i++)
-	temp_arg_vector.push_back (next_arg[i]);
       escape_args_func escape_func = (escape_args ? escape_shell_characters
 				      : escape_quotes_and_white_space);
-      program_args = construct_inferior_arguments (temp_arg_vector,
+      int arg_count = argc - (next_arg - argv) - 1;
+      gdb::array_view<char *> arg_view (&next_arg[1], arg_count);
+      program_args = construct_inferior_arguments (arg_view,
 						   escape_func);
 
       /* Wait till we are at first instruction in program.  */
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index cf2cd9a090a..458d78295ab 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -20,30 +20,67 @@ 
 
 #include "gdbsupport/common-defs.h"
 #include "gdbsupport/common-inferior.h"
+#include "gdbsupport/function-view.h"
 
 /* See common-inferior.h.  */
 
 bool startup_with_shell = true;
 
-/* See common-inferior.h.  */
+/* Helper function for the two construct_inferior_arguments overloads
+   below.  Accept a gdb::array_view over objects of type T.  Convert each T
+   to a std::string by calling CB, and join all the resulting strings
+   together with a single space between each.  */
 
-std::string
-construct_inferior_arguments (gdb::array_view<char * const> argv,
-			      escape_args_func escape_func)
+template<typename T>
+static std::string
+construct_inferior_arguments_1 (gdb::array_view<T const> argv,
+				gdb::function_view<std::string (const T &)> cb)
 {
   std::string result;
 
-  for (const char *a : argv)
+  for (const T &a : argv)
     {
       if (!result.empty ())
 	result += " ";
 
-      result += escape_func (a);
+      result += cb (a);
     }
 
   return result;
 }
 
+/* See common-inferior.h.  */
+
+std::string
+construct_inferior_arguments
+  (gdb::array_view<gdb::unique_xmalloc_ptr<char> const> argv,
+   escape_args_func escape_func)
+{
+  /* Convert ARG to a std::string by applying ESCAPE_FUNC.  */
+  auto escape_cb = [&] (const gdb::unique_xmalloc_ptr<char> &arg)
+  {
+    return escape_func (arg.get ());
+  };
+
+  return construct_inferior_arguments_1<gdb::unique_xmalloc_ptr<char>>
+    (argv, escape_cb);
+}
+
+/* See common-inferior.h.  */
+
+std::string
+construct_inferior_arguments (gdb::array_view<char * const> argv,
+			      escape_args_func escape_func)
+{
+  /* Convert ARG to a std::string by applying ESCAPE_FUNC.  */
+  auto escape_cb = [&] (const char * const &arg)
+  {
+    return escape_func (arg);
+  };
+
+  return construct_inferior_arguments_1<char *> (argv, escape_cb);
+}
+
 /* Escape characters in ARG and return an updated string.  The string
    SPECIAL contains the set of characters that must be escaped.  SPECIAL
    must not be nullptr, and it is assumed that SPECIAL contains the newline
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 7cc01fb2f28..c607ab6d98e 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -74,6 +74,13 @@  extern std::string escape_quotes_and_white_space (const char *arg);
    into a single string, separating each element with a single space
    character.  */
 
+extern std::string
+construct_inferior_arguments
+  (gdb::array_view<gdb::unique_xmalloc_ptr<char> const> args,
+   escape_args_func escape_func);
+
+/* An overload of the above that takes an array of raw pointers.  */
+
 extern std::string
 construct_inferior_arguments (gdb::array_view<char * const> args,
 			      escape_args_func escape_func);
diff --git a/gdbsupport/remote-args.cc b/gdbsupport/remote-args.cc
index 96c12ffac67..b808a64efce 100644
--- a/gdbsupport/remote-args.cc
+++ b/gdbsupport/remote-args.cc
@@ -37,7 +37,7 @@  gdb::remote_args::split (std::string args)
 /* See remote-args.h.  */
 
 std::string
-gdb::remote_args::join (std::vector<char *> &args)
+gdb::remote_args::join (std::vector<gdb::unique_xmalloc_ptr<char>> &args)
 {
   return construct_inferior_arguments (args, escape_shell_characters);
 }
diff --git a/gdbsupport/remote-args.h b/gdbsupport/remote-args.h
index c0acce9b7c4..1397a4d16d9 100644
--- a/gdbsupport/remote-args.h
+++ b/gdbsupport/remote-args.h
@@ -37,7 +37,7 @@  extern std::vector<std::string> split (std::string args);
    passed through ::join we will get back the string 'a\ b' (without the
    single quotes), that is, we choose to escape the white space, rather
    than wrap the argument in quotes.  */
-extern std::string join (std::vector<char *> &args);
+extern std::string join (std::vector <gdb::unique_xmalloc_ptr<char>> &args);
 
 } /* namespace remote_args */