This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v6] C++ify gdb/common/environ.c
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 */