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

Sergio Durigan Junior sergiodj@redhat.com
Tue Apr 18 03:03:00 GMT 2017


Changes from v2 (mostly from Simon's review):

- Not using std::unique_ptr for gdb_environ anymore.

- Got rid of "_environ" suffixes from the names of the methods.

- Remove (void) from methods without parameters.

- Constify return of 'get' method.

- Change 'const char *' for 'const std::string &' on methods'
  parameters.

- Returning a 'const std::vector<char *> &' on the 'get_vector'
  method.

- Typos, and other minor nits.



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): Declare object 'iconv_env'.
	Update code to use 'iconv_env' object.  Remove call to
	'free_environ'.
	* common/environ.c: Include <utility>.
	(make_environ): Delete function.
	(gdb_environ::init): New function.
	(free_environ): Delete function.
	(gdb_environ::clear): New function.
	(gdb_environ::gdb_environ): New function.
	(init_environ): Delete function.
	(gdb_environ::~gdb_environ): New function.
	(gdb_environ::reinit): Likewise.
	(gdb_environ::get): Likewise.
	(environ_vector): Delete function.
	(unset_in_vector): New function.
	(set_in_environ): Delete function.
	(gdb_environ::set): New function.
	(unset_in_environ): Delete function.
	(gdb_environ::unset): New function.
	(gdb_environ::get_char_vector): Likewise.
	(gdb_environ::get_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_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 (inferior::~inferior): Delete call to 'free_environ'.
	* inferior.h: Remove forward declaration of 'struct gdb_environ'.
	Include "environ.h".
	(class inferior) <environment>: Change type from 'struct
	gdb_environ' to '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 '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 | 223 ++++++++++++++++++++++++---------------------------
 gdb/common/environ.h |  64 ++++++++++-----
 gdb/infcmd.c         |  30 ++++---
 gdb/inferior.c       |   3 -
 gdb/inferior.h       |   6 +-
 gdb/mi/mi-cmd-env.c  |  13 ++-
 gdb/solib.c          |   7 +-
 8 files changed, 181 insertions(+), 176 deletions(-)

diff --git a/gdb/charset.c b/gdb/charset.c
index f55e482..236e918 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;
+  gdb_environ iconv_env;
   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 ("LANGUAGE", "C");
+  iconv_env.set ("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_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..131835e 100644
--- a/gdb/common/environ.c
+++ b/gdb/common/environ.c
@@ -18,165 +18,156 @@
 #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 ()
 {
-  struct gdb_environ *e;
+  extern char **environ;
+
+  if (environ == NULL)
+    return;
 
-  e = XNEW (struct gdb_environ);
+  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->allocated = 10;
-  e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char *));
-  e->vector[0] = 0;
-  return e;
+      this->m_environ_map.insert (std::make_pair (var, value));
+      this->m_environ_vector.push_back (xstrdup (environ[i]));
+    }
+
+  /* The last element of the vector is always going to be NULL.  */
+  this->m_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 ()
 {
-  char **vector = e->vector;
+  this->m_environ_map.clear ();
+  for (char *v : this->m_environ_vector)
+    xfree (v);
+  this->m_environ_vector.clear ();
+}
 
-  while (*vector)
-    xfree (*vector++);
+/* See common/environ.h.  */
 
-  xfree (e->vector);
-  xfree (e);
+gdb_environ::gdb_environ ()
+{
+  this->init ();
 }
 
-/* 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 ()
 {
-  extern char **environ;
-  int i;
+  this->clear ();
+}
 
-  if (environ == NULL)
-    return;
+/* See common/environ.h.  */
+
+void
+gdb_environ::reinit ()
+{
+  this->clear ();
+  this->init ();
+}
 
-  for (i = 0; environ[i]; i++) /*EMPTY */ ;
+/* See common/environ.h.  */
 
-  if (e->allocated < i)
+const char *
+gdb_environ::get (const std::string &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->m_environ_map.at (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.  */
+/* Unset (delete) the environment variable VAR on the environment
+   vector V.  */
 
-char **
-environ_vector (struct gdb_environ *e)
+static void
+unset_in_vector (std::vector<char *> &v, const std::string &var)
 {
-  return e->vector;
+  std::string match = var + '=';
+  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;
+      }
 }
-
-/* Return the value in environment E of variable VAR.  */
 
-char *
-get_in_environ (const struct gdb_environ *e, 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;
-}
-
-/* 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 (const std::string &var, const std::string &value)
 {
-  int i;
-  int len = strlen (var);
-  char **vector = e->vector;
-  char *s;
+  std::unordered_map<std::string, std::string>::iterator el;
 
-  for (i = 0; (s = vector[i]) != NULL; i++)
-    if (strncmp (s, var, len) == 0 && s[len] == '=')
-      break;
+  el = this->m_environ_map.find (var);
 
-  if (s == 0)
+  if (el != this->m_environ_map.end ())
     {
-      if (i == e->allocated)
+      if (el->second == value)
+	return;
+      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 = 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_vector (this->m_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->m_environ_map.insert (std::make_pair (var, value));
+
+  this->m_environ_vector.insert (this->m_environ_vector.end () - 1,
+				 xstrdup (std::string (var
+						       + '='
+						       + 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 (const std::string &var)
 {
-  int len = strlen (var);
-  char **vector = e->vector;
-  char *s;
+  this->m_environ_map.erase (var);
+  unset_in_vector (this->m_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_char_vector () const
+{
+  return const_cast<char **> (&m_environ_vector[0]);
+}
+
+/* See common/environ.h.  */
+
+const std::vector<char *> &
+gdb_environ::get_vector () const
+{
+  return this->m_environ_vector;
 }
diff --git a/gdb/common/environ.h b/gdb/common/environ.h
index 3ace69e..df5e381 100644
--- a/gdb/common/environ.h
+++ b/gdb/common/environ.h
@@ -17,33 +17,57 @@
 #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 seen 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 ();
 
-extern void init_environ (struct gdb_environ *);
+  /* Return the value in the environment for the variable VAR.  The
+     return pointer is only valid as long as VAR is not
+     removed/replaced from the environment.  */
+  const char *get (const std::string &var) const;
 
-extern char *get_in_environ (const struct gdb_environ *, const char *);
+  /* Store VAR=VALUE in the environment.  */
+  void set (const std::string &var, const std::string &value);
 
-extern void set_in_environ (struct gdb_environ *, const char *, const char *);
+  /* Unset VAR in environment.  */
+  void unset (const std::string &var);
 
-extern void unset_in_environ (struct gdb_environ *, const char *);
+  /* Return the environment vector represented as a 'char **'.  */
+  char **get_char_vector () const;
 
-extern char **environ_vector (struct gdb_environ *);
+  /* Return the environment vector.  */
+  const std::vector<char *> &get_vector () const;
+
+private:
+  /* Helper function that initializes our data structures with the
+     environment variables.  */
+  void init ();
+
+  /* Helper function that clears our data structures.  */
+  void clear ();
+
+  /* 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> m_environ_map;
+
+  /* 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 *> m_environ_vector;
+};
 
 #endif /* defined (ENVIRON_H) */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 22b2c7a..fb27572 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_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);
+      const char *val = current_inferior ()->environment.get (var);
 
       if (val)
 	{
@@ -2159,11 +2160,12 @@ environment_info (char *var, int from_tty)
     }
   else
     {
-      char **vector = environ_vector (current_inferior ()->environment);
+      const std::vector<char *> &vector =
+	current_inferior ()->environment.get_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 (var, "");
     }
   else
-    set_in_environ (current_inferior ()->environment, var, val);
+    current_inferior ()->environment.set (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 ();
     }
   else
-    unset_in_environ (current_inferior ()->environment, var);
+    current_inferior ()->environment.unset (var);
 }
 
 /* Handle the execution path (PATH variable).  */
@@ -2257,8 +2256,7 @@ 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 (path_var_name));
   puts_filtered ("\n");
 }
 
@@ -2271,13 +2269,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 (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 (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 327590a..b0cdaf6 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -100,7 +100,6 @@ inferior::~inferior ()
   inferior_free_data (inf);
   xfree (inf->args);
   xfree (inf->terminal);
-  free_environ (inf->environment);
   target_desc_info_free (inf->tdesc_info);
   xfree (inf->priv);
 }
@@ -108,10 +107,8 @@ inferior::~inferior ()
 inferior::inferior (int pid_)
   : num (++highest_inferior_num),
     pid (pid_),
-    environment (make_environ ()),
     registry_data ()
 {
-  init_environ (this->environment);
   inferior_alloc_data (this);
 }
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index b5eb3d1..dd74d4d 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -30,7 +30,6 @@ struct regcache;
 struct ui_out;
 struct terminal_info;
 struct target_desc_info;
-struct gdb_environ;
 struct continuation;
 
 /* For bpstat.  */
@@ -42,6 +41,9 @@ struct continuation;
 /* For struct frame_id.  */
 #include "frame.h"
 
+/* For gdb_environ.  */
+#include "environ.h"
+
 #include "progspace.h"
 #include "registry.h"
 
@@ -362,7 +364,7 @@ public:
 
   /* Environment to use for running inferior,
      in format described in environ.h.  */
-  gdb_environ *environment = NULL;
+  gdb_environ environment;
 
   /* True if this child process was attached rather than forked.  */
   bool attach_flag = false;
diff --git a/gdb/mi/mi-cmd-env.c b/gdb/mi/mi-cmd-env.c
index 4093178..37c0745 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 (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 (path_var_name, exec_path);
   xfree (exec_path);
-  env = get_in_environ (current_inferior ()->environment, path_var_name);
+  env = current_inferior ()->environment.get (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;
+  gdb_environ environment;
   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 (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..27eb451 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -350,16 +350,15 @@ 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 ("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
+			("LD_LIBRARY_PATH"),
 			OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, in_pathname,
 			O_RDONLY | O_BINARY, &temp_pathname);
 
-- 
2.9.3



More information about the Gdb-patches mailing list