This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>, Eli Zaretskii <eliz at gnu dot org>
- Date: Tue, 01 Aug 2017 11:34:56 +0200
- Subject: Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior
- Authentication-results: sourceware.org; auth=none
- References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> <e591752addd71e865c05fe958d18130f@polymtl.ca> <87wp6o9fv8.fsf@redhat.com>
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