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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Sergio,

On 2017-08-01 04:33, Sergio Durigan Junior wrote:
> > There is this use case for which the behavior is different between
> > native and remote, related to unset
> >
> > native:
> >
> > (gdb inf1) file /usr/bin/env
> > (gdb inf1) unset environment DISPLAY
> > (gdb inf1) r  # DISPLAY is not there
> > (gdb inf1) add-inferior -exec /usr/bin/env
> > (gdb inf1) inferior 2
> > (gdb inf2) r  # DISPLAY is there
> >
> > remote:
> >
> > (gdb inf1) tar ext :1234
> > (gdb inf1) file /usr/bin/env
> > (gdb inf1) set remote exec-file /usr/bin/env
> > (gdb inf1) unset environment DISPLAY
> > (gdb inf1) r  # DISPLAY is not there
> > (gdb inf1) add-inferior -exec /usr/bin/env
> > (gdb inf1) inferior 2
> > (gdb inf2) set remote exec-file /usr/bin/env
> > (gdb inf2) r  # DISPLAY is not there
> >
> > I think that's because in GDB, we make a new pristine copy of the host
> > environment for every inferior, which we don't in gdbserver.
>
> Thanks for the review, Simon.
>
> Yes, you're right, these cases are currently different because of the
> way we handle the environment on GDB and gdbserver.  On gdbserver we
> have 'our_environ', which is a global declared at server.c and that is
> passed to all inferiors being started.
>
> > The way I understand the QEnvironmentReset is that the remote agent
> > (gdbserver) should forget any previous modification to the environment
> > made using QEnvironmentHexEncoded and QEnvironmentUnset and return the
> > environment to its original state, when it was launched.  This should
> > allow supporting the use case above.  To implement that properly, we
> > would need to keep a copy of gdbserver's initial environment, which we
> > could revert to when receiving a QEnvironmentReset.
>
> Yes, and we already do that on gdbserver; see the 'our_environ' global.

Maybe I'm reading the code wrong, but that's not what I understand.  
gdb_environ is never reset to gdbserver's original state.  So if the 
DISPLAY env var is present in the original environment and is unset with 
a QEnvironmentUnset, a QEnvironmentReset won't make it reappear with the 
current implementation.  But we would want it to be back, to support the 
scenario illustrated above, wouldn't we?

I originally talked about keeping a copy of the initial environment, but 
actually when receiving a QEnvironmentReset, I think gdbserver should 
simply do

  our_environ = gdb_environ::from_host_environ ();

> > In any case, I just want to make sure that the packets we introduce
> > are not the things that limit us.
>
> Sorry, I'm not sure I understood what you have in mind.  Could you
> explain in what ways we'd be limited by the new packets?

Oh, I just wanted to say that if the gdbserver implementation is not 
perfect yet, it's not the end of the world because that can always 
change.  But the behavior of the RSP packets is more difficult to change 
once it becomes a published interface, so we need to be careful that 
their documented behavior covers all the use cases we want to support.  
But I know you already know that, so I don't know why I said it in the 
first place :).

> > > +
> > > +  /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables.
> > > */
> > > +  void clear_user_set_env ();
> >
> > I wouldn't put the "Iterate through M_USER_UNSET_ENV_LIST" in the
> > comment, since that's implementation details.  The function
> > documentation should focus on the visible effects or goals of the
> > function (i.e. remove the user set/unset variables in that
> > environment).
>
> Good point.  I've updated the comment to:
>
>   /* Unset all variables that were previously set by the user, i.e.,
>      that were added by calling the SET method.  */
>   void clear_user_set_env ();

Sounds good.

> > Similar thing here, what we pass to str is a const char*, so it leads
> > to an unnecessary std::string construction.  Also, the interface of
> > the function is not well-suited to encode arbitrary binary data, which
> > could any byte.  One could use
> >
> >   str2hex (std::string (p, count), hex);
> >
> > but again why copy the data in the first place? So what about this:
> >
> > extern std::string bin2hex (const char *bin, int count);
>
> Not sure if it was a typo or if you really meant to propose overloading
> the bin2hex method and have another version of it that returns a
> std::string, but I like the idea.  I don't think we need to treat the
> first parameter as a 'const char *'; TBH, treating it like a 'const
> gdb_byte *' (just like the original version does) is more clear.  So I
> went ahead and implemented it this way.

Sorry, I should have been more explicit.  I really meant to overload 
bin2hex, since there's no point in limiting this function's scope to 
hex-encoding text strings, it can handle any binary data (of which a 
text strings are a subset).  But you are right, it should be const 
gdb_byte *.

> > > --- a/gdb/gdbserver/server.c
> > > +++ b/gdb/gdbserver/server.c
> > > @@ -631,6 +631,75 @@ handle_general_set (char *own_buf)
> > >        return;
> > >      }
> > >
> > > +  if (startswith (own_buf, "QEnvironmentReset"))
> > > +    {
> > > +      our_environ.clear_user_set_env ();
> > > +
> > > +      write_ok (own_buf);
> > > +      return;
> > > +    }
> > > +
> > > +  if (startswith (own_buf, "QEnvironmentHexEncoded:"))
> > > +    {
> > > +      const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:")
> > > -
> > > 1;
> >
> > You can also use strlen to avoid having to do -1, but either is fine
> > with me.
>
> I personally like using sizeof here and avoiding the call to strlen.

Ok, but remember that the compilers optimize those calls to strlen 
("literal") away to a constant.

Thanks,

Simon