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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 05:26 PM, Simon Marchi wrote:
>> On 2017-06-19 17:44, Pedro Alves wrote:
>>> If we take the "always push a NULL on construction" approach, and
>>> we want moved-from gdb_environs to be valid, then yes.  Note how this
>>> results in extra heap allocations when e.g., returning a
>>> gdb_environ from functions by value, and makes std::vector<gdb_environ>
>>> much less efficient when it decides it needs to reallocate/move
>>> elements.  Representing the empty state with a cleared internal
>>> vector would avoid this.
>> 
>> Given the move case, since the goal is to be efficient, then yeah I
>> would agree
>> that it would make sense to make a little bit of efforts to avoid
>> allocating
>> memory for an objects we are almost certainly throwing away.
>> 
>> But still, in order to leave environ objects in a valid state after a
>> move and
>> to pedantically comply with the STL spec which says that the vector is
>> left in
>> an unspecified state, shouldn't we do a .clear () on the moved-from
>> vector after
>> the move?
>
> See accepted answer at:
>
>  https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty
>
> So the only case where it'd be needed would be in op=, and iff the 
> vectors had different allocators, which is not the case here.
> So no, it's not necessary.  But I'd be fine with calling it.
>
>> 
>>> Note BTW, that we need to be careful with self-move leaving the
>>> *this object in a valid state.
>> 
>> Should we just do
>> 
>> if (&other == this)
>>   return *this;
>
> Might not be necessary if without that the object ends up
> valid anyway.  But what you wrote is a safe bet.

So, what do you guys think about the patch below, which applies on top
of the original?

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

diff --git a/gdb/common/environ.c b/gdb/common/environ.c
index 09860c9..944276c 100644
--- a/gdb/common/environ.c
+++ b/gdb/common/environ.c
@@ -25,13 +25,18 @@
 gdb_environ &
 gdb_environ::operator= (gdb_environ &&e)
 {
-  clear ();
+  if (&e == this)
+    return *this;
+
   m_environ_vector = std::move (e.m_environ_vector);
+  e.m_environ_vector.clear ();
+  e.m_environ_vector.push_back (NULL);
   return *this;
 }
 
 /* Create a gdb_environ object using the host's environment
    variables.  */
+
 gdb_environ gdb_environ::from_host_environ ()
 {
   extern char **environ;
diff --git a/gdb/common/environ.h b/gdb/common/environ.h
index b882bc7..054813c 100644
--- a/gdb/common/environ.h
+++ b/gdb/common/environ.h
@@ -67,9 +67,7 @@ public:
   char **envp () const;
 
 private:
-  /* A vector containing the environment variables.  This is useful
-     for when we need to obtain a 'char **' with all the existing
-     variables.  */
+  /* A vector containing the environment variables.  */
   std::vector<char *> m_environ_vector;
 };
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index ee0754d..defa7b0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2151,12 +2151,11 @@ environment_info (char *var, int from_tty)
     {
       char **envp = current_inferior ()->environment.envp ();
 
-      if (envp != NULL)
-	for (int idx = 0; envp[idx] != NULL; ++idx)
-	  {
-	    puts_filtered (envp[idx]);
-	    puts_filtered ("\n");
-	  }
+      for (int idx = 0; envp[idx] != NULL; ++idx)
+	{
+	  puts_filtered (envp[idx]);
+	  puts_filtered ("\n");
+	}
     }
 }
 
diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
index 0bcc6cc..8670d58 100644
--- a/gdb/unittests/environ-selftests.c
+++ b/gdb/unittests/environ-selftests.c
@@ -27,36 +27,58 @@ namespace gdb_environ_tests {
 static void
 run_tests ()
 {
+  /* Set a test environment variable.  This will be unset at the end
+     of this function.  */
   if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
-    error ("Could not set environment variable for testing.");
+    error (_("Could not set environment variable for testing."));
 
   gdb_environ env;
 
+  /* When the vector is initialized, there should always be one NULL
+     element in it.  */
   SELF_CHECK (env.envp ()[0] == NULL);
 
+  /* Make sure that there is no other element.  */
   SELF_CHECK (env.get ("PWD") == NULL);
+
+  /* Check if unset followed by a set in an empty vector works.  */
   env.set ("PWD", "test");
+  SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0);
+  /* The second element must be NULL.  */
+  SELF_CHECK (env.envp ()[1] == NULL);
   env.unset ("PWD");
+  SELF_CHECK (env.envp ()[0] == NULL);
 
+  /* Initialize the environment vector using the host's environ.  */
   env = gdb_environ::from_host_environ ();
 
+  /* Our test environment variable should be present at the
+     vector.  */
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
 
+  /* Set our test variable to another value.  */
   env.set ("GDB_SELFTEST_ENVIRON", "test");
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "test") == 0);
 
+  /* And unset our test variable.  The variable still exists in the
+     host's environment, but doesn't exist in our vector.  */
   env.unset ("GDB_SELFTEST_ENVIRON");
   SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
 
+  /* Re-set the test variable.  */
   env.set ("GDB_SELFTEST_ENVIRON", "1");
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
 
+  /* When we clear our environ vector, there should be only one
+     element on it (NULL), and we shouldn't be able to get our test
+     variable.  */
   env.clear ();
-
   SELF_CHECK (env.envp ()[0] == NULL);
-
   SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
 
+  /* Reinitialize our environ vector using the host environ.  We
+     should be able to see one (and only one) instance of the test
+     variable.  */
   env = gdb_environ::from_host_environ ();
   char **envp = env.envp ();
   int num_found = 0;
@@ -64,11 +86,14 @@ run_tests ()
   for (size_t i = 0; envp[i] != NULL; ++i)
     if (strcmp (envp[i], "GDB_SELFTEST_ENVIRON=1") == 0)
       ++num_found;
-
   SELF_CHECK (num_found == 1);
 
+  /* Get rid of our test variable.  */
   unsetenv ("GDB_SELFTEST_ENVIRON");
 
+  /* Test the case when we set a variable A, then set a variable B,
+     then unset A, and make sure that we cannot find A in the environ
+     vector, but can still find B.  */
   env.set ("GDB_SELFTEST_ENVIRON_1", "aaa");
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
 
@@ -78,6 +103,21 @@ run_tests ()
   env.unset ("GDB_SELFTEST_ENVIRON_1");
   SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON_1") == NULL);
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
+
+  env.clear ();
+
+  /* Test that after a std::move the moved-from object is left at a
+     valid state (i.e., its only element is NULL).  */
+  env.set ("A", "1");
+  SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+  gdb_environ env2;
+  env2 = std::move (env);
+  SELF_CHECK (env.envp ()[0] == NULL);
+  SELF_CHECK (strcmp (env2.get ("A"), "1") == 0);
+  SELF_CHECK (env2.envp ()[1] == NULL);
+  env.set ("B", "2");
+  SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
+  SELF_CHECK (env.envp ()[1] == NULL);
 }
 } /* namespace gdb_environ */
 } /* namespace selftests */