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

  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(-)
  

On 2017-04-17 23:03, Sergio Durigan Junior wrote:
> 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.

It still says O(n) ;)

> 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));

I see that make_pair has a move/xvalue version:

   template< class T1, class T2 >
   std::pair<V1,V2> make_pair( T1&& t, T2&& u );

Does that mean that we could/should do

   this->m_environ_map.insert (std::make_pair (std::move (var), std::move 
(value)));

in order to reuse the resources of the existing strings instead of 
copying them?

> +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 ();

You can remove the cast here.

I didn't think about it at first, but some unit tests for this class 
would be nice as well.

Thanks,

Simon
  

On 04/19/2017 05:56 AM, Simon Marchi wrote:
>>
>> +      return (char *) this->m_environ_map.at (var).c_str ();
> 
> You can remove the cast here.

And while at it, please write "m_environ_map" without the leading 
"this->".

(Likewise other similar cases, I haven't looked at the patch in
any detail.)

Thanks,
Pedro Alves
  

On 04/18/2017 04:03 AM, Sergio Durigan Junior wrote:

> -  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)

Please use unordered_map::find instead.  In general,
use "at" with exceptions when you're "sure" the element
exists.  It'll be simpler, more efficient, and less
roundabout, since at() essentially does a find() and then
throws if not found.

>      {
> -      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;
>      }
>  }
>  

>  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);

In C++, it's a good practice to avoid first initializing,
and then assigning separately, because the default constructor
will then do useless work.  (And in some cases, there may not even
be a default constructor.)

I'm not a fan or "auto" everywhere, but with iterators,
I find it OK:

   auto 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 ()));

I don't really understand the "m_environ_vector.end () - 1" here.

Also, that's a lot of unnecessary copying/duping in that last
statement.  Using concat instead of xstrdup + operator+ would
easily avoid it.

And you could easily avoid having to dup the string into both the
map and the vector by making the map's value be a const char *
instead of a std::string:

  std::unordered_map<std::string, const char *>

and then make a map's value point directly into the value
part of the string that is owned by the vector (as in,
the substring that starts at VALUE in "VAR=VALUE").

>  }
>  
> -/* 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);
> +}


> -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 ();

This should mention that the initial state is copied from the host's
environ.   I think the default ctor could use a similar comment.
I was going to suggest to call this clear() instead, to go
with the standard containers' terminology, until I realized what
"init" really does.  Or maybe even find a more explicit name,
reset_from_host_environ or some such.

Perhaps an even clearer approach would be to make the default ctor
not do a deep copy of the host's "environ", but instead add a
static factor method that returned such a new gdb_environ, like:

static gdb_environ
gdb_environ::from_host_environ ()
{
   // build/return a gdb_environ that wraps the host's environ global.
}

Not sure.  Only experimenting would tell.

Speaking of copying the host environ:

>  _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);
>  }

This usage of gdb_environ looks like pointless
wrapping / dupping / freeing to me -- I don't see why we
need to dup the whole host environment just to get at some
env variable.  Using good old getenv(3) directly should do,
and end up trimming off a bit of work from gdb's startup.

I could see perhaps wanting to avoid / optimize the linear
walks that getenv must do, if we have many getenv calls in
gdb, but then that suggests keeping a gdb_environ global that
is initialized early on and is reused.  But that would miss
any setenv call that changes the environment since that
gdb_environ is created, so I'd prefer not do that unless
we find a real need.

> +
> +private:
> +  /* Helper function that initializes our data structures with the
> +     environment variables.  */
> +  void init ();
> +
> +  /* Helper function that clears our data structures.  */
> +  void clear ();

s/function/method/g throughout (ChangeLog too).

> +
> +  /* A map representing the environment variables and their values.
> +     It is easier to store them using a map because of the faster
> +     lookups.  */

It's not that it's easier to store them.  Not having a map
would be even easier.  Just do a linear walk on the vector,
just like getenv does.  You want to instead say that we're
optimizing for get operations.  (I'm not sure this is the
right trade off for this class, but I'll go along with it.)

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

This should say that the entries are in VAR=VALUE form, and what
is the typical user that needs that format.

> +  std::vector<char *> m_environ_vector;
> +};
>  
>  #endif /* defined (ENVIRON_H) */

>    else
>      {
> -      char **vector = environ_vector (current_inferior ()->environment);
> +      const std::vector<char *> &vector =
> +	current_inferior ()->environment.get_vector ();

= goes on the next line.

>  
> -      while (*vector)
> +      for (char *elem : vector)
>  	{
> -	  puts_filtered (*vector++);
> +	  puts_filtered (elem);
>  	  puts_filtered ("\n");
>  	}
>      }

I'm not sure it's worth it to expose the std::vector implementation
detail just for this loop.  I don't really understand how this
can work though, given that the vector is NULL terminated.

I agree with Simon -- this is begging for some unit tests.  :-)

Thanks,
Pedro Alves
  

Hey,

Thanks for the review, and really sorry about the delay.  I have a few
comments and questions below.

On Wednesday, April 19 2017, Pedro Alves wrote:

> On 04/18/2017 04:03 AM, Sergio Durigan Junior wrote:
>
>> -  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)
>
> Please use unordered_map::find instead.  In general,
> use "at" with exceptions when you're "sure" the element
> exists.  It'll be simpler, more efficient, and less
> roundabout, since at() essentially does a find() and then
> throws if not found.

I decided to go the easier way and not use unordered_map.  I confess I
was having second thoughts myself when I posted the patch, and your
message only made me more confident that I don't want to follow this
route.

>>      {
>> -      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;
>>      }
>>  }
>>  
>
>>  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);
>
> In C++, it's a good practice to avoid first initializing,
> and then assigning separately, because the default constructor
> will then do useless work.  (And in some cases, there may not even
> be a default constructor.)
>
> I'm not a fan or "auto" everywhere, but with iterators,
> I find it OK:
>
>    auto el = this->m_environ_map.find (var);

Without the unordered_map, this is no longer a problem.  But of course,
thanks for the review.

>>  
>> -  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 ()));
>
> I don't really understand the "m_environ_vector.end () - 1" here.

This is needed because the last element of m_environ_vector needs to be
always NULL.  Therefore, this code is basically inserting the new
variable in the second-to-last position of the vector.  I made a comment
on top of it to clarify this part.

Also, there are other places where I need to iterate through the
elements of the vector, and I'm also using the "- 1" in these places.
I'll put comments where applicable.

> Also, that's a lot of unnecessary copying/duping in that last
> statement.  Using concat instead of xstrdup + operator+ would
> easily avoid it.

You're right, I've replaced it by a concat and it's simpler now.

> And you could easily avoid having to dup the string into both the
> map and the vector by making the map's value be a const char *
> instead of a std::string:
>
>   std::unordered_map<std::string, const char *>
>
> and then make a map's value point directly into the value
> part of the string that is owned by the vector (as in,
> the substring that starts at VALUE in "VAR=VALUE").

That was a good idea.

>>  }
>>  
>> -/* 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);
>> +}
>
>
>> -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 ();
>
> This should mention that the initial state is copied from the host's
> environ.   I think the default ctor could use a similar comment.
> I was going to suggest to call this clear() instead, to go
> with the standard containers' terminology, until I realized what
> "init" really does.  Or maybe even find a more explicit name,
> reset_from_host_environ or some such.

Sorry, I guess I wasn't aware of the importance of specifying that the
variables come from the host.  Somehow I thought this was implied.

I guess reset_from_host_environ is a good name for the method; my other
option would be "reinit_using_host_environ", but that's longer.

> Perhaps an even clearer approach would be to make the default ctor
> not do a deep copy of the host's "environ", but instead add a
> static factor method that returned such a new gdb_environ, like:
>
> static gdb_environ
> gdb_environ::from_host_environ ()
> {
>    // build/return a gdb_environ that wraps the host's environ global.
> }
>
> Not sure.  Only experimenting would tell.

Sorry, I'm not sure I understand your suggestion.  Please correct me if
I'm wrong.

You're basically saying that the default ctor shouldn't actually do
anything; instead, the class should have this new from_host_environ
method which would be the "official" ctor.  The users would then call
from_host_environ directly when they wanted an instance of the class,
and the method would be responsible for initializing the object.  Is
that correct?

If yes, I think I fail to see the advantage of this method over having a
normal ctor (aside from explicitly naming the new ctor after the fact
that we're using the host environ to build the object).

... after some reading ...

Oh, I think I see what you're suggesting.  Instead of building one
gdb_environ every time someone requests it, we build just one and pass
it along.  OK, now it makes sense.

> Speaking of copying the host environ:
>
>>  _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);
>>  }
>
> This usage of gdb_environ looks like pointless
> wrapping / dupping / freeing to me -- I don't see why we
> need to dup the whole host environment just to get at some
> env variable.  Using good old getenv(3) directly should do,
> and end up trimming off a bit of work from gdb's startup.

Absolutely.  I didn't pay close attention to this bit; I was just
mechanically converting the code.

> I could see perhaps wanting to avoid / optimize the linear
> walks that getenv must do, if we have many getenv calls in
> gdb, but then that suggests keeping a gdb_environ global that
> is initialized early on and is reused.  But that would miss
> any setenv call that changes the environment since that
> gdb_environ is created, so I'd prefer not do that unless
> we find a real need.

I'll make the code use getenv instead.

>> +
>> +private:
>> +  /* Helper function that initializes our data structures with the
>> +     environment variables.  */
>> +  void init ();
>> +
>> +  /* Helper function that clears our data structures.  */
>> +  void clear ();
>
> s/function/method/g throughout (ChangeLog too).

Ops, thanks.  Fixed.

>> +
>> +  /* A map representing the environment variables and their values.
>> +     It is easier to store them using a map because of the faster
>> +     lookups.  */
>
> It's not that it's easier to store them.  Not having a map
> would be even easier.  Just do a linear walk on the vector,
> just like getenv does.  You want to instead say that we're
> optimizing for get operations.  (I'm not sure this is the
> right trade off for this class, but I'll go along with it.)

Well, no need to worry about this anymore.  The new code doesn't use
unordered_map, as mentioned above.

>> +  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.  */
>
> This should say that the entries are in VAR=VALUE form, and what
> is the typical user that needs that format.

I'll expand the comment.

>> +  std::vector<char *> m_environ_vector;
>> +};
>>  
>>  #endif /* defined (ENVIRON_H) */
>
>>    else
>>      {
>> -      char **vector = environ_vector (current_inferior ()->environment);
>> +      const std::vector<char *> &vector =
>> +	current_inferior ()->environment.get_vector ();
>
> = goes on the next line.

Fixed.

>>  
>> -      while (*vector)
>> +      for (char *elem : vector)
>>  	{
>> -	  puts_filtered (*vector++);
>> +	  puts_filtered (elem);
>>  	  puts_filtered ("\n");
>>  	}
>>      }
>
> I'm not sure it's worth it to expose the std::vector implementation
> detail just for this loop.  I don't really understand how this
> can work though, given that the vector is NULL terminated.

I'm not exposing the std::vector anymore.  Instead, I'm just using the
regular 'char **' and looping until I find a NULL.

> I agree with Simon -- this is begging for some unit tests.  :-)

I'm writing them.  It's taking some time because it's the first time I'm
using the unittest framework.

Thanks,
  

On 05/01/2017 03:22 AM, Sergio Durigan Junior wrote:

>>> +  this->m_environ_vector.insert (this->m_environ_vector.end () - 1,
>>> +				 xstrdup (std::string (var
>>> +						       + '='
>>> +						       + value).c_str ()));
>>
>> I don't really understand the "m_environ_vector.end () - 1" here.
> 
> This is needed because the last element of m_environ_vector needs to be
> always NULL.  Therefore, this code is basically inserting the new
> variable in the second-to-last position of the vector.  I made a comment
> on top of it to clarify this part.

Ah, OK.

> 
> Also, there are other places where I need to iterate through the
> elements of the vector, and I'm also using the "- 1" in these places.
> I'll put comments where applicable.

OK, I'll take another look when you post it.

>>> -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 ();
>>
>> This should mention that the initial state is copied from the host's
>> environ.   I think the default ctor could use a similar comment.
>> I was going to suggest to call this clear() instead, to go
>> with the standard containers' terminology, until I realized what
>> "init" really does.  Or maybe even find a more explicit name,
>> reset_from_host_environ or some such.
> 
> Sorry, I guess I wasn't aware of the importance of specifying that the
> variables come from the host.  Somehow I thought this was implied.
> 
> I guess reset_from_host_environ is a good name for the method; my other
> option would be "reinit_using_host_environ", but that's longer.
> 
>> Perhaps an even clearer approach would be to make the default ctor
>> not do a deep copy of the host's "environ", but instead add a
>> static factor method that returned such a new gdb_environ, like:
>>
>> static gdb_environ
>> gdb_environ::from_host_environ ()
>> {
>>    // build/return a gdb_environ that wraps the host's environ global.
>> }
>>
>> Not sure.  Only experimenting would tell.
> 
> Sorry, I'm not sure I understand your suggestion.  Please correct me if
> I'm wrong.
> 
> You're basically saying that the default ctor shouldn't actually do
> anything; instead, the class should have this new from_host_environ
> method which would be the "official" ctor.  The users would then call
> from_host_environ directly when they wanted an instance of the class,
> and the method would be responsible for initializing the object.  Is
> that correct?
> 
> If yes, I think I fail to see the advantage of this method over having a
> normal ctor (aside from explicitly naming the new ctor after the fact
> that we're using the host environ to build the object).

The advantage was all in that "aside" -- to make the code
document itself.  It was totally non-obvious to me at first
that:

  gdb_environ env;

makes a copy of the host's environment.  I just assumed it created
an empty environment.  Probably because in my mind I don't think
it'll make sense for a remote process to inherit the host's
environment variables.

> 
> ... after some reading ...
> 
> Oh, I think I see what you're suggesting.  Instead of building one
> gdb_environ every time someone requests it, we build just one and pass
> it along.  OK, now it makes sense.

That wasn't actually what I was saying.  :-)

> 
>> Speaking of copying the host environ:
>>
>>>  _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);
>>>  }
>>
>> This usage of gdb_environ looks like pointless
>> wrapping / dupping / freeing to me -- I don't see why we
>> need to dup the whole host environment just to get at some
>> env variable.  Using good old getenv(3) directly should do,
>> and end up trimming off a bit of work from gdb's startup.
> 
> Absolutely.  I didn't pay close attention to this bit; I was just
> mechanically converting the code.
> 
>> I could see perhaps wanting to avoid / optimize the linear
>> walks that getenv must do, if we have many getenv calls in
>> gdb, but then that suggests keeping a gdb_environ global that
>> is initialized early on and is reused.  But that would miss
>> any setenv call that changes the environment since that
>> gdb_environ is created, so I'd prefer not do that unless
>> we find a real need.
> 
> I'll make the code use getenv instead.

Thanks.  That bit could/should go in as its own
preparatory/obvious patch first, no need to carry it in
the same patch.

Thanks,
Pedro Alves