[PATCH] C++ify gdb/common/environ.c

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Disclaimer: this patch depends on
<https://sourceware.org/ml/gdb-patches/2017-03/msg00551.html> to be
fully testable.

As part of the preparation necessary for my upcoming task, I'd like to
propose that we turn gdb_environ into a class.  The approach taken
here is simple: the class gdb_environ contains everything that is
needed to manipulate the environment variables.  These variables are
stored in two data structures: an unordered_set, good because lookups
are O(n), and an std::vector<char *>, which can be converted to a
'char **' and passed as argument to functions that need it.

The usage has not changed much.  One has to initialize the class and
call its member functions if any operation needs to be done with the
environment variables.

As mentioned before, this is a preparation for an upcoming work that I
will be posting in the next few weeks or so.  For that work, I'll
probably create another data structure that will contain all the
environment variables that were set by the user using the 'set
environment' command, because I'll need access to them.  This will be
much easier with the class-ification of gdb_environ.

As noted, this has been regression-tested with the new version of
environ.exp and no regressions were found.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* charset.c (find_charset_names): Use a
	std::unique_ptr<gdb_environ>.  Update code to use
	'iconv_env' object.  Remove call to 'free_environ'.
	* common/environ.c: Include <utility>.
	(make_environ): Delete function.
	(gdb_environ::init_environ): New function.
	(free_environ): Delete function.
	(gdb_environ::clear_environ): New function.
	(gdb_environ::gdb_environ): New function.
	(init_environ): Delete function.
	(gdb_environ::~gdb_environ): New function.
	(gdb_environ::reinit_environ): Likewise.
	(gdb_environ::get_in_environ): Likewise.
	(environ_vector): Delete function.
	(unset_in_environ_vector): New function.
	(set_in_environ): Delete function.
	(gdb_environ::set_in_environ): New function.
	(unset_in_environ): Delete function.
	(gdb_environ::unset_in_environ): New function.
	(gdb_environ::get_environ_char_vector): Likewise.
	(gdb_environ::get_environ_vector): Likewise.
	* common/environ.h: Include <unordered_map> and <vector>.
	(struct gdb_environ): Delete; transform into...
	(class gdb_environ): ... this class.
	(free_environ): Delete prototype.
	(init_environ, get_in_environ, set_in_environ, unset_in_environ,
	environ_vector): Likewise.
	* infcmd.c (run_command_1): Update code to call
	'get_environ_char_vector' from 'gdb_environ' class.
	(environment_info): Update code to call methods from 'gdb_environ'
	class.
	(unset_environment_command): Likewise.
	(path_info): Likewise.
	(path_command): Likewise.
	* inferior.c (free_inferior): Delete call to 'free_environ'.
	(add_inferior_silent): Create 'inf->environment' from
	std::unique_ptr<gdb_environ>.
	* inferior.h: Include "environ.h".
	(struct inferior) <environment>: Change type from 'struct
	gdb_environ' to 'std::unique_ptr<gdb_environ>'.
	* mi/mi-cmd-env.c (mi_cmd_env_path): Update code to call
	methods from 'gdb_environ' class.
	(mi_cmd_inferior_tty_show): Use 'std::unique_ptr<gdb_environ>'.
	(_initialize_mi_cmd_env): Update code to call methods from
	'gdb_environ' class.
	* solib.c (solib_find_1): Likewise
---
 gdb/charset.c        |  11 +--
 gdb/common/environ.c | 228 +++++++++++++++++++++++++--------------------------
 gdb/common/environ.h |  62 +++++++++-----
 gdb/infcmd.c         |  31 ++++---
 gdb/inferior.c       |   4 +-
 gdb/inferior.h       |   5 +-
 gdb/mi/mi-cmd-env.c  |  13 ++-
 gdb/solib.c          |   8 +-
 8 files changed, 187 insertions(+), 175 deletions(-)

diff --git a/gdb/charset.c b/gdb/charset.c
index f55e482..a5ab383 100644
--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -794,16 +794,14 @@ find_charset_names (void)
   int err, status;
   int fail = 1;
   int flags;
-  struct gdb_environ *iconv_env;
+  std::unique_ptr<gdb_environ> iconv_env (new gdb_environ);
   char *iconv_program;
 
   /* Older iconvs, e.g. 2.2.2, don't omit the intro text if stdout is
      not a tty.  We need to recognize it and ignore it.  This text is
      subject to translation, so force LANGUAGE=C.  */
-  iconv_env = make_environ ();
-  init_environ (iconv_env);
-  set_in_environ (iconv_env, "LANGUAGE", "C");
-  set_in_environ (iconv_env, "LC_ALL", "C");
+  iconv_env->set_in_environ ("LANGUAGE", "C");
+  iconv_env->set_in_environ ("LC_ALL", "C");
 
   child = pex_init (PEX_USE_PIPES, "iconv", NULL);
 
@@ -827,7 +825,7 @@ find_charset_names (void)
   /* Note that we simply ignore errors here.  */
   if (!pex_run_in_environment (child, flags,
 			       args[0], const_cast<char **> (args),
-			       environ_vector (iconv_env),
+			       iconv_env->get_environ_char_vector (),
 			       NULL, NULL, &err))
     {
       FILE *in = pex_read_output (child, 0);
@@ -901,7 +899,6 @@ find_charset_names (void)
 
   xfree (iconv_program);
   pex_free (child);
-  free_environ (iconv_env);
 
   if (fail)
     {
diff --git a/gdb/common/environ.c b/gdb/common/environ.c
index 3145d01..bd3c964 100644
--- a/gdb/common/environ.c
+++ b/gdb/common/environ.c
@@ -18,165 +18,161 @@
 #include "common-defs.h"
 #include "environ.h"
 #include <algorithm>
-
+#include <utility>
 
-/* Return a new environment object.  */
+/* See common/environ.h.  */
 
-struct gdb_environ *
-make_environ (void)
+void
+gdb_environ::init_environ (void)
 {
-  struct gdb_environ *e;
+  extern char **environ;
+
+  if (environ == NULL)
+    return;
+
+  for (int i = 0; environ[i] != NULL; ++i)
+    {
+      std::string v = std::string (environ[i]);
+      std::size_t pos = v.find ('=');
+      std::string var = v.substr (0, pos);
+      std::string value = v.substr (pos + 1);
 
-  e = XNEW (struct gdb_environ);
+      this->env.insert (std::make_pair (var, value));
+      this->environ_vector.push_back (xstrdup (environ[i]));
+    }
 
-  e->allocated = 10;
-  e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char *));
-  e->vector[0] = 0;
-  return e;
+  /* The last element of the vector is always going to be NULL.  */
+  this->environ_vector.push_back (NULL);
 }
 
-/* Free an environment and all the strings in it.  */
+/* See common/environ.h.  */
 
 void
-free_environ (struct gdb_environ *e)
+gdb_environ::clear_environ (void)
 {
-  char **vector = e->vector;
+  this->env.clear ();
+  for (char *v : this->environ_vector)
+    xfree (v);
+  this->environ_vector.clear ();
+}
 
-  while (*vector)
-    xfree (*vector++);
+/* See common/environ.h.  */
 
-  xfree (e->vector);
-  xfree (e);
+gdb_environ::gdb_environ (void)
+{
+  this->init_environ ();
 }
 
-/* Copy the environment given to this process into E.
-   Also copies all the strings in it, so we can be sure
-   that all strings in these environments are safe to free.  */
+/* See common/environ.h.  */
 
-void
-init_environ (struct gdb_environ *e)
+gdb_environ::~gdb_environ (void)
 {
-  extern char **environ;
-  int i;
+  this->clear_environ ();
+}
 
-  if (environ == NULL)
-    return;
+/* See common/environ.h.  */
 
-  for (i = 0; environ[i]; i++) /*EMPTY */ ;
+void
+gdb_environ::reinit_environ (void)
+{
+  this->clear_environ ();
+  this->init_environ ();
+}
 
-  if (e->allocated < i)
+/* See common/environ.h.  */
+
+char *
+gdb_environ::get_in_environ (const char *var) const
+{
+  try
     {
-      e->allocated = std::max (i, e->allocated + 10);
-      e->vector = (char **) xrealloc ((char *) e->vector,
-				      (e->allocated + 1) * sizeof (char *));
+      return (char *) this->env.at (std::string (var)).c_str ();
     }
-
-  memcpy (e->vector, environ, (i + 1) * sizeof (char *));
-
-  while (--i >= 0)
+  catch (const std::out_of_range &ex)
     {
-      int len = strlen (e->vector[i]);
-      char *newobj = (char *) xmalloc (len + 1);
-
-      memcpy (newobj, e->vector[i], len + 1);
-      e->vector[i] = newobj;
+      return NULL;
     }
 }
 
-/* Return the vector of environment E.
-   This is used to get something to pass to execve.  */
-
-char **
-environ_vector (struct gdb_environ *e)
-{
-  return e->vector;
-}
-
-/* Return the value in environment E of variable VAR.  */
+/* Unset (delete) the environment variable VAR on the environment
+   vector V.  */
 
-char *
-get_in_environ (const struct gdb_environ *e, const char *var)
+static void
+unset_in_environ_vector (std::vector<char *> &v, const char *var)
 {
-  int len = strlen (var);
-  char **vector = e->vector;
-  char *s;
-
-  for (; (s = *vector) != NULL; vector++)
-    if (strncmp (s, var, len) == 0 && s[len] == '=')
-      return &s[len + 1];
-
-  return 0;
+  std::string match = std::string (var + std::string ("="));
+  const char *match_str = match.c_str ();
+
+  for (std::vector<char *>::const_iterator el = v.cbegin ();
+       el != v.cend ();
+       ++el)
+    if (startswith (*el, match_str))
+      {
+	xfree (*el);
+	v.erase (el);
+	break;
+      }
 }
 
-/* Store the value in E of VAR as VALUE.  */
+/* See common/environ.h.  */
 
 void
-set_in_environ (struct gdb_environ *e, const char *var, const char *value)
+gdb_environ::set_in_environ (const char *var, const char *value)
 {
-  int i;
-  int len = strlen (var);
-  char **vector = e->vector;
-  char *s;
+  std::unordered_map<std::string, std::string>::iterator el;
+  bool needs_update = true;
 
-  for (i = 0; (s = vector[i]) != NULL; i++)
-    if (strncmp (s, var, len) == 0 && s[len] == '=')
-      break;
+  el = this->env.find (var);
 
-  if (s == 0)
+  if (el != this->env.end ())
     {
-      if (i == e->allocated)
+      if (el->second.compare (value) == 0)
+	needs_update = false;
+      else
 	{
-	  e->allocated += 10;
-	  vector = (char **) xrealloc ((char *) vector,
-				       (e->allocated + 1) * sizeof (char *));
-	  e->vector = vector;
+	  /* If we found this item, it means that we have to update
+	     its value on the map.  */
+	  el->second = std::string (value);
+	  /* And we also have to update its value on the
+	     environ_vector.  For that, we just delete the item here
+	     and recreate it (with the new value) later.  */
+	  unset_in_environ_vector (this->environ_vector, var);
 	}
-      vector[i + 1] = 0;
     }
   else
-    xfree (s);
-
-  s = (char *) xmalloc (len + strlen (value) + 2);
-  strcpy (s, var);
-  strcat (s, "=");
-  strcat (s, value);
-  vector[i] = s;
-
-  /* This used to handle setting the PATH and GNUTARGET variables
-     specially.  The latter has been replaced by "set gnutarget"
-     (which has worked since GDB 4.11).  The former affects searching
-     the PATH to find SHELL, and searching the PATH to find the
-     argument of "symbol-file" or "exec-file".  Maybe we should have
-     some kind of "set exec-path" for that.  But in any event, having
-     "set env" affect anything besides the inferior is a bad idea.
-     What if we want to change the environment we pass to the program
-     without afecting GDB's behavior?  */
-
-  return;
+    {
+      this->env.insert (std::make_pair (std::string (var),
+					std::string (value)));
+    }
+
+  if (needs_update)
+    this->environ_vector.insert (this->environ_vector.end () - 1,
+				 xstrdup (std::string (var
+						       + std::string ("=")
+						       + value).c_str ()));
 }
 
-/* Remove the setting for variable VAR from environment E.  */
+/* See common/environ.h.  */
 
 void
-unset_in_environ (struct gdb_environ *e, const char *var)
+gdb_environ::unset_in_environ (const char *var)
 {
-  int len = strlen (var);
-  char **vector = e->vector;
-  char *s;
+  this->env.erase (var);
+  unset_in_environ_vector (this->environ_vector, var);
+}
 
-  for (; (s = *vector) != NULL; vector++)
-    {
-      if (strncmp (s, var, len) == 0 && s[len] == '=')
-	{
-	  xfree (s);
-	  /* Walk through the vector, shuffling args down by one, including
-	     the NULL terminator.  Can't use memcpy() here since the regions
-	     overlap, and memmove() might not be available.  */
-	  while ((vector[0] = vector[1]) != NULL)
-	    {
-	      vector++;
-	    }
-	  break;
-	}
-    }
+/* See common/environ.h.  */
+
+char **
+gdb_environ::get_environ_char_vector (void) const
+{
+  return (char **) &this->environ_vector[0];
+}
+
+/* See common/environ.h.  */
+
+std::vector<char *>
+gdb_environ::get_environ_vector (void) const
+{
+  return this->environ_vector;
 }
diff --git a/gdb/common/environ.h b/gdb/common/environ.h
index 3ace69e..c630425 100644
--- a/gdb/common/environ.h
+++ b/gdb/common/environ.h
@@ -17,33 +17,55 @@
 #if !defined (ENVIRON_H)
 #define ENVIRON_H 1
 
-/* We manipulate environments represented as these structures.  */
+#include <unordered_map>
+#include <vector>
 
-struct gdb_environ
-  {
-    /* Number of usable slots allocated in VECTOR.
-       VECTOR always has one slot not counted here,
-       to hold the terminating zero.  */
-    int allocated;
-    /* A vector of slots, ALLOCATED + 1 of them.
-       The first few slots contain strings "VAR=VALUE"
-       and the next one contains zero.
-       Then come some unused slots.  */
-    char **vector;
-  };
+/* Class that represents the environment variables as seeing by the
+   inferior.  */
 
-extern struct gdb_environ *make_environ (void);
+class gdb_environ
+{
+public:
+  /* Regular constructor and destructor.  */
+  gdb_environ ();
+  ~gdb_environ ();
 
-extern void free_environ (struct gdb_environ *);
+  /* Reinitialize the environment stored.  This is used when the user
+     wants to delete all environment variables added by him/her.  */
+  void reinit_environ (void);
 
-extern void init_environ (struct gdb_environ *);
+  /* Return the value in the environment for the variable VAR.  */
+  char *get_in_environ (const char *var) const;
 
-extern char *get_in_environ (const struct gdb_environ *, const char *);
+  /* Store VAR=VALUE in the environment.  */
+  void set_in_environ (const char *var, const char *value);
 
-extern void set_in_environ (struct gdb_environ *, const char *, const char *);
+  /* Unset VAR in environment.  */
+  void unset_in_environ (const char *var);
 
-extern void unset_in_environ (struct gdb_environ *, const char *);
+  /* Return the environment vector represented as a 'char **'.  */
+  char **get_environ_char_vector (void) const;
 
-extern char **environ_vector (struct gdb_environ *);
+  /* Return the environment vector.  */
+  std::vector<char *> get_environ_vector (void) const;
+
+private:
+  /* Helper function that initializes our data structures with the
+     environment variables.  */
+  void init_environ (void);
+
+  /* Helper function that clears our data structures.  */
+  void clear_environ (void);
+
+  /* A map representing the environment variables and their values.
+     It is easier to store them using a map because of the faster
+     lookups.  */
+  std::unordered_map<std::string, std::string> env;
+
+  /* A vector containing the environment variables.  This is useful
+     for when we need to obtain a 'char **' with all the existing
+     variables.  */
+  std::vector<char *> environ_vector;
+};
 
 #endif /* defined (ENVIRON_H) */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 22b2c7a..b1d0823 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -609,7 +609,8 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main)
      the value now.  */
   run_target->to_create_inferior (run_target, exec_file,
 				  std::string (get_inferior_args ()),
-				  environ_vector (current_inferior ()->environment),
+				  current_inferior ()
+				  ->environment->get_environ_char_vector (),
 				  from_tty);
   /* to_create_inferior should push the target, so after this point we
      shouldn't refer to run_target again.  */
@@ -2141,7 +2142,7 @@ environment_info (char *var, int from_tty)
 {
   if (var)
     {
-      char *val = get_in_environ (current_inferior ()->environment, var);
+      char *val = current_inferior ()->environment->get_in_environ (var);
 
       if (val)
 	{
@@ -2159,11 +2160,12 @@ environment_info (char *var, int from_tty)
     }
   else
     {
-      char **vector = environ_vector (current_inferior ()->environment);
+      std::vector<char *> vector =
+	current_inferior ()->environment->get_environ_vector ();
 
-      while (*vector)
+      for (char *&elem : vector)
 	{
-	  puts_filtered (*vector++);
+	  puts_filtered (elem);
 	  puts_filtered ("\n");
 	}
     }
@@ -2225,10 +2227,10 @@ set_environment_command (char *arg, int from_tty)
       printf_filtered (_("Setting environment variable "
 			 "\"%s\" to null value.\n"),
 		       var);
-      set_in_environ (current_inferior ()->environment, var, "");
+      current_inferior ()->environment->set_in_environ (var, "");
     }
   else
-    set_in_environ (current_inferior ()->environment, var, val);
+    current_inferior ()->environment->set_in_environ (var, val);
   xfree (var);
 }
 
@@ -2240,13 +2242,10 @@ unset_environment_command (char *var, int from_tty)
       /* If there is no argument, delete all environment variables.
          Ask for confirmation if reading from the terminal.  */
       if (!from_tty || query (_("Delete all environment variables? ")))
-	{
-	  free_environ (current_inferior ()->environment);
-	  current_inferior ()->environment = make_environ ();
-	}
+	current_inferior ()->environment->reinit_environ ();
     }
   else
-    unset_in_environ (current_inferior ()->environment, var);
+    current_inferior ()->environment->unset_in_environ (var);
 }
 
 /* Handle the execution path (PATH variable).  */
@@ -2257,8 +2256,8 @@ static void
 path_info (char *args, int from_tty)
 {
   puts_filtered ("Executable and object file path: ");
-  puts_filtered (get_in_environ (current_inferior ()->environment,
-				 path_var_name));
+  puts_filtered (current_inferior ()->environment->get_in_environ
+		 (path_var_name));
   puts_filtered ("\n");
 }
 
@@ -2271,13 +2270,13 @@ path_command (char *dirname, int from_tty)
   const char *env;
 
   dont_repeat ();
-  env = get_in_environ (current_inferior ()->environment, path_var_name);
+  env = current_inferior ()->environment->get_in_environ (path_var_name);
   /* Can be null if path is not set.  */
   if (!env)
     env = "";
   exec_path = xstrdup (env);
   mod_path (dirname, &exec_path);
-  set_in_environ (current_inferior ()->environment, path_var_name, exec_path);
+  current_inferior ()->environment->set_in_environ (path_var_name, exec_path);
   xfree (exec_path);
   if (from_tty)
     path_info ((char *) NULL, from_tty);
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 4ae265e..2529f1a 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -99,7 +99,6 @@ free_inferior (struct inferior *inf)
   inferior_free_data (inf);
   xfree (inf->args);
   xfree (inf->terminal);
-  free_environ (inf->environment);
   target_desc_info_free (inf->tdesc_info);
   xfree (inf->priv);
   xfree (inf);
@@ -147,8 +146,7 @@ add_inferior_silent (int pid)
       last->next = inf;
     }
 
-  inf->environment = make_environ ();
-  init_environ (inf->environment);
+  inf->environment = std::unique_ptr<gdb_environ> (new gdb_environ);
 
   inferior_alloc_data (inf);
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index bf06ac1..c1d2f87 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -40,6 +40,9 @@ struct target_desc_info;
 /* For struct frame_id.  */
 #include "frame.h"
 
+/* For gdb_environ.  */
+#include "environ.h"
+
 #include "progspace.h"
 #include "registry.h"
 
@@ -356,7 +359,7 @@ struct inferior
 
   /* Environment to use for running inferior,
      in format described in environ.h.  */
-  struct gdb_environ *environment;
+  std::unique_ptr<gdb_environ> environment;
 
   /* Nonzero if this child process was attached rather than
      forked.  */
diff --git a/gdb/mi/mi-cmd-env.c b/gdb/mi/mi-cmd-env.c
index 4093178..0a909c9 100644
--- a/gdb/mi/mi-cmd-env.c
+++ b/gdb/mi/mi-cmd-env.c
@@ -167,7 +167,7 @@ mi_cmd_env_path (const char *command, char **argv, int argc)
   else
     {
       /* Otherwise, get current path to modify.  */
-      env = get_in_environ (current_inferior ()->environment, path_var_name);
+      env = current_inferior ()->environment->get_in_environ (path_var_name);
 
       /* Can be null if path is not set.  */
       if (!env)
@@ -178,9 +178,9 @@ mi_cmd_env_path (const char *command, char **argv, int argc)
   for (i = argc - 1; i >= 0; --i)
     env_mod_path (argv[i], &exec_path);
 
-  set_in_environ (current_inferior ()->environment, path_var_name, exec_path);
+  current_inferior ()->environment->set_in_environ (path_var_name, exec_path);
   xfree (exec_path);
-  env = get_in_environ (current_inferior ()->environment, path_var_name);
+  env = current_inferior ()->environment->get_in_environ (path_var_name);
   uiout->field_string ("path", env);
 }
 
@@ -270,7 +270,7 @@ mi_cmd_inferior_tty_show (const char *command, char **argv, int argc)
 void 
 _initialize_mi_cmd_env (void)
 {
-  struct gdb_environ *environment;
+  std::unique_ptr<gdb_environ> environment (new gdb_environ);
   const char *env;
 
   /* We want original execution path to reset to, if desired later.
@@ -278,13 +278,10 @@ _initialize_mi_cmd_env (void)
      current_inferior ()->environment.  Also, there's no obvious
      place where this code can be moved such that it surely run
      before any code possibly mangles original PATH.  */
-  environment = make_environ ();
-  init_environ (environment);
-  env = get_in_environ (environment, path_var_name);
+  env = environment->get_in_environ (path_var_name);
 
   /* Can be null if path is not set.  */
   if (!env)
     env = "";
   orig_path = xstrdup (env);
-  free_environ (environment);
 }
diff --git a/gdb/solib.c b/gdb/solib.c
index af94383..6260df9 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -350,16 +350,16 @@ solib_find_1 (const char *in_pathname, int *fd, int is_solib)
 
   /* If not found, next search the inferior's $PATH environment variable.  */
   if (found_file < 0 && sysroot == NULL)
-    found_file = openp (get_in_environ (current_inferior ()->environment,
-					"PATH"),
+    found_file = openp (current_inferior ()->environment->get_in_environ
+			("PATH"),
 			OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, in_pathname,
 			O_RDONLY | O_BINARY, &temp_pathname);
 
   /* If not found, and we're looking for a solib, next search the
      inferior's $LD_LIBRARY_PATH environment variable.  */
   if (is_solib && found_file < 0 && sysroot == NULL)
-    found_file = openp (get_in_environ (current_inferior ()->environment,
-					"LD_LIBRARY_PATH"),
+    found_file = openp (current_inferior ()->environment->get_in_environ
+			("LD_LIBRARY_PATH"),
 			OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, in_pathname,
 			O_RDONLY | O_BINARY, &temp_pathname);
 
-- 
2.9.3