Patchwork [v6] C++ify gdb/common/environ.c

login
register
mail settings
Submitter Sergio Durigan Junior
Date June 19, 2017, 5:59 p.m.
Message ID <87tw3bx3i4.fsf@redhat.com>
Download mbox | patch
Permalink /patch/21111/
State New
Headers show

Comments

Sergio Durigan Junior - June 19, 2017, 5:59 p.m.
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,
Pedro Alves - June 19, 2017, 6:09 p.m.
On 06/19/2017 06:59 PM, Sergio Durigan Junior wrote:
> 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?

Missed fixing move ctor?

+  /* Move constructor.  */
+  gdb_environ (gdb_environ &&e)
+    : m_environ_vector (std::move (e.m_environ_vector))
+  {}

Thanks,
Pedro Alves
Sergio Durigan Junior - June 19, 2017, 6:23 p.m.
On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 06:59 PM, Sergio Durigan Junior wrote:
>> 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?
>
> Missed fixing move ctor?
>
> +  /* Move constructor.  */
> +  gdb_environ (gdb_environ &&e)
> +    : m_environ_vector (std::move (e.m_environ_vector))
> +  {}

Indeed.  Fixed now.  I'll submit v7.

Thanks,
Pedro Alves - June 19, 2017, 6:36 p.m.
On 06/19/2017 07:23 PM, Sergio Durigan Junior wrote:
> On Monday, June 19 2017, Pedro Alves wrote:

>> Missed fixing move ctor?
>>
>> +  /* Move constructor.  */
>> +  gdb_environ (gdb_environ &&e)
>> +    : m_environ_vector (std::move (e.m_environ_vector))
>> +  {}
> 
> Indeed.  Fixed now.  I'll submit v7.

Make sure to add unit tests for the move ctor.  E.g., reuse
the move assign tests.

Thanks,
Pedro Alves
Pedro Alves - June 19, 2017, 6:38 p.m.
On 06/19/2017 07:36 PM, Pedro Alves wrote:
> 
> On 06/19/2017 07:23 PM, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, Pedro Alves wrote:
> 
>>> Missed fixing move ctor?
>>>
>>> +  /* Move constructor.  */
>>> +  gdb_environ (gdb_environ &&e)
>>> +    : m_environ_vector (std::move (e.m_environ_vector))
>>> +  {}
>>
>> Indeed.  Fixed now.  I'll submit v7.
> 
> Make sure to add unit tests for the move ctor.  E.g., reuse
> the move assign tests.

And self-move tests...

Thanks,
Pedro Alves

Patch

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 */