Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Sergio,

I took a closer look at set/unset, I have a few comments.

> @@ -99,32 +107,104 @@ gdb_environ::get (const char *var) const
>  void
>  gdb_environ::set (const char *var, const char *value)
>  {
> +  char *fullvar = concat (var, "=", value, NULL);
> +
>    /* We have to unset the variable in the vector if it exists.  */
> -  unset (var);
> +  unset (var, false);
> 
>    /* Insert the element before the last one, which is always NULL.  */
> -  m_environ_vector.insert (m_environ_vector.end () - 1,
> -			   concat (var, "=", value, NULL));
> +  m_environ_vector.insert (m_environ_vector.end () - 1, fullvar);
> +
> +  /* Mark this environment variable as having been set by the user.
> +     This will be useful when we deal with setting environment
> +     variables on the remote target.  */
> +  m_user_set_env_list.push_back (fullvar);
> +
> +  /* If this environment variable is marked as unset by the user, then
> +     remove it from the list, because now the user wants to set
> +     it.  */
> +  for (std::vector<const char *>::iterator iter_user_unset
> +	 = m_user_unset_env_list.begin ();
> +       iter_user_unset != m_user_unset_env_list.end ();
> +       ++iter_user_unset)
> +    if (strcmp (var, *iter_user_unset) == 0)
> +      {
> +	void *v = (void *) *iter_user_unset;
> +
> +	m_user_unset_env_list.erase (iter_user_unset);
> +	xfree (v);
> +	break;
> +      }
>  }
> 
>  /* See common/environ.h.  */
> 
>  void
> -gdb_environ::unset (const char *var)
> +gdb_environ::unset (const char *var, bool update_unset_list)
>  {
>    size_t len = strlen (var);
> +  std::vector<char *>::iterator it_env;
> 
>    /* We iterate until '.end () - 1' because the last element is
>       always NULL.  */
> -  for (std::vector<char *>::iterator el = m_environ_vector.begin ();
> -       el != m_environ_vector.end () - 1;
> -       ++el)
> -    if (match_var_in_string (*el, var, len))
> -      {
> -	xfree (*el);
> -	m_environ_vector.erase (el);
> -	break;
> -      }
> +  for (it_env = m_environ_vector.begin ();
> +       it_env != m_environ_vector.end () - 1;
> +       ++it_env)
> +    if (match_var_in_string (*it_env, var, len))
> +      break;
> +
> +  if (it_env == m_environ_vector.end () - 1)
> +    {
> +      /* No element has been found.  */
> +      return;
> +    }

Do we want to support the use case to unset an environment variable that 
is defined on the remote system, but not on the local system?  If so, 
the function should probably not return immediately if the variable is 
not found in the env vector, so that the unset request ends up in the 
list of variables to unset.

> +
> +  std::vector<const char *>::iterator it_user_set_env;
> +  char *found_var = *it_env;
> +
> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
> +				 m_user_set_env_list.end (),
> +				 found_var);
> +  if (it_user_set_env != m_user_set_env_list.end ())
> +    {
> +      /* We found (and removed) the element from the user_set_env
> +	 vector.  */
> +      m_user_set_env_list.erase (it_user_set_env, 
> m_user_set_env_list.end ());
> +    }
> +
> +  if (update_unset_list)
> +    {
> +      bool found_in_unset = false;
> +
> +      for (const char *el : m_user_unset_env_list)
> +	if (strcmp (el, var) == 0)
> +	  {
> +	    found_in_unset = true;
> +	    break;
> +	  }
> +
> +      if (!found_in_unset)
> +	m_user_unset_env_list.push_back (xstrdup (var));
> +    }
> +
> +  m_environ_vector.erase (it_env);
> +  xfree (found_var);
> +}

I have the feeling that we can reduce the amount of boilerplate code in 
the set and unset methods by using std::set instead of std::vector.  
Performance-wise this may not be very good, since for any reasonable 
amount of variables, the vector would probably be more efficient.  But 
its interface makes the code clearer and lighter, in my opinion.  I 
suppose we could always make something with a set-like interface and 
behavior implemented on top of a vector.

> +
> +/* See common/environ.h.  */
> +
> +void
> +gdb_environ::clear_user_set_env ()
> +{
> +  std::vector<const char *> copy = m_user_set_env_list;
> +
> +  for (const char *var : copy)
> +    {
> +      std::string varname (var);
> +
> +      varname.erase (varname.find ('='), std::string::npos);
> +      unset (varname.c_str (), false);
> +    }

I am not sure having a method like this is correct.  It is used in 
gdbserver when we want to "reset" the environment, that is forget all 
user-made modifications, both set and unset.  To do it correctly, it 
should include restoring the variables that have been unset or 
overwritten.  But as I said in my other mail, I think the simplest 
solution will be to restore the environment from a backup copy of the 
original env.  If we do that, this method won't be needed.

I have prototyped some of my suggestions to make sure they made sense.  
I thought I would push them somewhere, feel free to use whatever you 
want if that's useful to you:

https://github.com/simark/binutils-gdb/commits/remote-env

Thanks,

Simon