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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, August 01 2017, Simon Marchi wrote:

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

You're right.  This specific behaviour is not implemented yet.  I guess
I replied too early saying that this is working, but it's not.

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

Aha, now it makes sense :-).  And no worries, it's always good to
make sure that we're on the right path.

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

Fair enough.  I'll use strlen then :-).

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/