This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Implement the ability to transmit 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>
- Date: Thu, 13 Jul 2017 23:47:24 +0200
- Subject: Re: [PATCH] Implement the ability to transmit environment variables to GDBserver when starting the inferior
- Authentication-results: sourceware.org; auth=none
- References: <20170629194106.23070-1-sergiodj@redhat.com>
On 2017-06-29 21:41, Sergio Durigan Junior wrote:
This is the first version of this patch, which aims to implement the
ability to transmit user-set environment variables to GDBserver when
starting the remote inferior.
User-set environment variables are only the variables that are
explicitly set by the user, using the 'set environment' command. This
means that variables that were already present in the environment when
starting GDB/GDBserver are not transmitted/considered by this feature.
The idea behind this patch is to store user-set environment variables
in a separate vector, part of gdb_environ, and provide this list to
extended_remote_create_inferior when the preparations to start the
inferior are happening. extended_remote_create_inferior, then,
iterates over this list and sends a new packet,
QEnvironmentHexEncoded, which contains the hex-encoded string that
represents the "VAR=VALUE" format (VALUE can be empty if the user set
a variable with a null value, by doing 'set environment VAR=').
The QEnvironmentHexEncoded packet is inspired on LLDB's extensions to
the RSP. Details about it can be seen here:
<https://raw.githubusercontent.com/llvm-mirror/lldb/master/docs/lldb-gdb-remote.txt>
I decided not to implement the QEnvironment packet because it is
considered deprecated by LLDB. This packet, on LLDB, serves the same
purpose of QEnvironmentHexEncoded, but sends the information using a
plain text, non-hex-encoded string.
This patch also includes updates to the documentation, testsuite, and
unit tests, without introducing regressions.
gdb/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* NEWS (Changes since GDB 8.0): Add entry mentioning new support
for sending environment variables to GDBserver.
(New remote packets): Add entry of QEnvironmentHexEncoded.
* common/environ.c (gdb_environ::operator=): Extend method to
handle m_user_env_list.
(gdb_environ::clear): Likewise.
(match_var_in_string): Change type of first parameter from 'char
*' to 'const char *'.
(gdb_environ::get): New variable 'fullvar'. Handle
m_user_env_list.
(gdb_environ::unset): Extend method to handle m_user_env_list.
(gdb_environ::user_envp): New method.
* common/environ.h (gdb_environ): Add NULL to m_user_env_list;
handle m_user_env_list on move constructor/assignment.
(user_envp): New method.
(m_user_env_list): New vector.
* remote.c: Include "environ.h". Add QEnvironmentHexEncoded.
(remote_protocol_features): Add
QEnvironmentHexEncoded packet.
(extended_remote_environment_support): New function.
(extended_remote_create_inferior): Call
extended_remote_environment_support.
(_initialize_remote): Add QEnvironmentHexEncoded packet config.
* unittests/environ-selftests.c (run_tests): Add tests for
m_user_env_list.
gdb/gdbserver/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* server.c (handle_general_set): Handle QEnvironmentHexEncoded
packet.
(handle_query): Inform remote that QEnvironmentHexEncoded is
supported.
gdb/doc/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.texinfo (set environment): Add @anchor. Explain that
environment variables set by the user are sent to GDBserver.
(Connecting) <Remote Packet>: Add "environment-hex-encoded"
and "QEnvironmentHexEncoded" to the table.
(Remote Protocol) <QEnvironmentHexEncoded>: New item, explaining
the packet.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.base/share-env-with-gdbserver.c: New file.
* gdb.base/share-env-with-gdbserver.exp: Likewise.
---
gdb/NEWS | 12 +++
gdb/common/environ.c | 59 +++++++++---
gdb/common/environ.h | 14 ++-
gdb/doc/gdb.texinfo | 45 +++++++++
gdb/gdbserver/server.c | 54 ++++++++++-
gdb/remote.c | 50 ++++++++++
gdb/testsuite/gdb.base/share-env-with-gdbserver.c | 40 ++++++++
.../gdb.base/share-env-with-gdbserver.exp | 105
+++++++++++++++++++++
gdb/unittests/environ-selftests.c | 30 ++++++
9 files changed, 395 insertions(+), 14 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.c
create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
diff --git a/gdb/NEWS b/gdb/NEWS
index 7c8a8f6..9026733 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,14 @@
*** Changes since GDB 8.0
+* On Unix systems, GDB now supports transmitting environment variables
+ to GDBserver that are to be passed to the inferior when it is being
+ started.
+
+ To inform GDB of environment variables that are to be transmitted to
+ GDBserver, use the "set environment" command. Only user set
+ environment variables are sent to GDBserver.
+
* On Unix systems, GDBserver now does globbing expansion and variable
substitution in inferior command line arguments.
@@ -14,6 +22,10 @@
* New remote packets
+QEnvironmentHexEncoded
+ Inform GDBserver of an environment variable that is to be passed to
+ the inferior when starting it.
+
QStartupWithShell
Indicates whether the inferior must be started with a shell or not.
diff --git a/gdb/common/environ.c b/gdb/common/environ.c
index 698bda3..da0af98 100644
--- a/gdb/common/environ.c
+++ b/gdb/common/environ.c
@@ -30,8 +30,11 @@ gdb_environ::operator= (gdb_environ &&e)
return *this;
m_environ_vector = std::move (e.m_environ_vector);
+ m_user_env_list = std::move (e.m_user_env_list);
e.m_environ_vector.clear ();
+ e.m_user_env_list.clear ();
e.m_environ_vector.push_back (NULL);
+ e.m_user_env_list.push_back (NULL);
return *this;
}
@@ -63,8 +66,10 @@ gdb_environ::clear ()
for (char *v : m_environ_vector)
xfree (v);
m_environ_vector.clear ();
+ m_user_env_list.clear ();
/* Always add the NULL element. */
m_environ_vector.push_back (NULL);
+ m_user_env_list.push_back (NULL);
}
/* Helper function to check if STRING contains an environment variable
@@ -72,7 +77,7 @@ gdb_environ::clear ()
if it contains, false otherwise. */
static bool
-match_var_in_string (char *string, const char *var, size_t var_len)
+match_var_in_string (const char *string, const char *var, size_t
var_len)
{
if (strncmp (string, var, var_len) == 0 && string[var_len] == '=')
return true;
@@ -99,12 +104,14 @@ 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);
/* 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);
+ m_user_env_list.insert (m_user_env_list.end () - 1, fullvar);
}
/* See common/environ.h. */
@@ -113,18 +120,38 @@ void
gdb_environ::unset (const char *var)
{
size_t len = strlen (var);
+ std::vector<char *>::iterator it_env;
+ std::vector<const char *>::iterator it_user_env;
+ char *found_var;
/* 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;
+ }
+
+ found_var = *it_env;
+
+ /* We iterate until '.end () - 1' because the last element is
+ always NULL. */
+ for (it_user_env = m_user_env_list.begin ();
+ it_user_env != m_user_env_list.end () - 1;
+ ++it_user_env)
+ if (match_var_in_string (*it_user_env, var, len))
+ break;
In this second loop, can you search by pointer value instead? A value
in m_user_env_list will always have a duplicate value in
m_environ_vector, right? If so, std::remove might be a good option.
+
+ m_environ_vector.erase (it_env);
+ if (it_user_env != m_user_env_list.end () - 1)
+ m_user_env_list.erase (it_user_env);
+ xfree (found_var);
}
/* See common/environ.h. */
@@ -134,3 +161,11 @@ gdb_environ::envp () const
{
return const_cast<char **> (&m_environ_vector[0]);
}
+
+/* See common/environ.h. */
+
+const char **
+gdb_environ::user_envp () const
+{
+ return const_cast<const char **> (&m_user_env_list[0]);
+}
For m_environ_p, we have the envp method the way it is because we have
to pass it to execve. We don't have such limitation for
m_user_env_list. We don't need to add a NULL element, and user_envp can
return a const reference to the vector instead.
diff --git a/gdb/common/environ.h b/gdb/common/environ.h
index 0bbb191..cce7f87 100644
--- a/gdb/common/environ.h
+++ b/gdb/common/environ.h
@@ -32,6 +32,7 @@ public:
If/when we add more variables to it, NULL will always be the
last element. */
m_environ_vector.push_back (NULL);
+ m_user_env_list.push_back (NULL);
}
~gdb_environ ()
@@ -41,12 +42,15 @@ public:
/* Move constructor. */
gdb_environ (gdb_environ &&e)
- : m_environ_vector (std::move (e.m_environ_vector))
+ : m_environ_vector (std::move (e.m_environ_vector)),
+ m_user_env_list (std::move (e.m_user_env_list))
{
/* Make sure that the moved-from vector is left at a valid
state (only one NULL element). */
e.m_environ_vector.clear ();
+ e.m_user_env_list.clear ();
e.m_environ_vector.push_back (NULL);
+ e.m_user_env_list.push_back (NULL);
}
/* Move assignment. */
@@ -73,9 +77,17 @@ public:
/* Return the environment vector represented as a 'char **'. */
char **envp () const;
+ /* Return the user-set environment vector represented as a 'const
+ char **'. */
+ const char **user_envp () const;
+
private:
/* A vector containing the environment variables. */
std::vector<char *> m_environ_vector;
+
+ /* The vector containing the environment variables set by the
+ user. */
+ std::vector<const char *> m_user_env_list;
};
#endif /* defined (ENVIRON_H) */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c167a86..d632523 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2363,6 +2363,7 @@ print the names and values of all environment
variables to be given to
your program. You can abbreviate @code{environment} as @code{env}.
@kindex set environment
+@anchor{set environment}
@item set environment @var{varname} @r{[}=@var{value}@r{]}
Set environment variable @var{varname} to @var{value}. The value
changes for your program (and the shell @value{GDBN} uses to launch
@@ -2391,6 +2392,10 @@ If necessary, you can avoid that by using the
@samp{env} program as a
wrapper instead of using @code{set environment}. @xref{set
exec-wrapper}, for an example doing just that.
+Environment variables that are set by the user are also transmitted to
+@command{gdbserver} to be used when starting the remote inferior.
+@pxref{QEnvironmentHexEncoded}.
+
@kindex unset environment
@item unset environment @var{varname}
Remove variable @var{varname} from the environment to be passed to
your
@@ -20816,6 +20821,10 @@ are:
@tab @code{QStartupWithShell}
@tab @code{set startup-with-shell}
+@item @code{environment-hex-encoded}
+@tab @code{QEnvironmentHexEncoded}
+@tab @code{set environment}
+
@item @code{conditional-breakpoints-packet}
@tab @code{Z0 and Z1}
@tab @code{Support for target-side breakpoint condition evaluation}
@@ -36480,6 +36489,42 @@ actually support starting the inferior using a
shell.
Use of this packet is controlled by the @code{set startup-with-shell}
command; @pxref{set startup-with-shell}.
+@item QEnvironmentHexEncoded:@var{hex-value}
+@anchor{QEnvironmentHexEncoded}
+@cindex environment variable, remote request
+@cindex @samp{QEnvironmentHexEncoded} packet
+On UNIX-like targets, it is possible to set environment variables that
+will be passed to the inferior during the startup process. This
+packet is used to inform @command{gdbserver} of an environment
+variable that has been defined by the user on @value{GDBN} (@pxref{set
+environment}).
+
+The packet is composed by @var{hex-value}, an hex encoded
+representation of the @var{name=value} format representing an
+environment variable. The name of the environment variable is
+represented by @var{name}, and the value to be assigned to the
+environment variable is represented by @var{value}. If the variable
+has no value (i.e., the value is @code{null}), then @var{value} will
+not be present.
+
+This packet is only available in extended mode (@pxref{extended
+mode}).
+
+Reply:
+@table @samp
+@item OK
+The request succeeded.
+@end table
+
+This packet is not probed by default; the remote stub must request it,
+by supplying an appropriate @samp{qSupported} response
+(@pxref{qSupported}). This should only be done on targets that
+actually support passing environment variables to the starting
+inferior.
+
+This packet is related to the @code{set environment} command;
+@pxref{set environment}.
+
@item qfThreadInfo
@itemx qsThreadInfo
@cindex list active threads, remote request
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3838351..622898d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -631,6 +631,57 @@ handle_general_set (char *own_buf)
return;
}
+ if (startswith (own_buf, "QEnvironmentHexEncoded:"))
+ {
+ const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:") -
1;
+ /* The final form of the environment variable. FINAL_VAR will
+ hold the 'VAR=VALUE' format. */
+ char *final_var = (char *) xcalloc (1, strlen (p) * 2);
+ struct cleanup *c = make_cleanup (xfree, final_var);
+ /* VAR_NAME will hold the environment variable name; VAR_VALUE
+ will hold its value. These will be just pointers to
+ FINAL_VAR. */
+ char *var_name, *var_value;
+
+ if (remote_debug)
+ debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
+
+ /* Un-hexify the string received from the remote, which should
+ be in the form 'VAR=VALUE'. */
+ hex2bin (p, (gdb_byte *) final_var, strlen (p));
+
+ if (remote_debug)
+ {
+ debug_printf (_("[Environment variable to be set: '%s']\n"),
+ final_var);
+ debug_flush ();
+ }
+
+ var_name = final_var;
+ var_value = strchr (final_var, '=');
+ /* There should always be an equal sign, even if the variable is
+ empty. */
+ if (var_value == NULL)
+ {
+ warning (_("Unknown environment variable '%s' from remote side."),
+ final_var);
+ write_enn (own_buf);
+ do_cleanups (c);
+ return;
+ }
+ /* Mark the end of the variable's name. */
+ *var_value = '\0';
+ /* And skip the '=' sign (which now is the '\0'). */
+ ++var_value;
+
+ /* Finally, set the variable to be passed to the inferior. */
+ our_environ.set (var_name, var_value);
+
+ write_ok (own_buf);
+ do_cleanups (c);
+ return;
+ }
+
if (strcmp (own_buf, "QStartNoAckMode") == 0)
{
if (remote_debug)
@@ -2228,7 +2279,8 @@ handle_query (char *own_buf, int packet_len, int
*new_packet_len_p)
}
sprintf (own_buf,
-
"PacketSize=%x;QPassSignals+;QProgramSignals+;QStartupWithShell+",
+ "PacketSize=%x;QPassSignals+;QProgramSignals+;"
+ "QStartupWithShell+;QEnvironmentHexEncoded+",
PBUFSIZ - 1);
if (target_supports_catch_syscall ())
diff --git a/gdb/remote.c b/gdb/remote.c
index 8e8ee6f..8be6fd1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -72,6 +72,7 @@
#include "btrace.h"
#include "record-btrace.h"
#include <algorithm>
+#include "environ.h"
/* Temp hacks for tracepoint encoding migration. */
static char *target_buf;
@@ -1429,6 +1430,7 @@ enum {
PACKET_QCatchSyscalls,
PACKET_QProgramSignals,
PACKET_QStartupWithShell,
+ PACKET_QEnvironmentHexEncoded,
PACKET_qCRC,
PACKET_qSearch_memory,
PACKET_vAttach,
@@ -4636,6 +4638,8 @@ static const struct protocol_feature
remote_protocol_features[] = {
PACKET_QProgramSignals },
{ "QStartupWithShell", PACKET_DISABLE, remote_supported_packet,
PACKET_QStartupWithShell },
+ { "QEnvironmentHexEncoded", PACKET_DISABLE, remote_supported_packet,
+ PACKET_QEnvironmentHexEncoded },
{ "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
PACKET_QStartNoAckMode },
{ "multiprocess", PACKET_DISABLE, remote_supported_packet,
@@ -9585,6 +9589,44 @@ extended_remote_run (const std::string &args)
}
}
+/* Helper function to handle the QEnvironmentHexEncoded packet and
+ send the user-defined environment variables to the remote. */
+
+static void
+extended_remote_environment_support (struct remote_state *rs)
+{
+ gdb_environ *e = ¤t_inferior ()->environment;
+ const char **user_envp = e->user_envp ();
+
+ for (int i = 0; user_envp[i] != NULL; ++i)
+ {
+ char *encoded_fullvar;
+ const char *fullvar = user_envp[i];
+ struct cleanup *c;
+
+ encoded_fullvar = (char *) xmalloc (get_remote_packet_size ());
+ c = make_cleanup (xfree, encoded_fullvar);
+
+ /* Convert the environment variable to an hex string, which
+ is the best format to be transmitted over the wire. */
+ bin2hex ((gdb_byte *) fullvar, encoded_fullvar, strlen
(fullvar));
A bin2hex version that returns an std::string would be nice :)
+
+ xsnprintf (rs->buf, get_remote_packet_size (),
+ "QEnvironmentHexEncoded:%s", encoded_fullvar);
+
+ if (remote_debug)
+ fprintf_unfiltered (gdb_stdlog, _("sending packet '%s'\n"),
+ rs->buf);
Is this useful? When "set debug remote" is active, we already have the
packets printed out, isn't it the same thing?
+
+ putpkt (rs->buf);
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (strcmp (rs->buf, "OK") != 0)
+ warning (_("Unable to set environment variable '%s' on remote."),
+ fullvar);
+ do_cleanups (c);
+ }
+}
+
/* In the extended protocol we want to be able to do things like
"run" and have them basically work as expected. So we need
a special create_inferior function. We support changing the
@@ -9625,6 +9667,9 @@ Remote replied unexpectedly while setting
startup-with-shell: %s"),
rs->buf);
}
+ if (packet_support (PACKET_QEnvironmentHexEncoded) !=
PACKET_DISABLE)
+ extended_remote_environment_support (rs);
It doesn't make a big difference, but I would prefer the check for
packet support in extended_remove_environment_support(), closer to the
code its related to. If we have to re-use that function for some
reason, the check will be already there so we can't forget it.
I have to go now, I haven't had time to look at the test yet.
One thing that worries me about the protocol is that there doesn't seem
to be a way to unset an environment variable (empty value is different
than unset right?). It doesn't seem possible to start an inferior with
just A=1 and another with just B=2. Once you set A=1, it's there for
good.
So do we need another packet for GDB to tell GDBserver "forget about all
variables set with QEnvironmentHexEncoded previously", like
QEnvironmentReset? GDB would send that just before setting the
variables again with QEnvironmentHexEncoded.
Simon