Re: [PATCH v4] C++ify gdb/common/environ.c

[Pedro Alves <palves at redhat dot com>]

On 06/16/2017 07:01 PM, Sergio Durigan Junior wrote:
> On Friday, June 16 2017, Pedro Alves wrote:
> 
>> On 06/14/2017 08:22 PM, Sergio Durigan Junior wrote:
>>
>>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>>> index 5e5fcaa..133db1a 100644
>>> --- a/gdb/Makefile.in
>>> +++ b/gdb/Makefile.in
>>> @@ -530,14 +530,16 @@ SUBDIR_UNITTESTS_SRCS = \
>>>  	unittests/offset-type-selftests.c \
>>>  	unittests/optional-selftests.c \
>>>  	unittests/ptid-selftests.c \
>>> -	unittests/scoped_restore-selftests.c
>>> +	unittests/scoped_restore-selftests.c \
>>> +	unittests/environ-selftests.c
>>
>> Please keep the list sorted.
>>
>> (I'm guilty of missing that before too.)
> 
> Done.
> 
>>>  
>>>  SUBDIR_UNITTESTS_OBS = \
>>>  	function-view-selftests.o \
>>>  	offset-type-selftests.o \
>>>  	optional-selftests.o \
>>>  	ptid-selftests.o \
>>> -	scoped_restore-selftests.o
>>> +	scoped_restore-selftests.o \
>>> +	environ-selftests.o
>>
>> Ditto.
> 
> Done.
> 
>>> diff --git a/gdb/common/environ.c b/gdb/common/environ.c
>>> index 3145d01..657f2e0 100644
>>> --- a/gdb/common/environ.c
>>> +++ b/gdb/common/environ.c
>>> @@ -18,165 +18,102 @@
>>>  #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)
>>> +gdb_environ::gdb_environ ()
>>>  {
>>> -  struct gdb_environ *e;
>>> +}
>>>  
>>> -  e = XNEW (struct gdb_environ);
>>> +/* See common/environ.h.  */
>>>  
>>> -  e->allocated = 10;
>>> -  e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char *));
>>> -  e->vector[0] = 0;
>>> -  return e;
>>> +gdb_environ::~gdb_environ ()
>>> +{
>>> +  clear ();
>>>  }
>>>  
>>> -/* Free an environment and all the strings in it.  */
>>> +/* See common/environ.h.  */
>>>  
>>> -void
>>> -free_environ (struct gdb_environ *e)
>>> +gdb_environ::gdb_environ (gdb_environ &&e)
>>> +  : m_environ_vector (std::move (e.m_environ_vector))
>>>  {
>>> -  char **vector = e->vector;
>>> +}
>>>  
>>> -  while (*vector)
>>> -    xfree (*vector++);
>>> +/* See common/environ.h.  */
>>>  
>>> -  xfree (e->vector);
>>> -  xfree (e);
>>> +gdb_environ &
>>> +gdb_environ::operator= (gdb_environ &&e)
>>> +{
>>> +  clear ();
>>> +  m_environ_vector = std::move (e.m_environ_vector);
>>> +  return *this;
>>>  }
>>>  
>>> -/* 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::clear ()
>>>  {
>>> -  extern char **environ;
>>> -  int i;
>>> -
>>> -  if (environ == NULL)
>>> -    return;
>>> -
>>> -  for (i = 0; environ[i]; i++) /*EMPTY */ ;
>>> +  for (char *v : m_environ_vector)
>>> +    xfree (v);
>>> +  m_environ_vector.clear ();
>>> +}
>>>  
>>> -  if (e->allocated < i)
>>> -    {
>>> -      e->allocated = std::max (i, e->allocated + 10);
>>> -      e->vector = (char **) xrealloc ((char *) e->vector,
>>> -				      (e->allocated + 1) * sizeof (char *));
>>> -    }
>>> +/* See common/environ.h.  */
>>>  
>>> -  memcpy (e->vector, environ, (i + 1) * sizeof (char *));
>>> +const char *
>>> +gdb_environ::get (const std::string &var) const
>>> +{
>>
>> Does this need to be a std::string instead of "const char *"?
>> Callers always pass a string literal down, so this is going to
>> force a deep string dup for no good reason, AFAICS.
> 
> It doesn't.  Changed to const char *.
> 
>>
>>> +  size_t len = var.size ();
>>> +  const char *var_str = var.c_str ();
>>>  
>>> -  while (--i >= 0)
>>> -    {
>>> -      int len = strlen (e->vector[i]);
>>> -      char *newobj = (char *) xmalloc (len + 1);
>>> +  for (char *el : m_environ_vector)
>>> +    if (el != NULL && strncmp (el, var_str, len) == 0 && el[len] == '=')
>>> +      return &el[len + 1];
>>>  
>>> -      memcpy (newobj, e->vector[i], len + 1);
>>> -      e->vector[i] = newobj;
>>> -    }
>>> +  return NULL;
>>>  }
>>
>>> -char *
>>> -get_in_environ (const struct gdb_environ *e, const char *var)
>>> +void
>>> +gdb_environ::set (const std::string &var, const std::string &value)
>>
>> Ditto.
> 
> Likewise.
> 
>>>  {
>>> -  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];
>>> +  /* We have to unset the variable in the vector if it exists.  */
>>> +  unset (var);
>>>  
>>> -  return 0;
>>> +  /* Insert the element before the last one, which is always NULL.  */
>>> +  m_environ_vector.insert (m_environ_vector.end () - 1,
>>> +			   concat (var.c_str (), "=",
>>> +				   value.c_str (), NULL));
>>>  }
>>>  
>>> -/* 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::unset (const std::string &var)
>>
>> Ditto.
> 
> Likewise.
> 
>>
>>> -
>>> -  return;
>>> +  std::string match = var + '=';
>>> +  const char *match_str = match.c_str ();
>>> +
>>> +  /* We iterate until '.cend () - 1' because the last element is
>>> +     always NULL.  */
>>> +  for (std::vector<char *>::const_iterator el = m_environ_vector.cbegin ();
>>> +       el != m_environ_vector.cend () - 1;
>>> +       ++el)
>>> +    if (startswith (*el, match_str))
>>
>> In gdb_environ::set you used:
> 
> I assume you meant gdb_environ::get, right?

Maybe, (I don't have the patch handy anymore.)  Whatever the
other code that looped over all vars.

>> Nit: I find it a bit odd that the ctors/dtors are short but 
>> defined out of line, while this function is defined inline.
>> If I was looking at controlling what the compiler could inline,
>> then I'd do it the other way around -- small ctor/dtor in
>> the header, and this larger function out of line in the .c file.
> 
> Question: if I define a method inside the class, does this implicitly
> tell the compiler that I want to inline it, as oppose to defining the
> method outside?

It's not about inside vs outside.  It's about the compiler seeing the
body when compiling a foo.c file that includes the gdb_environ.h header.
The compiler is invoked on a per-compilation-unit base.  If you put the
method's definition outside the class but still in the gdb_environ.h header,
then the compiler would still be able to inline the method's body in the
foo.c compilation unit if it so chooses.  Of course, then you'd run into
multiple definition problems at link time.  So you can then mark
the definition as inline explicitly.  But that's no different from putting a
free function's definition in a header.

If you put the method instead in gdb_environ.c instead, then when the compiler
is compiling compilation unit foo.c, it has no idea what the body of
the method is, so it can't inline it.  Unless you build with -flto,
of course.

>> So we either always add a NULL to the vector, or we
>> change gdb_environ::get_char_vector instead, like:
>>
>>  char **
>>  gdb_environ::get_char_vector () const
>>  {
>>    if (m_environ_vector.empty ())
>>      {
>>        static const char *const empty_envp[1] = { NULL };
>>        return const_cast<char **> (empty_envp);
>>      }
>>    return const_cast<char **> (&m_environ_vector[0]);
>>  }
>>
>> This is OK because execve etc. are garanteed to never change
>> the envp they're passed.
> 
> Oh, good catch.  I prefer to just initialize the vector with a NULL
> value in the ctor; will do that now.

I'd prefer the other option.  Because then constructing gdb_environ
is dirt cheap and doesn't require heap memory.  We're constructing one
environ per inferior, even if we end up not setting any variable
[now thinking ahead to when we make this work with remote].

Thanks,
Pedro Alves