Implement the ability to transmit environment variables to GDBserver when starting the inferior

  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
  

On Thursday, June 29 2017, I 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.

Ping.

> 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;
> +
> +  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]);
> +}
> 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 = &current_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));
> +
> +      xsnprintf (rs->buf, get_remote_packet_size (),
> +		 "QEnvironmentHexEncoded:%s", encoded_fullvar);
> +
> +      if (remote_debug)
> +	fprintf_unfiltered (gdb_stdlog, _("sending packet '%s'\n"),
> +			    rs->buf);
> +
> +      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);
> +
>    /* Now restart the remote server.  */
>    run_worked = extended_remote_run (args) != -1;
>    if (!run_worked)
> @@ -14118,6 +14163,11 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartupWithShell],
>  			 "QStartupWithShell", "startup-with-shell", 0);
>  
> +  add_packet_config_cmd (&remote_protocol_packets
> +			 [PACKET_QEnvironmentHexEncoded],
> +			 "QEnvironmentHexEncoded", "environment-hex-encoded",
> +			 0);
> +
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_qSymbol],
>  			 "qSymbol", "symbol-lookup", 0);
>  
> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.c b/gdb/testsuite/gdb.base/share-env-with-gdbserver.c
> new file mode 100644
> index 0000000..740bd0f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.c
> @@ -0,0 +1,40 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +/* Wrapper around getenv for GDB.  */
> +
> +static const char *
> +my_getenv (const char *name)
> +{
> +  return getenv (name);
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  const char *myvar = getenv ("GDB_TEST_VAR");
> +
> +  if (myvar != NULL)
> +    printf ("It worked!  myvar = '%s'\n", myvar);
> +  else
> +    printf ("It failed.");
> +
> +  return 0;	/* break-here */
> +}
> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> new file mode 100644
> index 0000000..5cae141
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> @@ -0,0 +1,105 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This test doesn't make sense on native-gdbserver.
> +if { [use_gdb_stub] } {
> +    untested "not supported"
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +set test_var_name "GDB_TEST_VAR"
> +
> +# Helper function that does the actual testing.
> +#
> +# - VAR_VALUE is the value of the environment variable.
> +#
> +# - VAR_NAME is the name of the environment variable.  If empty,
> +#   defaults to $test_var_name.
> +#
> +# - VAR_NAME_MATCH is the name (regex) that will be used to query the
> +#   environment about the variable (via getenv).  This is useful when
> +#   we're testing variables with strange names (e.g., with an equal
> +#   sign in the name) and we know that the variable will actually be
> +#   set using another name.  If empty, defatults, to $var_name.
> +#
> +# - VAR_VALUE_MATCH is the value (regex) that will be used to match
> +#   the result of getenv.  The rationale is the same as explained for
> +#   VAR_NAME_MATCH.  If empty, defaults, to $var_value.
> +
> +proc do_test { var_value { var_name "" } { var_name_match "" } { var_value_match "" } } {
> +    global hex decimal binfile test_var_name
> +
> +    clean_restart $binfile
> +
> +    if { $var_name == "" } {
> +	set var_name $test_var_name
> +    }
> +
> +    if { $var_name_match == "" } {
> +	set var_name_match $var_name
> +    }
> +
> +    if { $var_value_match == "" } {
> +	set var_value_match $var_value
> +    }
> +
> +    if { $var_value != "" } {
> +	gdb_test_no_output "set environment $var_name = $var_value" \
> +	    "set $var_name = $var_value"
> +    } else {
> +	gdb_test "set environment $var_name =" \
> +	    "Setting environment variable \"$var_name\" to null value." \
> +	    "set $var_name to null value"
> +    }
> +
> +    if { ![runto_main] } {
> +	return -1
> +    }
> +
> +    gdb_breakpoint [gdb_get_line_number "break-here"]
> +
> +    gdb_test "continue" "Breakpoint $decimal, main \\\(argc=1, argv=$hex\\\) at.*" \
> +	"continue until breakpoint"
> +
> +    gdb_test "print my_getenv (\"$var_name_match\")" "\\\$$decimal = $hex \"$var_value_match\"" \
> +	"print result of getenv for $var_name"
> +}
> +
> +with_test_prefix "long var value" {
> +    do_test "this is my test variable; testing long vars; {}"
> +}
> +
> +with_test_prefix "empty var" {
> +    do_test ""
> +}
> +
> +with_test_prefix "strange named var" {
> +    # In this test we're doing the following:
> +    # 
> +    #   (gdb) set environment 'asd =' = 123 43; asd b ### [];;;
> +    #
> +    # However, due to how GDB parses this line, the environment
> +    # variable will end up named <'asd> (without the <>), and its
> +    # value will be <' = 123 43; asd b ### [];;;> (without the <>).
> +    do_test "123 43; asd b ### [];;;" "'asd ='" "'asd" "' = 123 43; asd b ### [];;;"
> +}
> diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
> index 28b16f8..b740583 100644
> --- a/gdb/unittests/environ-selftests.c
> +++ b/gdb/unittests/environ-selftests.c
> @@ -38,6 +38,7 @@ run_tests ()
>    /* When the vector is initialized, there should always be one NULL
>       element in it.  */
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
>  
>    /* Make sure that there is no other element.  */
>    SELF_CHECK (env.get ("PWD") == NULL);
> @@ -45,14 +46,20 @@ run_tests ()
>    /* Check if unset followed by a set in an empty vector works.  */
>    env.set ("PWD", "test");
>    SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "PWD=test") == 0);
>    /* The second element must be NULL.  */
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>    env.unset ("PWD");
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
>  
>    /* Initialize the environment vector using the host's environ.  */
>    env = gdb_environ::from_host_environ ();
>  
> +  /* The user-set list must be empty.  */
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
> +
>    /* Our test environment variable should be present at the
>       vector.  */
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
> @@ -75,6 +82,7 @@ run_tests ()
>       variable.  */
>    env.clear ();
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
>    SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
>  
>    /* Reinitialize our environ vector using the host environ.  We
> @@ -97,6 +105,9 @@ run_tests ()
>       vector, but can still find B.  */
>    env.set ("GDB_SELFTEST_ENVIRON_1", "aaa");
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
> +  /* User-set environ var list must contain one element.  */
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "GDB_SELFTEST_ENVIRON_1=aaa") == 0);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>  
>    env.set ("GDB_SELFTEST_ENVIRON_2", "bbb");
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
> @@ -104,6 +115,10 @@ run_tests ()
>    env.unset ("GDB_SELFTEST_ENVIRON_1");
>    SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON_1") == NULL);
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
> +  /* The user-set environ var list must contain only one element
> +     now.  */
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "GDB_SELFTEST_ENVIRON_2=bbb") == 0);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>  
>    env.clear ();
>  
> @@ -111,11 +126,16 @@ run_tests ()
>       valid state (i.e., its only element is NULL).  */
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "A=1") == 0);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>    gdb_environ env2;
>    env2 = std::move (env);
>    SELF_CHECK (env.envp ()[0] == NULL);
>    SELF_CHECK (strcmp (env2.get ("A"), "1") == 0);
>    SELF_CHECK (env2.envp ()[1] == NULL);
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
> +  SELF_CHECK (strcmp (env2.user_envp ()[0], "A=1") == 0);
> +  SELF_CHECK (env2.user_envp ()[1] == NULL);
>    env.set ("B", "2");
>    SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> @@ -125,18 +145,26 @@ run_tests ()
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "A=1") == 0);
>    gdb_environ env3 = std::move (env);
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_envp ()[0] == NULL);
>    SELF_CHECK (strcmp (env3.get ("A"), "1") == 0);
>    SELF_CHECK (env3.envp ()[1] == NULL);
> +  SELF_CHECK (strcmp (env3.user_envp ()[0], "A=1") == 0);
> +  SELF_CHECK (env3.user_envp ()[1] == NULL);
>    env.set ("B", "2");
>    SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "B=2") == 0);
> +  SELF_CHECK (env.user_envp ()[2] == NULL);
>  
>    /* Test self-move.  */
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "A=1") == 0);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>  
>    /* Some compilers warn about moving to self, but that's precisely what we want
>       to test here, so turn this warning off.  */
> @@ -148,6 +176,8 @@ run_tests ()
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>    SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (strcmp (env.user_envp ()[0], "A=1") == 0);
> +  SELF_CHECK (env.user_envp ()[1] == NULL);
>  }
>  } /* namespace gdb_environ */
>  } /* namespace selftests */
> -- 
> 2.9.3
  

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 = &current_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
  

Thanks for the review, Simon.  Comments below.

On Thursday, July 13 2017, Simon Marchi wrote:

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

That's true.  I'll do that, thanks for the tip.

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

I was thinking about that, but was unsure if returning a direct
reference to the private vector was a good idea.  One of my first drafts
even implemented it this way, but then I changed things.  But I guess
you're right; there's no harm in returning a *const* reference to the
vector.  I'll change the code accordingly.

>> 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 = &current_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 :)

Wouldn't it?  ;-)

I'll see what can be done.

>> +
>> +      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?

Maybe change the message to print the variable name and its value
instead, then?  Looking at this now, it doesn't really make sense to
print the full hexified packet.

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

Sure, no problem.

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

Yeah, this was something in my mind since the beginning as well.  My
plan, which I should have made clearer, is to tackle the "variable
setting" first, 

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

Not only that, right?  I think we could have another packet,
QEnvironmentUnset, which could also be used to unset specific variables.
For example, suppose that the user wants to unset an environment
variable that was set previously (i.e., before starting GDB).  After
issuing an "unset env VAR" inside GDB, it makes sense to replicate this
to gdbserver as well.

I'll take a look at adding these other packets to the protocol and
extending the patch.

Comments about the testcase are also welcome, when you have a chance to
look at it.

Thanks,
  

Ping.

On Sunday, August 13 2017, I wrote:

> Changes from v2 (addressing comments by Simon):
>
> - Make m_user_set_env_list and m_user_unset_env_list std::set's,
>   instead of std::vector's.  This simplified the code and had the nice
>   side effect of "automatically" avoid duplicates in the sets (at the
>   expense of a bit more processing).  This involved the adjustment of
>   several parts of the code.
>
> - Removed automatic parameter from unset method; now the code has two
>   unset's which are called depending on the case.
>
> - Implement bin2hex overload, and get rid of str2hex.
>
> - Nits and small mistakes fixed.
>
> - s/rerun_to_main/do_prepare_inferior/g in the testcase, in order to
>   avoid accidentally overwriting the top rerun_to_main proc (and mess
>   up with subsequent tests).
>
>
> This patch implements the ability to set/unset environment variables
> on the remote target, mimicking what GDB already offers to the user.
> There are two features present here: user-set and user-unset
> environment variables.
>
> 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.
>
> User-unset environment variables are variables that are explicitly
> unset by the user, using the 'unset environment' command.
>
> The idea behind this patch is to store user-set and user-unset
> environment variables in two separate sets, both part of gdb_environ.
> Then, when extended_remote_create_inferior is preparing to start the
> inferior, it will iterate over the two sets and set/unset variables
> accordingly.  Three new packets are introduced:
>
> - QEnvironmentHexEncoded, which is used to set environment variables,
>   and contains an hex-encoded string in the format "VAR=VALUE" (VALUE
>   can be empty if the user set a variable with a null value, by doing
>   'set environment VAR=').
>
> - QEnvironmentUnset, which is used to unset environment variables, and
>   contains an hex-encoded string in the format "VAR".
>
> - QEnvironmentReset, which is always the first packet to be
>   transmitted, and is used to reset the environment, i.e., discard any
>   changes made by the user on previous runs.
>
> 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.
>
> The other two packets are new.
>
> 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 setting/unsetting environment variables on the remote target.
> 	(New remote packets): Add entries for QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset.
> 	* common/environ.c (gdb_environ::operator=): Extend method to
> 	handle m_user_set_env_list and m_user_unset_env_list.
> 	(gdb_environ::clear): Likewise.
> 	(match_var_in_string): Change type of first parameter from 'char
> 	*' to 'const char *'.
> 	(gdb_environ::set): Extend method to handle
> 	m_user_set_env_list and m_user_unset_env_list.
> 	(gdb_environ::unset): Likewise.
> 	(gdb_environ::clear_user_set_env): New method.
> 	(gdb_environ::user_set_envp): Likewise.
> 	(gdb_environ::user_unset_envp): Likewise.
> 	* common/environ.h (gdb_environ): Handle m_user_set_env_list and
> 	m_user_unset_env_list on move constructor/assignment.
> 	(unset): Add new default parameter 'update_unset_list = true'.
> 	(clear_user_set_env): New method.
> 	(user_set_envp): Likewise.
> 	(user_unset_envp): Likewise.
> 	(m_user_set_env_list): New std::set.
> 	(m_user_unset_env_list): Likewise.
> 	* common/rsp-low.c (hex2str): New function.
> 	(bin2hex): New overload for bin2hex function.
> 	* common/rsp-low.c (hex2str): New prototype.
> 	(str2hex): New overload prototype.
> 	* remote.c: Include "environ.h". Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset.
> 	(remote_protocol_features): Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packets.
> 	(send_environment_packet): New function.
> 	(extended_remote_environment_support): Likewise.
> 	(extended_remote_create_inferior): Call
> 	extended_remote_environment_support.
> 	(_initialize_remote): Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packet configs.
> 	* unittests/environ-selftests.c (run_tests): Add tests for
> 	m_user_set_env_list and m_user_unset_env_list.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* server.c (handle_general_set): Handle QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packets.
> 	(handle_query): Inform remote that QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset are 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.
> 	(unset environment): Likewise, but for unsetting variables.
> 	(Connecting) <Remote Packet>: Add "environment-hex-encoded",
> 	"QEnvironmentHexEncoded", "environment-unset", "QEnvironmentUnset",
> 	"environment-reset" and "QEnvironmentReset" to the table.
> 	(Remote Protocol) <QEnvironmentHexEncoded, QEnvironmentUnset,
> 	QEnvironmentReset>: 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                                           |  24 ++
>  gdb/common/environ.c                               |  75 ++++--
>  gdb/common/environ.h                               |  24 +-
>  gdb/common/rsp-low.c                               |  39 ++++
>  gdb/common/rsp-low.h                               |   8 +
>  gdb/doc/gdb.texinfo                                | 116 ++++++++++
>  gdb/gdbserver/server.c                             |  65 +++++-
>  gdb/remote.c                                       |  76 ++++++
>  gdb/testsuite/gdb.base/share-env-with-gdbserver.c  |  40 ++++
>  .../gdb.base/share-env-with-gdbserver.exp          | 254 +++++++++++++++++++++
>  gdb/unittests/environ-selftests.c                  |  64 ++++++
>  11 files changed, 769 insertions(+), 16 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 9cd1df1cfe..4349cce6e2 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,18 @@
>  
>  *** Changes since GDB 8.0
>  
> +* On Unix systems, GDB now supports transmitting environment variables
> +  that are to be set or unset to GDBserver.  These variables will
> +  affect the environment to be passed to the remote inferior.
> +
> +  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.
> +
> +  To inform GDB of environment variables that are to be unset before
> +  the remote inferior is started by the GDBserver, use the "unset
> +  environment" command.
> +
>  * On Unix systems, GDBserver now does globbing expansion and variable
>    substitution in inferior command line arguments.
>  
> @@ -14,6 +26,18 @@
>  
>  * New remote packets
>  
> +QEnvironmentHexEncoded
> +  Inform GDBserver of an environment variable that is to be passed to
> +  the inferior when starting it.
> +
> +QEnvironmentUnset
> +  Inform GDBserver of an environment variable that is to be unset
> +  before starting the remote inferior.
> +
> +QEnvironmentReset
> +  Inform GDBserver that the environment should be reset (i.e.,
> +  user-set environment variables should be unset).
> +
>  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 698bda3ac1..7753f6ebca 100644
> --- a/gdb/common/environ.c
> +++ b/gdb/common/environ.c
> @@ -30,8 +30,12 @@ gdb_environ::operator= (gdb_environ &&e)
>      return *this;
>  
>    m_environ_vector = std::move (e.m_environ_vector);
> +  m_user_set_env = std::move (e.m_user_set_env);
> +  m_user_unset_env = std::move (e.m_user_unset_env);
>    e.m_environ_vector.clear ();
>    e.m_environ_vector.push_back (NULL);
> +  e.m_user_set_env.clear ();
> +  e.m_user_unset_env.clear ();
>    return *this;
>  }
>  
> @@ -65,6 +69,8 @@ gdb_environ::clear ()
>    m_environ_vector.clear ();
>    /* Always add the NULL element.  */
>    m_environ_vector.push_back (NULL);
> +  m_user_set_env.clear ();
> +  m_user_unset_env.clear ();
>  }
>  
>  /* Helper function to check if STRING contains an environment variable
> @@ -72,7 +78,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,32 +105,59 @@ 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);
> +  unset (var, false);
>  
>    /* 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);
> +
> +  /* Mark this environment variable as having been set by the user.
> +     This will be useful when we deal with setting environment
> +     variables on the remote target.  */
> +  m_user_set_env.insert (std::string (fullvar));
> +
> +  /* If this environment variable is marked as unset by the user, then
> +     remove it from the list, because now the user wants to set
> +     it.  */
> +  m_user_unset_env.erase (std::string (var));
>  }
>  
>  /* See common/environ.h.  */
>  
>  void
> -gdb_environ::unset (const char *var)
> +gdb_environ::unset (const char *var, bool update_unset_list)
>  {
>    size_t len = strlen (var);
> +  std::vector<char *>::iterator it_env;
>  
>    /* 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)
> +    {
> +      m_user_set_env.erase (std::string (*it_env));
> +      xfree (*it_env);
> +
> +      m_environ_vector.erase (it_env);
> +    }
> +
> +  if (update_unset_list)
> +    m_user_unset_env.insert (std::string (var));
> +}
> +
> +/* See common/environ.h.  */
> +
> +void
> +gdb_environ::unset (const char *var)
> +{
> +  unset (var, true);
>  }
>  
>  /* See common/environ.h.  */
> @@ -134,3 +167,17 @@ gdb_environ::envp () const
>  {
>    return const_cast<char **> (&m_environ_vector[0]);
>  }
> +
> +/* See common/environ.h.  */
> +
> +const std::set<std::string> &
> +gdb_environ::user_set_env () const
> +{
> +  return m_user_set_env;
> +}
> +
> +const std::set<std::string> &
> +gdb_environ::user_unset_env () const
> +{
> +  return m_user_unset_env;
> +}
> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
> index 0bbb191361..5ac27fe761 100644
> --- a/gdb/common/environ.h
> +++ b/gdb/common/environ.h
> @@ -18,6 +18,7 @@
>  #define ENVIRON_H 1
>  
>  #include <vector>
> +#include <set>
>  
>  /* Class that represents the environment variables as seen by the
>     inferior.  */
> @@ -41,12 +42,16 @@ 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_set_env (std::move (e.m_user_set_env)),
> +      m_user_unset_env (std::move (e.m_user_unset_env))
>    {
>      /* Make sure that the moved-from vector is left at a valid
>         state (only one NULL element).  */
>      e.m_environ_vector.clear ();
>      e.m_environ_vector.push_back (NULL);
> +    e.m_user_set_env.clear ();
> +    e.m_user_unset_env.clear ();
>    }
>  
>    /* Move assignment.  */
> @@ -73,9 +78,26 @@ public:
>    /* Return the environment vector represented as a 'char **'.  */
>    char **envp () const;
>  
> +  /* Return the user-set environment vector.  */
> +  const std::set<std::string> &user_set_env () const;
> +
> +  /* Return the user-set environment vector.  */
> +  const std::set<std::string> &user_unset_env () const;
> +
>  private:
> +  /* Unset VAR in environment.  If UPDATE_UNSET_LIST is true, then
> +     also update M_USER_UNSET_ENV to reflect the unsetting of the
> +     environment variable.  */
> +  void unset (const char *var, bool update_unset_list);
> +
>    /* A vector containing the environment variables.  */
>    std::vector<char *> m_environ_vector;
> +
> +  /* The environment variables explicitly set by the user.  */
> +  std::set<std::string> m_user_set_env;
> +
> +  /* The environment variables explicitly unset by the user.  */
> +  std::set<std::string> m_user_unset_env;
>  };
>  
>  #endif /* defined (ENVIRON_H) */
> diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
> index eb85ab5701..9f71699144 100644
> --- a/gdb/common/rsp-low.c
> +++ b/gdb/common/rsp-low.c
> @@ -132,6 +132,29 @@ hex2bin (const char *hex, gdb_byte *bin, int count)
>  
>  /* See rsp-low.h.  */
>  
> +std::string
> +hex2str (const char *hex)
> +{
> +  std::string ret;
> +  size_t len = strlen (hex);
> +
> +  for (size_t i = 0; i < len; ++i)
> +    {
> +      if (hex[0] == '\0' || hex[1] == '\0')
> +	{
> +	  /* Hex string is short, or of uneven length.  Return what we
> +	     have so far.  */
> +	  return ret;
> +	}
> +      ret += fromhex (hex[0]) * 16 + fromhex (hex[1]);
> +      hex += 2;
> +    }
> +
> +  return ret;
> +}
> +
> +/* See rsp-low.h.  */
> +
>  int
>  bin2hex (const gdb_byte *bin, char *hex, int count)
>  {
> @@ -146,6 +169,22 @@ bin2hex (const gdb_byte *bin, char *hex, int count)
>    return i;
>  }
>  
> +/* See rsp-low.h.  */
> +
> +std::string
> +bin2hex (const gdb_byte *bin, int count)
> +{
> +  std::string ret;
> +
> +  for (int i = 0; i < count; ++i)
> +    {
> +      ret += tohex ((*bin >> 4) & 0xf);
> +      ret += tohex (*bin++ & 0xf);
> +    }
> +
> +  return ret;
> +}
> +
>  /* Return whether byte B needs escaping when sent as part of binary data.  */
>  
>  static int
> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
> index b57f58bc75..91cff4dac5 100644
> --- a/gdb/common/rsp-low.h
> +++ b/gdb/common/rsp-low.h
> @@ -52,6 +52,10 @@ extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
>  
>  extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>  
> +/* Like hex2bin, but return a std::string.  */
> +
> +extern std::string hex2str (const char *hex);
> +
>  /* Convert some bytes to a hexadecimal representation.  BIN holds the
>     bytes to convert.  COUNT says how many bytes to convert.  The
>     resulting characters are stored in HEX, followed by a NUL
> @@ -59,6 +63,10 @@ extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>  
>  extern int bin2hex (const gdb_byte *bin, char *hex, int count);
>  
> +/* Overloaded version of bin2hex that return a std::string.  */
> +
> +extern std::string bin2hex (const gdb_byte *bin, int count);
> +
>  /* Convert BUFFER, binary data at least LEN_UNITS addressable memory units
>     long, into escaped binary data in OUT_BUF.  Only copy memory units that fit
>     completely in OUT_BUF.  Set *OUT_LEN_UNITS to the number of units from
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 7528183506..1c871cb34c 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,12 +2392,21 @@ 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
> +@anchor{unset environment}
>  @item unset environment @var{varname}
>  Remove variable @var{varname} from the environment to be passed to your
>  program.  This is different from @samp{set env @var{varname} =};
>  @code{unset environment} removes the variable from the environment,
>  rather than assigning it an empty value.
> +
> +Environment variables that are unset by the user are also unset on
> +@command{gdbserver} when starting the remote inferior.
> +@pxref{QEnvironmentUnset}.
>  @end table
>  
>  @emph{Warning:} On Unix systems, @value{GDBN} runs your program using
> @@ -20816,6 +20826,18 @@ are:
>  @tab @code{QStartupWithShell}
>  @tab @code{set startup-with-shell}
>  
> +@item @code{environment-hex-encoded}
> +@tab @code{QEnvironmentHexEncoded}
> +@tab @code{set environment}
> +
> +@item @code{environment-unset}
> +@tab @code{QEnvironmentUnset}
> +@tab @code{unset environment}
> +
> +@item @code{environment-reset}
> +@tab @code{QEnvironmentReset}
> +@tab @code{Reset the inferior environment (i.e., unset user-set variables)}
> +
>  @item @code{conditional-breakpoints-packet}
>  @tab @code{Z0 and Z1}
>  @tab @code{Support for target-side breakpoint condition evaluation}
> @@ -36490,6 +36512,100 @@ 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 set 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 QEnvironmentUnset:@var{hex-value}
> +@anchor{QEnvironmentUnset}
> +@cindex unset environment variable, remote request
> +@cindex @samp{QEnvironmentUnset} packet
> +On UNIX-like targets, it is possible to unset environment variables
> +before starting the inferior in the remote target.  This packet is
> +used to inform @command{gdbserver} of an environment variable that has
> +been unset by the user on @value{GDBN} (@pxref{unset environment}).
> +
> +The packet is composed by @var{hex-value}, an hex encoded
> +representation of the name of the environment variable to be unset.
> +
> +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{unset environment} command;
> +@pxref{unset environment}.
> +
> +@item QEnvironmentReset
> +@anchor{QEnvironmentReset}
> +@cindex reset environment, remote request
> +@cindex @samp{QEnvironmentReset} packet
> +On UNIX-like targets, this packet is used to reset the state of
> +environment variables in the remote target before starting the
> +inferior.  In this context, reset means unsetting all environment
> +variables that were previously set by the user (i.e., were not
> +initially present in the environment).  It is sent to
> +@command{gdbserver} before the @samp{QEnvironmentHexEncoded}
> +(@pxref{QEnvironmentHexEncoded}) and the @samp{QEnvironmentUnset}
> +(@pxref{QEnvironmentUnset}) packets.
> +
> +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.
> +
>  @item qfThreadInfo
>  @itemx qsThreadInfo
>  @cindex list active threads, remote request
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 38383510e8..fa1116a5ee 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -631,6 +631,67 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (startswith (own_buf, "QEnvironmentReset"))
> +    {
> +      our_environ = gdb_environ::from_host_environ ();
> +
> +      write_ok (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.  */
> +      std::string final_var = hex2str (p);
> +      std::string var_name, var_value;
> +
> +      if (remote_debug)
> +	{
> +	  debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
> +	  debug_printf (_("[Environment variable to be set: '%s']\n"),
> +			final_var.c_str ());
> +	  debug_flush ();
> +	}
> +
> +      size_t pos = final_var.find ('=');
> +      if (pos == std::string::npos)
> +	{
> +	  warning (_("Unexpected format for environment variable: '%s'"),
> +		   final_var.c_str ());
> +	  write_enn (own_buf);
> +	  return;
> +	}
> +
> +      var_name = final_var.substr (0, pos);
> +      var_value = final_var.substr (pos + 1, std::string::npos);
> +
> +      our_environ.set (var_name.c_str (), var_value.c_str ());
> +
> +      write_ok (own_buf);
> +      return;
> +    }
> +
> +  if (startswith (own_buf, "QEnvironmentUnset:"))
> +    {
> +      const char *p = own_buf + sizeof ("QEnvironmentUnset:") - 1;
> +      std::string varname = hex2str (p);
> +
> +      if (remote_debug)
> +	{
> +	  debug_printf (_("[QEnvironmentUnset received '%s']\n"), p);
> +	  debug_printf (_("[Environment variable to be unset: '%s']\n"),
> +			varname.c_str ());
> +	  debug_flush ();
> +	}
> +
> +      our_environ.unset (varname.c_str ());
> +
> +      write_ok (own_buf);
> +      return;
> +    }
> +
>    if (strcmp (own_buf, "QStartNoAckMode") == 0)
>      {
>        if (remote_debug)
> @@ -2228,7 +2289,9 @@ 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+;"
> +	       "QEnvironmentReset+;QEnvironmentUnset+",
>  	       PBUFSIZ - 1);
>  
>        if (target_supports_catch_syscall ())
> diff --git a/gdb/remote.c b/gdb/remote.c
> index ff59a0f81f..8c275fcf4f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -73,6 +73,7 @@
>  #include "record-btrace.h"
>  #include <algorithm>
>  #include "common/scoped_restore.h"
> +#include "environ.h"
>  
>  /* Temp hacks for tracepoint encoding migration.  */
>  static char *target_buf;
> @@ -1430,6 +1431,9 @@ enum {
>    PACKET_QCatchSyscalls,
>    PACKET_QProgramSignals,
>    PACKET_QStartupWithShell,
> +  PACKET_QEnvironmentHexEncoded,
> +  PACKET_QEnvironmentReset,
> +  PACKET_QEnvironmentUnset,
>    PACKET_qCRC,
>    PACKET_qSearch_memory,
>    PACKET_vAttach,
> @@ -4637,6 +4641,12 @@ 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 },
> +  { "QEnvironmentReset", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_QEnvironmentReset },
> +  { "QEnvironmentUnset", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_QEnvironmentUnset },
>    { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QStartNoAckMode },
>    { "multiprocess", PACKET_DISABLE, remote_supported_packet,
> @@ -9556,6 +9566,57 @@ extended_remote_run (const std::string &args)
>      }
>  }
>  
> +/* Helper function to send set/unset environment packets.  ACTION is
> +   either "set" or "unset".  PACKET is either "QEnvironmentHexEncoded"
> +   or "QEnvironmentUnsetVariable".  VALUE is the variable to be
> +   sent.  */
> +
> +static void
> +send_environment_packet (struct remote_state *rs,
> +			 const char *action,
> +			 const char *packet,
> +			 const char *value)
> +{
> +  /* Convert the environment variable to an hex string, which
> +     is the best format to be transmitted over the wire.  */
> +  std::string encoded_value = bin2hex ((const gdb_byte *) value,
> +					 strlen (value));
> +
> +  xsnprintf (rs->buf, get_remote_packet_size (),
> +	     "%s:%s", packet, encoded_value.c_str ());
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, &rs->buf_size, 0);
> +  if (strcmp (rs->buf, "OK") != 0)
> +    warning (_("Unable to %s environment variable '%s' on remote."),
> +	     action, value);
> +}
> +
> +/* Helper function to handle the QEnvironment* packets.  */
> +
> +static void
> +extended_remote_environment_support (struct remote_state *rs)
> +{
> +  if (packet_support (PACKET_QEnvironmentReset) != PACKET_DISABLE)
> +    {
> +      putpkt ("QEnvironmentReset");
> +      getpkt (&rs->buf, &rs->buf_size, 0);
> +      if (strcmp (rs->buf, "OK") != 0)
> +	warning (_("Unable to reset environment on remote."));
> +    }
> +
> +  gdb_environ *e = &current_inferior ()->environment;
> +
> +  if (packet_support (PACKET_QEnvironmentHexEncoded) != PACKET_DISABLE)
> +    for (const std::string &el : e->user_set_env ())
> +      send_environment_packet (rs, "set", "QEnvironmentHexEncoded",
> +			       el.c_str ());
> +
> +  if (packet_support (PACKET_QEnvironmentUnset) != PACKET_DISABLE)
> +    for (const std::string &el : e->user_unset_env ())
> +      send_environment_packet (rs, "unset", "QEnvironmentUnset", el.c_str ());
> +}
> +
>  /* 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
> @@ -9596,6 +9657,8 @@ Remote replied unexpectedly while setting startup-with-shell: %s"),
>  	       rs->buf);
>      }
>  
> +  extended_remote_environment_support (rs);
> +
>    /* Now restart the remote server.  */
>    run_worked = extended_remote_run (args) != -1;
>    if (!run_worked)
> @@ -14067,6 +14130,19 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartupWithShell],
>  			 "QStartupWithShell", "startup-with-shell", 0);
>  
> +  add_packet_config_cmd (&remote_protocol_packets
> +			 [PACKET_QEnvironmentHexEncoded],
> +			 "QEnvironmentHexEncoded", "environment-hex-encoded",
> +			 0);
> +
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_QEnvironmentReset],
> +			 "QEnvironmentReset", "environment-reset",
> +			 0);
> +
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_QEnvironmentUnset],
> +			 "QEnvironmentUnset", "environment-unset",
> +			 0);
> +
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_qSymbol],
>  			 "qSymbol", "symbol-lookup", 0);
>  
> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.c b/gdb/testsuite/gdb.base/share-env-with-gdbserver.c
> new file mode 100644
> index 0000000000..740bd0fb52
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.c
> @@ -0,0 +1,40 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +/* Wrapper around getenv for GDB.  */
> +
> +static const char *
> +my_getenv (const char *name)
> +{
> +  return getenv (name);
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  const char *myvar = getenv ("GDB_TEST_VAR");
> +
> +  if (myvar != NULL)
> +    printf ("It worked!  myvar = '%s'\n", myvar);
> +  else
> +    printf ("It failed.");
> +
> +  return 0;	/* break-here */
> +}
> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> new file mode 100644
> index 0000000000..0aca8f6b5e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> @@ -0,0 +1,254 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This test doesn't make sense on native-gdbserver.
> +if { [use_gdb_stub] } {
> +    untested "not supported"
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +set test_var_name "GDB_TEST_VAR"
> +
> +# Helper function that performs a check on the output of "getenv".
> +#
> +# - VAR_NAME is the name of the variable to be checked.
> +#
> +# - VAR_VALUE is the value expected.
> +#
> +# - TEST_MSG, if not empty, is the test message to be used by the
> +#   "gdb_test".
> +#
> +# - EMPTY_VAR_P, if non-zero, means that the variable is not expected
> +#   to exist.  In this case, VAR_VALUE is not considered.
> +
> +proc check_getenv { var_name var_value { test_msg "" } { empty_var_p 0 } } {
> +    global hex decimal
> +
> +    if { $test_msg == "" } {
> +	set test_msg "print result of getenv for $var_name"
> +    }
> +
> +    if { $empty_var_p } {
> +	set var_value_match "0x0"
> +    } else {
> +	set var_value_match "$hex \"$var_value\""
> +    }
> +
> +    gdb_test "print my_getenv (\"$var_name\")" "\\\$$decimal = $var_value_match" \
> +	$test_msg
> +}
> +
> +# Helper function to re-run to main and breaking at the "break-here"
> +# label.
> +
> +proc do_prepare_inferior { } {
> +    global decimal hex
> +
> +    if { ![runto_main] } {
> +	return -1
> +    }
> +
> +    gdb_breakpoint [gdb_get_line_number "break-here"]
> +
> +    gdb_test "continue" "Breakpoint $decimal, main \\\(argc=1, argv=$hex\\\) at.*" \
> +	"continue until breakpoint"
> +}
> +
> +# Helper function that does the actual testing.
> +#
> +# - VAR_VALUE is the value of the environment variable.
> +#
> +# - VAR_NAME is the name of the environment variable.  If empty,
> +#   defaults to $test_var_name.
> +#
> +# - VAR_NAME_MATCH is the name (regex) that will be used to query the
> +#   environment about the variable (via getenv).  This is useful when
> +#   we're testing variables with strange names (e.g., with an equal
> +#   sign in the name) and we know that the variable will actually be
> +#   set using another name.  If empty, defatults, to $var_name.
> +#
> +# - VAR_VALUE_MATCH is the value (regex) that will be used to match
> +#   the result of getenv.  The rationale is the same as explained for
> +#   VAR_NAME_MATCH.  If empty, defaults, to $var_value.
> +
> +proc do_test { var_value { var_name "" } { var_name_match "" } { var_value_match "" } } {
> +    global hex decimal binfile test_var_name
> +
> +    clean_restart $binfile
> +
> +    if { $var_name == "" } {
> +	set var_name $test_var_name
> +    }
> +
> +    if { $var_name_match == "" } {
> +	set var_name_match $var_name
> +    }
> +
> +    if { $var_value_match == "" } {
> +	set var_value_match $var_value
> +    }
> +
> +    if { $var_value != "" } {
> +	gdb_test_no_output "set environment $var_name = $var_value" \
> +	    "set $var_name = $var_value"
> +    } else {
> +	gdb_test "set environment $var_name =" \
> +	    "Setting environment variable \"$var_name\" to null value." \
> +	    "set $var_name to null value"
> +    }
> +
> +    do_prepare_inferior
> +
> +    check_getenv "$var_name_match" "$var_value_match" \
> +	"print result of getenv for $var_name"
> +}
> +
> +with_test_prefix "long var value" {
> +    do_test "this is my test variable; testing long vars; {}"
> +}
> +
> +with_test_prefix "empty var" {
> +    do_test ""
> +}
> +
> +with_test_prefix "strange named var" {
> +    # In this test we're doing the following:
> +    # 
> +    #   (gdb) set environment 'asd =' = 123 43; asd b ### [];;;
> +    #
> +    # However, due to how GDB parses this line, the environment
> +    # variable will end up named <'asd> (without the <>), and its
> +    # value will be <' = 123 43; asd b ### [];;;> (without the <>).
> +    do_test "123 43; asd b ### [];;;" "'asd ='" "'asd" "' = 123 43; asd b ### [];;;"
> +}
> +
> +# Test setting and unsetting environment variables in various
> +# fashions.
> +
> +proc test_set_unset_vars { } {
> +    global hex decimal binfile
> +
> +    clean_restart $binfile
> +
> +    with_test_prefix "set 3 environment variables" {
> +	# Set some environment variables
> +	gdb_test_no_output "set environment A = 1" \
> +	    "set A to 1"
> +	gdb_test_no_output "set environment B = 2" \
> +	    "set B to 2"
> +	gdb_test_no_output "set environment C = 3" \
> +	    "set C to 3"
> +
> +	do_prepare_inferior
> +
> +	# Check that the variables are known by the inferior
> +	check_getenv "A" "1"
> +	check_getenv "B" "2"
> +	check_getenv "C" "3"
> +    }
> +
> +    with_test_prefix "unset one variable, reset one" {
> +	# Now, unset/reset some values
> +	gdb_test_no_output "unset environment A" \
> +	    "unset A"
> +	gdb_test_no_output "set environment B = 4" \
> +	    "set B to 4"
> +
> +	do_prepare_inferior
> +
> +	check_getenv "A" "" "" 1
> +	check_getenv "B" "4"
> +	check_getenv "C" "3"
> +    }
> +
> +    with_test_prefix "unset two variables, reset one" {
> +	# Unset more values
> +	gdb_test_no_output "unset environment B" \
> +	    "unset B"
> +	gdb_test_no_output "set environment A = 1" \
> +	    "set A to 1 again"
> +	gdb_test_no_output "unset environment C" \
> +	    "unset C"
> +
> +	do_prepare_inferior
> +
> +	check_getenv "A" "1"
> +	check_getenv "B" "" "" 1
> +	check_getenv "C" "" "" 1
> +    }
> +}
> +
> +with_test_prefix "test set/unset of vars" {
> +    test_set_unset_vars
> +}
> +
> +# Test that unsetting works.
> +
> +proc test_unset { } {
> +    global hex decimal binfile gdb_prompt
> +
> +    clean_restart $binfile
> +
> +    do_prepare_inferior
> +
> +    set test_msg "check if unset works"
> +    set found_home 0
> +    gdb_test_multiple "print my_getenv (\"HOME\")" $test_msg {
> +	-re "\\\$$decimal = $hex \".*\"\r\n$gdb_prompt $" {
> +	    pass $test_msg
> +	    set found_home 1
> +	}
> +	-re "\\\$$decimal = 0x0\r\n$gdb_prompt $" {
> +	    untested $test_msg
> +	}
> +    }
> +
> +    if { $found_home == 1 } {
> +	with_test_prefix "simple unset" {
> +	    # We can do the test, because $HOME exists (and therefore can
> +	    # be unset).
> +	    gdb_test_no_output "unset environment HOME" "unset HOME"
> +
> +	    do_prepare_inferior
> +
> +	    # $HOME now must be empty
> +	    check_getenv "HOME" "" "" 1
> +	}
> +
> +	with_test_prefix "set-then-unset" {
> +	    clean_restart $binfile
> +
> +	    # Test if setting and then unsetting $HOME works.
> +	    gdb_test_no_output "set environment HOME = test" "set HOME as test"
> +	    gdb_test_no_output "unset environment HOME" "unset HOME again"
> +
> +	    do_prepare_inferior
> +
> +	    check_getenv "HOME" "" "" 1
> +	}
> +    }
> +}
> +
> +with_test_prefix "test unset of vars" {
> +    test_unset
> +}
> diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
> index 28b16f828f..e17715b43c 100644
> --- a/gdb/unittests/environ-selftests.c
> +++ b/gdb/unittests/environ-selftests.c
> @@ -22,6 +22,12 @@
>  #include "common/environ.h"
>  #include "common/diagnostics.h"
>  
> +static bool
> +set_contains (const std::set<std::string> &set, std::string key)
> +{
> +  return set.find (key) != set.end ();
> +}
> +
>  namespace selftests {
>  namespace gdb_environ_tests {
>  
> @@ -38,6 +44,8 @@ run_tests ()
>    /* When the vector is initialized, there should always be one NULL
>       element in it.  */
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 0);
>  
>    /* Make sure that there is no other element.  */
>    SELF_CHECK (env.get ("PWD") == NULL);
> @@ -45,14 +53,24 @@ run_tests ()
>    /* Check if unset followed by a set in an empty vector works.  */
>    env.set ("PWD", "test");
>    SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("PWD=test")));
> +  SELF_CHECK (env.user_unset_env ().size () == 0);
>    /* The second element must be NULL.  */
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>    env.unset ("PWD");
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_unset_env (), std::string ("PWD")));
>  
>    /* Initialize the environment vector using the host's environ.  */
>    env = gdb_environ::from_host_environ ();
>  
> +  /* The user-set and user-unset lists must be empty.  */
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 0);
> +
>    /* Our test environment variable should be present at the
>       vector.  */
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
> @@ -65,6 +83,9 @@ run_tests ()
>       host's environment, but doesn't exist in our vector.  */
>    env.unset ("GDB_SELFTEST_ENVIRON");
>    SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
> +  SELF_CHECK (env.user_unset_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_unset_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON")));
>  
>    /* Re-set the test variable.  */
>    env.set ("GDB_SELFTEST_ENVIRON", "1");
> @@ -75,6 +96,8 @@ run_tests ()
>       variable.  */
>    env.clear ();
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 0);
>    SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
>  
>    /* Reinitialize our environ vector using the host environ.  We
> @@ -89,6 +112,23 @@ run_tests ()
>        ++num_found;
>    SELF_CHECK (num_found == 1);
>  
> +  /* Before unsetting our test variable, test that user-unset works
> +     fine.  */
> +  env.unset ("GDB_SELFTEST_ENVIRON");
> +  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_unset_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON")));
> +  env.set ("GDB_SELFTEST_ENVIRON", "1");
> +  SELF_CHECK (env.user_set_env ().size () == 1);
> +  SELF_CHECK (env.user_unset_env ().size () == 0);
> +  env.unset ("GDB_SELFTEST_ENVIRON");
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_unset_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON")));
> +
>    /* Get rid of our test variable.  */
>    unsetenv ("GDB_SELFTEST_ENVIRON");
>  
> @@ -97,6 +137,10 @@ run_tests ()
>       vector, but can still find B.  */
>    env.set ("GDB_SELFTEST_ENVIRON_1", "aaa");
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
> +  /* User-set environ var list must contain one element.  */
> +  SELF_CHECK (env.user_set_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_set_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON_1=aaa")));
>  
>    env.set ("GDB_SELFTEST_ENVIRON_2", "bbb");
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
> @@ -104,6 +148,11 @@ run_tests ()
>    env.unset ("GDB_SELFTEST_ENVIRON_1");
>    SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON_1") == NULL);
>    SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
> +  /* The user-set environ var list must contain only one element
> +     now.  */
> +  SELF_CHECK (set_contains (env.user_set_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON_2=bbb")));
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>  
>    env.clear ();
>  
> @@ -111,11 +160,16 @@ run_tests ()
>       valid state (i.e., its only element is NULL).  */
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("A=1")));
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>    gdb_environ env2;
>    env2 = std::move (env);
>    SELF_CHECK (env.envp ()[0] == NULL);
>    SELF_CHECK (strcmp (env2.get ("A"), "1") == 0);
>    SELF_CHECK (env2.envp ()[1] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (set_contains (env2.user_set_env (), std::string ("A=1")));
> +  SELF_CHECK (env2.user_set_env ().size () == 1);
>    env.set ("B", "2");
>    SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> @@ -125,18 +179,26 @@ run_tests ()
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("A=1")));
>    gdb_environ env3 = std::move (env);
>    SELF_CHECK (env.envp ()[0] == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
>    SELF_CHECK (strcmp (env3.get ("A"), "1") == 0);
>    SELF_CHECK (env3.envp ()[1] == NULL);
> +  SELF_CHECK (set_contains (env3.user_set_env (), std::string ("A=1")));
> +  SELF_CHECK (env3.user_set_env ().size () == 1);
>    env.set ("B", "2");
>    SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("B=2")));
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>  
>    /* Test self-move.  */
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("A=1")));
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>  
>    /* Some compilers warn about moving to self, but that's precisely what we want
>       to test here, so turn this warning off.  */
> @@ -148,6 +210,8 @@ run_tests ()
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>    SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> +  SELF_CHECK (set_contains (env.user_set_env (), std::string ("A=1")));
> +  SELF_CHECK (env.user_set_env ().size () == 1);
>  }
>  } /* namespace gdb_environ */
>  } /* namespace selftests */
> -- 
> 2.13.3
  

Hi Sergio,

Just some nits about the code, and I also looked at the tests, which I 
hadn't looked at before.

On 2017-08-13 08:19, Sergio Durigan Junior wrote:
> @@ -73,9 +78,26 @@ public:
>    /* Return the environment vector represented as a 'char **'.  */
>    char **envp () const;
> 
> +  /* Return the user-set environment vector.  */
> +  const std::set<std::string> &user_set_env () const;
> +
> +  /* Return the user-set environment vector.  */

user-set -> user-unset ?

> +  const std::set<std::string> &user_unset_env () const;
> +
>  private:
> +  /* Unset VAR in environment.  If UPDATE_UNSET_LIST is true, then
> +     also update M_USER_UNSET_ENV to reflect the unsetting of the
> +     environment variable.  */
> +  void unset (const char *var, bool update_unset_list);
> +
>    /* A vector containing the environment variables.  */
>    std::vector<char *> m_environ_vector;
> +
> +  /* The environment variables explicitly set by the user.  */
> +  std::set<std::string> m_user_set_env;
> +
> +  /* The environment variables explicitly unset by the user.  */
> +  std::set<std::string> m_user_unset_env;
>  };
> 
>  #endif /* defined (ENVIRON_H) */
> diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
> index eb85ab5701..9f71699144 100644
> --- a/gdb/common/rsp-low.c
> +++ b/gdb/common/rsp-low.c
> @@ -132,6 +132,29 @@ hex2bin (const char *hex, gdb_byte *bin, int 
> count)
> 
>  /* See rsp-low.h.  */
> 
> +std::string
> +hex2str (const char *hex)
> +{
> +  std::string ret;
> +  size_t len = strlen (hex);

We could call ret.reserve (len / 2) to avoid reallocations when 
appending to the string.

> +
> +  for (size_t i = 0; i < len; ++i)
> +    {
> +      if (hex[0] == '\0' || hex[1] == '\0')
> +	{
> +	  /* Hex string is short, or of uneven length.  Return what we
> +	     have so far.  */
> +	  return ret;
> +	}
> +      ret += fromhex (hex[0]) * 16 + fromhex (hex[1]);
> +      hex += 2;
> +    }
> +
> +  return ret;
> +}
> +
> +/* See rsp-low.h.  */
> +
>  int
>  bin2hex (const gdb_byte *bin, char *hex, int count)
>  {
> @@ -146,6 +169,22 @@ bin2hex (const gdb_byte *bin, char *hex, int 
> count)
>    return i;
>  }
> 
> +/* See rsp-low.h.  */
> +
> +std::string
> +bin2hex (const gdb_byte *bin, int count)
> +{
> +  std::string ret;

Here too (but with count * 2).

> +
> +  for (int i = 0; i < count; ++i)
> +    {
> +      ret += tohex ((*bin >> 4) & 0xf);
> +      ret += tohex (*bin++ & 0xf);
> +    }
> +
> +  return ret;
> +}
> +
>  /* Return whether byte B needs escaping when sent as part of binary 
> data.  */
> 
>  static int
> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
> index b57f58bc75..91cff4dac5 100644
> --- a/gdb/common/rsp-low.h
> +++ b/gdb/common/rsp-low.h
> @@ -52,6 +52,10 @@ extern char *unpack_varlen_hex (char *buff,
> ULONGEST *result);
> 
>  extern int hex2bin (const char *hex, gdb_byte *bin, int count);
> 
> +/* Like hex2bin, but return a std::string.  */
> +
> +extern std::string hex2str (const char *hex);
> +
>  /* Convert some bytes to a hexadecimal representation.  BIN holds the
>     bytes to convert.  COUNT says how many bytes to convert.  The
>     resulting characters are stored in HEX, followed by a NUL
> @@ -59,6 +63,10 @@ extern int hex2bin (const char *hex, gdb_byte *bin,
> int count);
> 
>  extern int bin2hex (const gdb_byte *bin, char *hex, int count);
> 
> +/* Overloaded version of bin2hex that return a std::string.  */

"returns"

> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 38383510e8..fa1116a5ee 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -631,6 +631,67 @@ handle_general_set (char *own_buf)
>        return;
>      }
> 
> +  if (startswith (own_buf, "QEnvironmentReset"))

I would use strcmp here.  Otherwise, it would also match if we sent a 
packet "QEnvironmentResetFoo" (that could conflict with some packet we 
add in the future).

> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> new file mode 100644
> index 0000000000..0aca8f6b5e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
> @@ -0,0 +1,254 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.
> +
> +# This test doesn't make sense on native-gdbserver.
> +if { [use_gdb_stub] } {
> +    untested "not supported"
> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile 
> debug] } {
> +    return -1
> +}
> +
> +set test_var_name "GDB_TEST_VAR"
> +
> +# Helper function that performs a check on the output of "getenv".
> +#
> +# - VAR_NAME is the name of the variable to be checked.
> +#
> +# - VAR_VALUE is the value expected.
> +#
> +# - TEST_MSG, if not empty, is the test message to be used by the
> +#   "gdb_test".
> +#
> +# - EMPTY_VAR_P, if non-zero, means that the variable is not expected
> +#   to exist.  In this case, VAR_VALUE is not considered.
> +
> +proc check_getenv { var_name var_value { test_msg "" } { empty_var_p 0 
> } } {
> +    global hex decimal
> +
> +    if { $test_msg == "" } {
> +	set test_msg "print result of getenv for $var_name"
> +    }
> +
> +    if { $empty_var_p } {
> +	set var_value_match "0x0"
> +    } else {
> +	set var_value_match "$hex \"$var_value\""
> +    }
> +
> +    gdb_test "print my_getenv (\"$var_name\")" "\\\$$decimal =
> $var_value_match" \
> +	$test_msg
> +}
> +
> +# Helper function to re-run to main and breaking at the "break-here"
> +# label.
> +
> +proc do_prepare_inferior { } {
> +    global decimal hex
> +
> +    if { ![runto_main] } {
> +	return -1
> +    }
> +
> +    gdb_breakpoint [gdb_get_line_number "break-here"]
> +
> +    gdb_test "continue" "Breakpoint $decimal, main \\\(argc=1,
> argv=$hex\\\) at.*" \
> +	"continue until breakpoint"
> +}
> +
> +# Helper function that does the actual testing.
> +#
> +# - VAR_VALUE is the value of the environment variable.
> +#
> +# - VAR_NAME is the name of the environment variable.  If empty,
> +#   defaults to $test_var_name.
> +#
> +# - VAR_NAME_MATCH is the name (regex) that will be used to query the
> +#   environment about the variable (via getenv).  This is useful when
> +#   we're testing variables with strange names (e.g., with an equal
> +#   sign in the name) and we know that the variable will actually be
> +#   set using another name.  If empty, defatults, to $var_name.
> +#
> +# - VAR_VALUE_MATCH is the value (regex) that will be used to match
> +#   the result of getenv.  The rationale is the same as explained for
> +#   VAR_NAME_MATCH.  If empty, defaults, to $var_value.
> +
> +proc do_test { var_value { var_name "" } { var_name_match "" } {
> var_value_match "" } } {
> +    global hex decimal binfile test_var_name

hex and decimal are unused

> +
> +    clean_restart $binfile
> +
> +    if { $var_name == "" } {
> +	set var_name $test_var_name
> +    }
> +
> +    if { $var_name_match == "" } {
> +	set var_name_match $var_name
> +    }
> +
> +    if { $var_value_match == "" } {
> +	set var_value_match $var_value
> +    }
> +
> +    if { $var_value != "" } {
> +	gdb_test_no_output "set environment $var_name = $var_value" \
> +	    "set $var_name = $var_value"
> +    } else {
> +	gdb_test "set environment $var_name =" \
> +	    "Setting environment variable \"$var_name\" to null value." \
> +	    "set $var_name to null value"
> +    }
> +
> +    do_prepare_inferior
> +
> +    check_getenv "$var_name_match" "$var_value_match" \
> +	"print result of getenv for $var_name"
> +}
> +
> +with_test_prefix "long var value" {
> +    do_test "this is my test variable; testing long vars; {}"
> +}
> +
> +with_test_prefix "empty var" {
> +    do_test ""
> +}
> +
> +with_test_prefix "strange named var" {
> +    # In this test we're doing the following:
> +    #

git am complains about a trailing whitespace here.

> +    #   (gdb) set environment 'asd =' = 123 43; asd b ### [];;;
> +    #
> +    # However, due to how GDB parses this line, the environment
> +    # variable will end up named <'asd> (without the <>), and its
> +    # value will be <' = 123 43; asd b ### [];;;> (without the <>).
> +    do_test "123 43; asd b ### [];;;" "'asd ='" "'asd" "' = 123 43;
> asd b ### [];;;"

Watch out, these square brackets are evaluated by tcl and replaced with 
an empty string.

> +}
> +
> +# Test setting and unsetting environment variables in various
> +# fashions.
> +
> +proc test_set_unset_vars { } {
> +    global hex decimal binfile

hex and decimal are unused.

> @@ -89,6 +112,23 @@ run_tests ()
>        ++num_found;
>    SELF_CHECK (num_found == 1);
> 
> +  /* Before unsetting our test variable, test that user-unset works
> +     fine.  */
> +  env.unset ("GDB_SELFTEST_ENVIRON");
> +  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
> +  SELF_CHECK (env.user_set_env ().size () == 0);
> +  SELF_CHECK (env.user_unset_env ().size () == 1);
> +  SELF_CHECK (set_contains (env.user_unset_env (),
> +			    std::string ("GDB_SELFTEST_ENVIRON")));

It seems to me that the same thing has been tested a few lines higher.  
Can we keep just one?  But in general, I find it quite confusing to find 
out what is tested and see if anything it missing.  I think it would be 
better to have several functions (run_tests can call sub-functions), 
where each function tests one particular behavior.  Not only would it 
help readability, but it would also make it easier to add tests without 
fear of breaking the existing tests.

Simon
  

On Thursday, August 24 2017, Simon Marchi wrote:

> Hi Sergio,
>
> Just some nits about the code, and I also looked at the tests, which I
> hadn't looked at before.

Thanks for the review, Simon!

> On 2017-08-13 08:19, Sergio Durigan Junior wrote:
>> @@ -73,9 +78,26 @@ public:
>>    /* Return the environment vector represented as a 'char **'.  */
>>    char **envp () const;
>>
>> +  /* Return the user-set environment vector.  */
>> +  const std::set<std::string> &user_set_env () const;
>> +
>> +  /* Return the user-set environment vector.  */
>
> user-set -> user-unset ?

Fixed.

>> +  const std::set<std::string> &user_unset_env () const;
>> +
>>  private:
>> +  /* Unset VAR in environment.  If UPDATE_UNSET_LIST is true, then
>> +     also update M_USER_UNSET_ENV to reflect the unsetting of the
>> +     environment variable.  */
>> +  void unset (const char *var, bool update_unset_list);
>> +
>>    /* A vector containing the environment variables.  */
>>    std::vector<char *> m_environ_vector;
>> +
>> +  /* The environment variables explicitly set by the user.  */
>> +  std::set<std::string> m_user_set_env;
>> +
>> +  /* The environment variables explicitly unset by the user.  */
>> +  std::set<std::string> m_user_unset_env;
>>  };
>>
>>  #endif /* defined (ENVIRON_H) */
>> diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
>> index eb85ab5701..9f71699144 100644
>> --- a/gdb/common/rsp-low.c
>> +++ b/gdb/common/rsp-low.c
>> @@ -132,6 +132,29 @@ hex2bin (const char *hex, gdb_byte *bin, int
>> count)
>>
>>  /* See rsp-low.h.  */
>>
>> +std::string
>> +hex2str (const char *hex)
>> +{
>> +  std::string ret;
>> +  size_t len = strlen (hex);
>
> We could call ret.reserve (len / 2) to avoid reallocations when
> appending to the string.

Done.

>> +
>> +  for (size_t i = 0; i < len; ++i)
>> +    {
>> +      if (hex[0] == '\0' || hex[1] == '\0')
>> +	{
>> +	  /* Hex string is short, or of uneven length.  Return what we
>> +	     have so far.  */
>> +	  return ret;
>> +	}
>> +      ret += fromhex (hex[0]) * 16 + fromhex (hex[1]);
>> +      hex += 2;
>> +    }
>> +
>> +  return ret;
>> +}
>> +
>> +/* See rsp-low.h.  */
>> +
>>  int
>>  bin2hex (const gdb_byte *bin, char *hex, int count)
>>  {
>> @@ -146,6 +169,22 @@ bin2hex (const gdb_byte *bin, char *hex, int
>> count)
>>    return i;
>>  }
>>
>> +/* See rsp-low.h.  */
>> +
>> +std::string
>> +bin2hex (const gdb_byte *bin, int count)
>> +{
>> +  std::string ret;
>
> Here too (but with count * 2).

Done.

>> +
>> +  for (int i = 0; i < count; ++i)
>> +    {
>> +      ret += tohex ((*bin >> 4) & 0xf);
>> +      ret += tohex (*bin++ & 0xf);
>> +    }
>> +
>> +  return ret;
>> +}
>> +
>>  /* Return whether byte B needs escaping when sent as part of binary
>> data.  */
>>
>>  static int
>> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
>> index b57f58bc75..91cff4dac5 100644
>> --- a/gdb/common/rsp-low.h
>> +++ b/gdb/common/rsp-low.h
>> @@ -52,6 +52,10 @@ extern char *unpack_varlen_hex (char *buff,
>> ULONGEST *result);
>>
>>  extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>>
>> +/* Like hex2bin, but return a std::string.  */
>> +
>> +extern std::string hex2str (const char *hex);
>> +
>>  /* Convert some bytes to a hexadecimal representation.  BIN holds the
>>     bytes to convert.  COUNT says how many bytes to convert.  The
>>     resulting characters are stored in HEX, followed by a NUL
>> @@ -59,6 +63,10 @@ extern int hex2bin (const char *hex, gdb_byte *bin,
>> int count);
>>
>>  extern int bin2hex (const gdb_byte *bin, char *hex, int count);
>>
>> +/* Overloaded version of bin2hex that return a std::string.  */
>
> "returns"

Fixed.

>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index 38383510e8..fa1116a5ee 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -631,6 +631,67 @@ handle_general_set (char *own_buf)
>>        return;
>>      }
>>
>> +  if (startswith (own_buf, "QEnvironmentReset"))
>
> I would use strcmp here.  Otherwise, it would also match if we sent a
> packet "QEnvironmentResetFoo" (that could conflict with some packet we
> add in the future).

Hm, good point.  Fixed.

>> diff --git a/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
>> b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
>> new file mode 100644
>> index 0000000000..0aca8f6b5e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
>> @@ -0,0 +1,254 @@
>> +# This testcase is part of GDB, the GNU debugger.
>> +
>> +# Copyright 2017 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see
>> <http://www.gnu.org/licenses/>.
>> +
>> +# This test doesn't make sense on native-gdbserver.
>> +if { [use_gdb_stub] } {
>> +    untested "not supported"
>> +    return
>> +}
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile
>> debug] } {
>> +    return -1
>> +}
>> +
>> +set test_var_name "GDB_TEST_VAR"
>> +
>> +# Helper function that performs a check on the output of "getenv".
>> +#
>> +# - VAR_NAME is the name of the variable to be checked.
>> +#
>> +# - VAR_VALUE is the value expected.
>> +#
>> +# - TEST_MSG, if not empty, is the test message to be used by the
>> +#   "gdb_test".
>> +#
>> +# - EMPTY_VAR_P, if non-zero, means that the variable is not expected
>> +#   to exist.  In this case, VAR_VALUE is not considered.
>> +
>> +proc check_getenv { var_name var_value { test_msg "" } {
>> empty_var_p 0 } } {
>> +    global hex decimal
>> +
>> +    if { $test_msg == "" } {
>> +	set test_msg "print result of getenv for $var_name"
>> +    }
>> +
>> +    if { $empty_var_p } {
>> +	set var_value_match "0x0"
>> +    } else {
>> +	set var_value_match "$hex \"$var_value\""
>> +    }
>> +
>> +    gdb_test "print my_getenv (\"$var_name\")" "\\\$$decimal =
>> $var_value_match" \
>> +	$test_msg
>> +}
>> +
>> +# Helper function to re-run to main and breaking at the "break-here"
>> +# label.
>> +
>> +proc do_prepare_inferior { } {
>> +    global decimal hex
>> +
>> +    if { ![runto_main] } {
>> +	return -1
>> +    }
>> +
>> +    gdb_breakpoint [gdb_get_line_number "break-here"]
>> +
>> +    gdb_test "continue" "Breakpoint $decimal, main \\\(argc=1,
>> argv=$hex\\\) at.*" \
>> +	"continue until breakpoint"
>> +}
>> +
>> +# Helper function that does the actual testing.
>> +#
>> +# - VAR_VALUE is the value of the environment variable.
>> +#
>> +# - VAR_NAME is the name of the environment variable.  If empty,
>> +#   defaults to $test_var_name.
>> +#
>> +# - VAR_NAME_MATCH is the name (regex) that will be used to query the
>> +#   environment about the variable (via getenv).  This is useful when
>> +#   we're testing variables with strange names (e.g., with an equal
>> +#   sign in the name) and we know that the variable will actually be
>> +#   set using another name.  If empty, defatults, to $var_name.
>> +#
>> +# - VAR_VALUE_MATCH is the value (regex) that will be used to match
>> +#   the result of getenv.  The rationale is the same as explained for
>> +#   VAR_NAME_MATCH.  If empty, defaults, to $var_value.
>> +
>> +proc do_test { var_value { var_name "" } { var_name_match "" } {
>> var_value_match "" } } {
>> +    global hex decimal binfile test_var_name
>
> hex and decimal are unused

Removed.

>> +
>> +    clean_restart $binfile
>> +
>> +    if { $var_name == "" } {
>> +	set var_name $test_var_name
>> +    }
>> +
>> +    if { $var_name_match == "" } {
>> +	set var_name_match $var_name
>> +    }
>> +
>> +    if { $var_value_match == "" } {
>> +	set var_value_match $var_value
>> +    }
>> +
>> +    if { $var_value != "" } {
>> +	gdb_test_no_output "set environment $var_name = $var_value" \
>> +	    "set $var_name = $var_value"
>> +    } else {
>> +	gdb_test "set environment $var_name =" \
>> +	    "Setting environment variable \"$var_name\" to null value." \
>> +	    "set $var_name to null value"
>> +    }
>> +
>> +    do_prepare_inferior
>> +
>> +    check_getenv "$var_name_match" "$var_value_match" \
>> +	"print result of getenv for $var_name"
>> +}
>> +
>> +with_test_prefix "long var value" {
>> +    do_test "this is my test variable; testing long vars; {}"
>> +}
>> +
>> +with_test_prefix "empty var" {
>> +    do_test ""
>> +}
>> +
>> +with_test_prefix "strange named var" {
>> +    # In this test we're doing the following:
>> +    #
>
> git am complains about a trailing whitespace here.

Removed.

>> +    #   (gdb) set environment 'asd =' = 123 43; asd b ### [];;;
>> +    #
>> +    # However, due to how GDB parses this line, the environment
>> +    # variable will end up named <'asd> (without the <>), and its
>> +    # value will be <' = 123 43; asd b ### [];;;> (without the <>).
>> +    do_test "123 43; asd b ### [];;;" "'asd ='" "'asd" "' = 123 43;
>> asd b ### [];;;"
>
> Watch out, these square brackets are evaluated by tcl and replaced
> with an empty string.

Hm, OK.  I'll make sure to properly quote them.

>> +}
>> +
>> +# Test setting and unsetting environment variables in various
>> +# fashions.
>> +
>> +proc test_set_unset_vars { } {
>> +    global hex decimal binfile
>
> hex and decimal are unused.

Removed.

>> @@ -89,6 +112,23 @@ run_tests ()
>>        ++num_found;
>>    SELF_CHECK (num_found == 1);
>>
>> +  /* Before unsetting our test variable, test that user-unset works
>> +     fine.  */
>> +  env.unset ("GDB_SELFTEST_ENVIRON");
>> +  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
>> +  SELF_CHECK (env.user_set_env ().size () == 0);
>> +  SELF_CHECK (env.user_unset_env ().size () == 1);
>> +  SELF_CHECK (set_contains (env.user_unset_env (),
>> +			    std::string ("GDB_SELFTEST_ENVIRON")));
>
> It seems to me that the same thing has been tested a few lines higher.
> Can we keep just one?  But in general, I find it quite confusing to
> find out what is tested and see if anything it missing.  I think it
> would be better to have several functions (run_tests can call
> sub-functions), where each function tests one particular behavior.
> Not only would it help readability, but it would also make it easier
> to add tests without fear of breaking the existing tests.

Good point.  I'll make some adjustments to the way things are tested
here.

I'll submit v4 soon.

Thanks,