[v2] C++ify gdb/common/environ.c
Commit Message
This is the version 2 of the patch. It doesn't have any huge changes,
just the necessary to make it work with the recent conversion of
'struct inferior' to a class.
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 (inferior::~inferior): Delete call to 'free_environ'.
(inferior::inferior): Creating new environment.
* inferior.h: Include "environ.h".
(class 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(-)
Comments
On 2017-04-15 14:50, Sergio Durigan Junior wrote:
> 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);
Can it be allocated on the stack?
> 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)
I think operator== will do what you want.
If you did a return here, you wouldn't need the needs_update variable.
I don't mind though, some people prefer a single return point.
> 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>
In the interface of gdb_environ, I think you could get rid of the
"environ" in method names. You can also remove the (void) for methods
without parameters.
>
> -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
s/as seeing/as seen/
> + 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;
Looking at the users, I think that this could return const char *. It
would also be good to mention that the pointer is valid as long as the
corresponding variable environment is not removed/replaced.
It could also return something based on std::string, but I don't know
how good it would be:
1. returning const std::string &: can't return "NULL".
2. returning gdb::optional<std::string>: it works but makes a copy of
the string, maybe it's ok since there isn't a ton of that. It would
however make the return value usable even after the corresponding
variable is removed from the env.
3. returning gdb::optional<const std::string &>: I don't think it can be
done.
4. returning gdb::optional<std::reference_wrapper<const std::string>>:
it works, but it gets a bit ugly.
One thing is sure, since you are constructing some std::string inside
the methods, you could change the parameters in the interface from const
char * to const std::string &. With the implicit string constructor
from const char *, the users won't have to be changed, but it will be
possible to pass an std::string directly.
>
> -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;
Return a const reference, and change the (sole) user of this method to
get a reference too?
> @@ -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)
Is there advantage of doing
for (char *&elem : vector)
vs
for (char *elem : vector)
?
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index b5eb3d1..e48d5cb 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -42,6 +42,9 @@ struct continuation;
> /* For struct frame_id. */
> #include "frame.h"
>
> +/* For gdb_environ. */
> +#include "environ.h"
> +
You can remove the "struct gdb_environ;" above this.
> #include "progspace.h"
> #include "registry.h"
>
> @@ -362,7 +365,7 @@ public:
>
> /* Environment to use for running inferior,
> in format described in environ.h. */
> - gdb_environ *environment = NULL;
> + std::unique_ptr<gdb_environ> environment;
Can this be simply
gdb_environ environment;
?
> @@ -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);
Again, can it be allocated on the stack?
Thanks,
Simon
On 2017-04-15 14:50, Sergio Durigan Junior wrote:
> 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.
Forgot to mention (probably a typo) that lookups in the map are O(1) on
average. If they were O(n), it wouldn't be better than looking up in a
vector :).
At least that's what it says here:
[1] http://en.cppreference.com/w/cpp/container/unordered_map/at
Simon
On Sunday, April 16 2017, Simon Marchi wrote:
> On 2017-04-15 14:50, Sergio Durigan Junior wrote:
>> 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.
>
> Forgot to mention (probably a typo) that lookups in the map are O(1)
> on average. If they were O(n), it wouldn't be better than looking up
> in a vector :).
You're right, they're O(1). I was thinking about the vector when I
wrote. Thanks for the correction!
Thanks for the review, Simon. I don't know why I didn't receive this
message on my INBOX, so it took a little bit until I saw it on
gdb-patches.
On Saturday, April 15 2017, Simon Marchi wrote:
> On 2017-04-15 14:50, Sergio Durigan Junior wrote:
>> 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);
>
> Can it be allocated on the stack?
Yeah, it can. It's probably an overkill to use a unique_ptr here
because the object will be short-lived anyway. I'll change that.
>> 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)
>
> I think operator== will do what you want.
Good catch, thanks.
> If you did a return here, you wouldn't need the needs_update variable.
> I don't mind though, some people prefer a single return point.
No, you're totally right, I overlooked this detail. Changed to a
return.
>> 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>
>
> In the interface of gdb_environ, I think you could get rid of the
> "environ" in method names. You can also remove the (void) for methods
> without parameters.
Indeed, the "environ" in the names doesn't make sense anymore.
As for removing (void)... I personally like to make it explicit, but I
understand we're living in other times now (C++11 and all). I'll
remove.
>>
>> -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
>
> s/as seeing/as seen/
Fixed.
>> + 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;
>
> Looking at the users, I think that this could return const char *. It
> would also be good to mention that the pointer is valid as long as the
> corresponding variable environment is not removed/replaced.
Fair enough. I constified the return type of the 'get' method, and
changed the callers accordingly. I've also added the
> It could also return something based on std::string, but I don't know
> how good it would be:
>
> 1. returning const std::string &: can't return "NULL".
> 2. returning gdb::optional<std::string>: it works but makes a copy of
> the string, maybe it's ok since there isn't a ton of that. It would
> however make the return value usable even after the corresponding
> variable is removed from the env.
> 3. returning gdb::optional<const std::string &>: I don't think it can
> be done.
> 4. returning gdb::optional<std::reference_wrapper<const std::string>>:
> it works, but it gets a bit ugly.
Ahm, I'm not sure. I mean, I know the rationale here (you're trying to
make sure that the return value only exists for as long as the
environment variable is there), but I don't think it's necessary to
overcomplicate things for now. IMHO, a 'const char *' + the updated
comment are enough.
> One thing is sure, since you are constructing some std::string inside
> the methods, you could change the parameters in the interface from
> const char * to const std::string &. With the implicit string
> constructor from const char *, the users won't have to be changed, but
> it will be possible to pass an std::string directly.
I confess this hasn't even crossed my mind. Thanks, I adopted the idea.
>>
>> -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;
>
> Return a const reference, and change the (sole) user of this method to
> get a reference too?
Heh, I had done that before I got to this part of the e-mail. Thanks.
>> @@ -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)
>
> Is there advantage of doing
>
> for (char *&elem : vector)
>
> vs
>
> for (char *elem : vector)
>
> ?
No, this is just a brain fart. I wrote this code when I was also
dealing with the fork-inferior sharing stuff, and you can see the same
pattern there. Fixed.
>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index b5eb3d1..e48d5cb 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -42,6 +42,9 @@ struct continuation;
>> /* For struct frame_id. */
>> #include "frame.h"
>>
>> +/* For gdb_environ. */
>> +#include "environ.h"
>> +
>
> You can remove the "struct gdb_environ;" above this.
Fixed.
>
>> #include "progspace.h"
>> #include "registry.h"
>>
>> @@ -362,7 +365,7 @@ public:
>>
>> /* Environment to use for running inferior,
>> in format described in environ.h. */
>> - gdb_environ *environment = NULL;
>> + std::unique_ptr<gdb_environ> environment;
>
> Can this be simply
>
> gdb_environ environment;
>
> ?
>
>> @@ -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);
>
> Again, can it be allocated on the stack?
Yes, already done that. Thanks.
I'll send v3 soon.
@@ -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)
{
@@ -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;
}
@@ -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) */
@@ -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);
@@ -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,9 @@ inferior::~inferior ()
inferior::inferior (int pid_)
: num (++highest_inferior_num),
pid (pid_),
- environment (make_environ ()),
+ environment (new gdb_environ),
registry_data ()
{
- init_environ (this->environment);
inferior_alloc_data (this);
}
@@ -42,6 +42,9 @@ struct continuation;
/* For struct frame_id. */
#include "frame.h"
+/* For gdb_environ. */
+#include "environ.h"
+
#include "progspace.h"
#include "registry.h"
@@ -362,7 +365,7 @@ public:
/* Environment to use for running inferior,
in format described in environ.h. */
- gdb_environ *environment = NULL;
+ std::unique_ptr<gdb_environ> environment;
/* True if this child process was attached rather than forked. */
bool attach_flag = false;
@@ -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);
}
@@ -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);