diff mbox

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

Message ID 20170727033531.23066-1-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior July 27, 2017, 3:35 a.m. UTC 
  Changes from v1:

- Implement suggestions by Simon.

- Improve gdb_environ::unset (use std::remove).

- Implement the ability to unset environment variables in the remote
  target.

- Implement hex2str and str2hex.

- Extend unittest, testcase and documentation.


This version of the patch extends the first version/idea, which only
allowed environment variables to be set in the remote target.  Now it
is also possible to unset them.

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 vectors, both part of
gdb_environ.  Then, when extended_remote_create_inferior is preparing
to start the inferior, it will iterate over the two lists 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 (i.e., unset any user-set
  environment variable) the environment.

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): Likewise.
	(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 vector.
	(m_user_unset_env_list): Likewise.
	* common/rsp-low.c (hex2str): New function.
	(str2hex): Likewise.
	* common/rsp-low.c (hex2str): New prototype.
	(str2hex): Likewise.
	* remote.c: Include "environ.h". Add QEnvironmentHexEncoded,
	QEnvironmentUnset and QEnvironmentReset.
	(remote_protocol_features): Add QEnvironmentHexEncoded,
	QEnvironmentUnset and QEnvironmentReset packets.
	(send_environment_packets): 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                               | 122 ++++++++--
 gdb/common/environ.h                               |  25 +-
 gdb/common/rsp-low.c                               |  37 +++
 gdb/common/rsp-low.h                               |   8 +
 gdb/doc/gdb.texinfo                                | 116 ++++++++++
 gdb/gdbserver/server.c                             |  73 +++++-
 gdb/remote.c                                       |  82 +++++++
 gdb/testsuite/gdb.base/share-env-with-gdbserver.c  |  40 ++++
 .../gdb.base/share-env-with-gdbserver.exp          | 254 +++++++++++++++++++++
 gdb/unittests/environ-selftests.c                  |  55 +++++
 11 files changed, 819 insertions(+), 17 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

Comments

Eli Zaretskii July 27, 2017, 5:10 p.m. UTC | #1 
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>,
> 	Simon Marchi <simon.marchi@polymtl.ca>,
> 	Eli Zaretskii <eliz@gnu.org>,
> 	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Wed, 26 Jul 2017 23:35:31 -0400
> 
> Changes from v1:
> 
> - Implement suggestions by Simon.
> 
> - Improve gdb_environ::unset (use std::remove).
> 
> - Implement the ability to unset environment variables in the remote
>   target.
> 
> - Implement hex2str and str2hex.
> 
> - Extend unittest, testcase and documentation.

OK for the documentation parts.

Thanks.
Simon Marchi July 27, 2017, 10:04 p.m. UTC | #2 
Hi Sergio,

On 2017-07-27 05:35, Sergio Durigan Junior wrote:
> Changes from v1:
> 
> - Implement suggestions by Simon.
> 
> - Improve gdb_environ::unset (use std::remove).
> 
> - Implement the ability to unset environment variables in the remote
>   target.
> 
> - Implement hex2str and str2hex.
> 
> - Extend unittest, testcase and documentation.
> 
> This version of the patch extends the first version/idea, which only
> allowed environment variables to be set in the remote target.  Now it
> is also possible to unset them.

Awesome!

> 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 vectors, both part of
> gdb_environ.  Then, when extended_remote_create_inferior is preparing
> to start the inferior, it will iterate over the two lists 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 (i.e., unset any user-set
>   environment variable) the environment.

There is this use case for which the behavior is different between 
native and remote, related to unset

native:

(gdb inf1) file /usr/bin/env
(gdb inf1) unset environment DISPLAY
(gdb inf1) r  # DISPLAY is not there
(gdb inf1) add-inferior -exec /usr/bin/env
(gdb inf1) inferior 2
(gdb inf2) r  # DISPLAY is there

remote:

(gdb inf1) tar ext :1234
(gdb inf1) file /usr/bin/env
(gdb inf1) set remote exec-file /usr/bin/env
(gdb inf1) unset environment DISPLAY
(gdb inf1) r  # DISPLAY is not there
(gdb inf1) add-inferior -exec /usr/bin/env
(gdb inf1) inferior 2
(gdb inf2) set remote exec-file /usr/bin/env
(gdb inf2) r  # DISPLAY is not there

I think that's because in GDB, we make a new pristine copy of the host 
environment for every inferior, which we don't in gdbserver.  The way I 
understand the QEnvironmentReset is that the remote agent (gdbserver) 
should forget any previous modification to the environment made using 
QEnvironmentHexEncoded and QEnvironmentUnset and return the environment 
to its original state, when it was launched.  This should allow 
supporting the use case above.  To implement that properly, we would 
need to keep a copy of gdbserver's initial environment, which we could 
revert to when receiving a QEnvironmentReset.

In any case, I just want to make sure that the packets we introduce are 
not the things that limit us.

> 
> 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): Likewise.
> 	(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 vector.
> 	(m_user_unset_env_list): Likewise.
> 	* common/rsp-low.c (hex2str): New function.
> 	(str2hex): Likewise.
> 	* common/rsp-low.c (hex2str): New prototype.
> 	(str2hex): Likewise.
> 	* remote.c: Include "environ.h". Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset.
> 	(remote_protocol_features): Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packets.
> 	(send_environment_packets): 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                               | 122 ++++++++--
>  gdb/common/environ.h                               |  25 +-
>  gdb/common/rsp-low.c                               |  37 +++
>  gdb/common/rsp-low.h                               |   8 +
>  gdb/doc/gdb.texinfo                                | 116 ++++++++++
>  gdb/gdbserver/server.c                             |  73 +++++-
>  gdb/remote.c                                       |  82 +++++++
>  gdb/testsuite/gdb.base/share-env-with-gdbserver.c  |  40 ++++
>  .../gdb.base/share-env-with-gdbserver.exp          | 254 
> +++++++++++++++++++++
>  gdb/unittests/environ-selftests.c                  |  55 +++++
>  11 files changed, 819 insertions(+), 17 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 9cd1df1..4349cce 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 698bda3..59b081f 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_list = std::move (e.m_user_set_env_list);
> +  m_user_unset_env_list = std::move (e.m_user_unset_env_list);
>    e.m_environ_vector.clear ();
>    e.m_environ_vector.push_back (NULL);
> +  e.m_user_set_env_list.clear ();
> +  e.m_user_unset_env_list.clear ();
>    return *this;
>  }
> 
> @@ -63,6 +67,10 @@ gdb_environ::clear ()
>    for (char *v : m_environ_vector)
>      xfree (v);
>    m_environ_vector.clear ();
> +  m_user_set_env_list.clear ();
> +  for (const char *v : m_user_unset_env_list)
> +    xfree ((void *) v);
> +  m_user_unset_env_list.clear ();
>    /* Always add the NULL element.  */
>    m_environ_vector.push_back (NULL);

I'd keep these last two lines just after the "m_environ_vector.clear 
()", since they're related.

>  }
> @@ -72,7 +80,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 +107,104 @@ 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_list.push_back (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.  */
> +  for (std::vector<const char *>::iterator iter_user_unset
> +	 = m_user_unset_env_list.begin ();
> +       iter_user_unset != m_user_unset_env_list.end ();
> +       ++iter_user_unset)
> +    if (strcmp (var, *iter_user_unset) == 0)
> +      {
> +	void *v = (void *) *iter_user_unset;
> +
> +	m_user_unset_env_list.erase (iter_user_unset);
> +	xfree (v);
> +	break;
> +      }
>  }
> 
>  /* 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)
> +    {
> +      /* No element has been found.  */
> +      return;
> +    }
> +
> +  std::vector<const char *>::iterator it_user_set_env;
> +  char *found_var = *it_env;
> +
> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
> +				 m_user_set_env_list.end (),
> +				 found_var);
> +  if (it_user_set_env != m_user_set_env_list.end ())
> +    {
> +      /* We found (and removed) the element from the user_set_env
> +	 vector.  */
> +      m_user_set_env_list.erase (it_user_set_env, 
> m_user_set_env_list.end ());
> +    }
> +
> +  if (update_unset_list)
> +    {
> +      bool found_in_unset = false;
> +
> +      for (const char *el : m_user_unset_env_list)
> +	if (strcmp (el, var) == 0)
> +	  {
> +	    found_in_unset = true;
> +	    break;
> +	  }
> +
> +      if (!found_in_unset)
> +	m_user_unset_env_list.push_back (xstrdup (var));
> +    }
> +
> +  m_environ_vector.erase (it_env);
> +  xfree (found_var);
> +}
> +
> +/* See common/environ.h.  */
> +
> +void
> +gdb_environ::clear_user_set_env ()
> +{
> +  std::vector<const char *> copy = m_user_set_env_list;
> +
> +  for (const char *var : copy)
> +    {
> +      std::string varname (var);
> +
> +      varname.erase (varname.find ('='), std::string::npos);
> +      unset (varname.c_str (), false);
> +    }
>  }
> 
>  /* See common/environ.h.  */
> @@ -134,3 +214,17 @@ gdb_environ::envp () const
>  {
>    return const_cast<char **> (&m_environ_vector[0]);
>  }
> +
> +/* See common/environ.h.  */
> +
> +const std::vector<const char *> &
> +gdb_environ::user_set_envp () const
> +{
> +  return m_user_set_env_list;
> +}
> +
> +const std::vector<const char *> &
> +gdb_environ::user_unset_envp () const
> +{
> +  return m_user_unset_env_list;
> +}
> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
> index 0bbb191..2038170 100644
> --- a/gdb/common/environ.h
> +++ b/gdb/common/environ.h
> @@ -41,12 +41,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_list (std::move (e.m_user_set_env_list)),
> +      m_user_unset_env_list (std::move (e.m_user_unset_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_environ_vector.push_back (NULL);
> +    e.m_user_set_env_list.clear ();
> +    e.m_user_unset_env_list.clear ();
>    }
> 
>    /* Move assignment.  */
> @@ -68,14 +72,31 @@ public:
>    void set (const char *var, const char *value);
> 
>    /* Unset VAR in environment.  */
> -  void unset (const char *var);
> +  void unset (const char *var, bool update_unset_list = true);

Can you document the new parameter?

If this parameter is only useful internally, you could make it a private 
method, so that it's not exposed to the outside:

private:
   void unset (const char *var, bool update_unset_list);

and have the public version with a single parameter call it:

   void gdb_environ::unset (const char *var)
   {
     unset (var, true);
   }

> +
> +  /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables.  
> */
> +  void clear_user_set_env ();

I wouldn't put the "Iterate through M_USER_UNSET_ENV_LIST" in the 
comment, since that's implementation details.  The function 
documentation should focus on the visible effects or goals of the 
function (i.e. remove the user set/unset variables in that environment).


> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
> index b57f58b..0c22d4f 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 work on std::string.  */
> +
> +extern int hex2str (const std::string &hex, std::string &str);

The variables passed to this hex parameter are char* pointing inside the 
received packet in server.c.  Therefore, to avoid unnecessary copies and 
construction of string, I'd leave hex as a const char *.  The return 
value doesn't seem needed either.  What about this signature?

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);
> 
> +/* Like bin2hex, but work on std::strings.  */
> +
> +extern int str2hex (const std::string &str, std::string &hex);
> +

Similar thing here, what we pass to str is a const char*, so it leads to 
an unnecessary std::string construction.  Also, the interface of the 
function is not well-suited to encode arbitrary binary data, which could 
any byte.  One could use

   str2hex (std::string (p, count), hex);

but again why copy the data in the first place? So what about this:

extern std::string bin2hex (const char *bin, int count);


> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -631,6 +631,75 @@ handle_general_set (char *own_buf)
>        return;
>      }
> 
> +  if (startswith (own_buf, "QEnvironmentReset"))
> +    {
> +      our_environ.clear_user_set_env ();
> +
> +      write_ok (own_buf);
> +      return;
> +    }
> +
> +  if (startswith (own_buf, "QEnvironmentHexEncoded:"))
> +    {
> +      const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:") - 
> 1;

You can also use strlen to avoid having to do -1, but either is fine 
with me.

> +      /* The final form of the environment variable.  FINAL_VAR will
> +	 hold the 'VAR=VALUE' format.  */
> +      std::string final_var;
> +      std::string var_name, var_value;
> +
> +      if (remote_debug)
> +	debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
> +
> +      hex2str (p, final_var);
> +
> +      if (remote_debug)
> +	{
> +	  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 (_("Unknown environment variable '%s' from remote side."),

I don't find this error message very clear, maybe more something like 
"Unexpected format"?


> +/* Helper function to handle the QEnvironment* packets.  */
> +
> +static void
> +extended_remote_environment_support (struct remote_state *rs)
> +{
> +  if (packet_support (PACKET_QEnvironmentReset) != PACKET_DISABLE)
> +    {
> +      static const char qenvreset[] = "QEnvironmentReset";
> +
> +      putpkt (qenvreset);

Could this be directly

   putpkt ("QEnvironmentReset")?


I haven't looked at everything yet and it's getting late here, I'll try 
to continue tomorrow.

Thanks!

Simon
Simon Marchi July 29, 2017, 10:35 p.m. UTC | #3 
Hi Sergio,

I took a closer look at set/unset, I have a few comments.

> @@ -99,32 +107,104 @@ 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_list.push_back (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.  */
> +  for (std::vector<const char *>::iterator iter_user_unset
> +	 = m_user_unset_env_list.begin ();
> +       iter_user_unset != m_user_unset_env_list.end ();
> +       ++iter_user_unset)
> +    if (strcmp (var, *iter_user_unset) == 0)
> +      {
> +	void *v = (void *) *iter_user_unset;
> +
> +	m_user_unset_env_list.erase (iter_user_unset);
> +	xfree (v);
> +	break;
> +      }
>  }
> 
>  /* 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)
> +    {
> +      /* No element has been found.  */
> +      return;
> +    }

Do we want to support the use case to unset an environment variable that 
is defined on the remote system, but not on the local system?  If so, 
the function should probably not return immediately if the variable is 
not found in the env vector, so that the unset request ends up in the 
list of variables to unset.

> +
> +  std::vector<const char *>::iterator it_user_set_env;
> +  char *found_var = *it_env;
> +
> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
> +				 m_user_set_env_list.end (),
> +				 found_var);
> +  if (it_user_set_env != m_user_set_env_list.end ())
> +    {
> +      /* We found (and removed) the element from the user_set_env
> +	 vector.  */
> +      m_user_set_env_list.erase (it_user_set_env, 
> m_user_set_env_list.end ());
> +    }
> +
> +  if (update_unset_list)
> +    {
> +      bool found_in_unset = false;
> +
> +      for (const char *el : m_user_unset_env_list)
> +	if (strcmp (el, var) == 0)
> +	  {
> +	    found_in_unset = true;
> +	    break;
> +	  }
> +
> +      if (!found_in_unset)
> +	m_user_unset_env_list.push_back (xstrdup (var));
> +    }
> +
> +  m_environ_vector.erase (it_env);
> +  xfree (found_var);
> +}

I have the feeling that we can reduce the amount of boilerplate code in 
the set and unset methods by using std::set instead of std::vector.  
Performance-wise this may not be very good, since for any reasonable 
amount of variables, the vector would probably be more efficient.  But 
its interface makes the code clearer and lighter, in my opinion.  I 
suppose we could always make something with a set-like interface and 
behavior implemented on top of a vector.

> +
> +/* See common/environ.h.  */
> +
> +void
> +gdb_environ::clear_user_set_env ()
> +{
> +  std::vector<const char *> copy = m_user_set_env_list;
> +
> +  for (const char *var : copy)
> +    {
> +      std::string varname (var);
> +
> +      varname.erase (varname.find ('='), std::string::npos);
> +      unset (varname.c_str (), false);
> +    }

I am not sure having a method like this is correct.  It is used in 
gdbserver when we want to "reset" the environment, that is forget all 
user-made modifications, both set and unset.  To do it correctly, it 
should include restoring the variables that have been unset or 
overwritten.  But as I said in my other mail, I think the simplest 
solution will be to restore the environment from a backup copy of the 
original env.  If we do that, this method won't be needed.

I have prototyped some of my suggestions to make sure they made sense.  
I thought I would push them somewhere, feel free to use whatever you 
want if that's useful to you:

https://github.com/simark/binutils-gdb/commits/remote-env

Thanks,

Simon
Sergio Durigan Junior Aug. 1, 2017, 2:33 a.m. UTC | #4 
On Thursday, July 27 2017, Simon Marchi wrote:

> Hi Sergio,
>
> On 2017-07-27 05:35, Sergio Durigan Junior wrote:
>> 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 vectors, both part of
>> gdb_environ.  Then, when extended_remote_create_inferior is preparing
>> to start the inferior, it will iterate over the two lists 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 (i.e., unset any user-set
>>   environment variable) the environment.
>
> There is this use case for which the behavior is different between
> native and remote, related to unset
>
> native:
>
> (gdb inf1) file /usr/bin/env
> (gdb inf1) unset environment DISPLAY
> (gdb inf1) r  # DISPLAY is not there
> (gdb inf1) add-inferior -exec /usr/bin/env
> (gdb inf1) inferior 2
> (gdb inf2) r  # DISPLAY is there
>
> remote:
>
> (gdb inf1) tar ext :1234
> (gdb inf1) file /usr/bin/env
> (gdb inf1) set remote exec-file /usr/bin/env
> (gdb inf1) unset environment DISPLAY
> (gdb inf1) r  # DISPLAY is not there
> (gdb inf1) add-inferior -exec /usr/bin/env
> (gdb inf1) inferior 2
> (gdb inf2) set remote exec-file /usr/bin/env
> (gdb inf2) r  # DISPLAY is not there
>
> I think that's because in GDB, we make a new pristine copy of the host
> environment for every inferior, which we don't in gdbserver.

Thanks for the review, Simon.

Yes, you're right, these cases are currently different because of the
way we handle the environment on GDB and gdbserver.  On gdbserver we
have 'our_environ', which is a global declared at server.c and that is
passed to all inferiors being started.

> The way I understand the QEnvironmentReset is that the remote agent
> (gdbserver) should forget any previous modification to the environment
> made using QEnvironmentHexEncoded and QEnvironmentUnset and return the
> environment to its original state, when it was launched.  This should
> allow supporting the use case above.  To implement that properly, we
> would need to keep a copy of gdbserver's initial environment, which we
> could revert to when receiving a QEnvironmentReset.

Yes, and we already do that on gdbserver; see the 'our_environ' global.

> In any case, I just want to make sure that the packets we introduce
> are not the things that limit us.

Sorry, I'm not sure I understood what you have in mind.  Could you
explain in what ways we'd be limited by the new packets?

> [...]
>> diff --git a/gdb/common/environ.c b/gdb/common/environ.c
>> index 698bda3..59b081f 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_list = std::move (e.m_user_set_env_list);
>> +  m_user_unset_env_list = std::move (e.m_user_unset_env_list);
>>    e.m_environ_vector.clear ();
>>    e.m_environ_vector.push_back (NULL);
>> +  e.m_user_set_env_list.clear ();
>> +  e.m_user_unset_env_list.clear ();
>>    return *this;
>>  }
>>
>> @@ -63,6 +67,10 @@ gdb_environ::clear ()
>>    for (char *v : m_environ_vector)
>>      xfree (v);
>>    m_environ_vector.clear ();
>> +  m_user_set_env_list.clear ();
>> +  for (const char *v : m_user_unset_env_list)
>> +    xfree ((void *) v);
>> +  m_user_unset_env_list.clear ();
>>    /* Always add the NULL element.  */
>>    m_environ_vector.push_back (NULL);
>
> I'd keep these last two lines just after the "m_environ_vector.clear
> ()", since they're related.

Hm, good, I was trying to keep them close but I forgot this one.  Fixed.

>
>>  }
>> @@ -72,7 +80,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 +107,104 @@ 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_list.push_back (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.  */
>> +  for (std::vector<const char *>::iterator iter_user_unset
>> +	 = m_user_unset_env_list.begin ();
>> +       iter_user_unset != m_user_unset_env_list.end ();
>> +       ++iter_user_unset)
>> +    if (strcmp (var, *iter_user_unset) == 0)
>> +      {
>> +	void *v = (void *) *iter_user_unset;
>> +
>> +	m_user_unset_env_list.erase (iter_user_unset);
>> +	xfree (v);
>> +	break;
>> +      }
>>  }
>>
>>  /* 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)
>> +    {
>> +      /* No element has been found.  */
>> +      return;
>> +    }
>> +
>> +  std::vector<const char *>::iterator it_user_set_env;
>> +  char *found_var = *it_env;
>> +
>> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
>> +				 m_user_set_env_list.end (),
>> +				 found_var);
>> +  if (it_user_set_env != m_user_set_env_list.end ())
>> +    {
>> +      /* We found (and removed) the element from the user_set_env
>> +	 vector.  */
>> +      m_user_set_env_list.erase (it_user_set_env,
>> m_user_set_env_list.end ());
>> +    }
>> +
>> +  if (update_unset_list)
>> +    {
>> +      bool found_in_unset = false;
>> +
>> +      for (const char *el : m_user_unset_env_list)
>> +	if (strcmp (el, var) == 0)
>> +	  {
>> +	    found_in_unset = true;
>> +	    break;
>> +	  }
>> +
>> +      if (!found_in_unset)
>> +	m_user_unset_env_list.push_back (xstrdup (var));
>> +    }
>> +
>> +  m_environ_vector.erase (it_env);
>> +  xfree (found_var);
>> +}
>> +
>> +/* See common/environ.h.  */
>> +
>> +void
>> +gdb_environ::clear_user_set_env ()
>> +{
>> +  std::vector<const char *> copy = m_user_set_env_list;
>> +
>> +  for (const char *var : copy)
>> +    {
>> +      std::string varname (var);
>> +
>> +      varname.erase (varname.find ('='), std::string::npos);
>> +      unset (varname.c_str (), false);
>> +    }
>>  }
>>
>>  /* See common/environ.h.  */
>> @@ -134,3 +214,17 @@ gdb_environ::envp () const
>>  {
>>    return const_cast<char **> (&m_environ_vector[0]);
>>  }
>> +
>> +/* See common/environ.h.  */
>> +
>> +const std::vector<const char *> &
>> +gdb_environ::user_set_envp () const
>> +{
>> +  return m_user_set_env_list;
>> +}
>> +
>> +const std::vector<const char *> &
>> +gdb_environ::user_unset_envp () const
>> +{
>> +  return m_user_unset_env_list;
>> +}
>> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
>> index 0bbb191..2038170 100644
>> --- a/gdb/common/environ.h
>> +++ b/gdb/common/environ.h
>> @@ -41,12 +41,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_list (std::move (e.m_user_set_env_list)),
>> +      m_user_unset_env_list (std::move (e.m_user_unset_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_environ_vector.push_back (NULL);
>> +    e.m_user_set_env_list.clear ();
>> +    e.m_user_unset_env_list.clear ();
>>    }
>>
>>    /* Move assignment.  */
>> @@ -68,14 +72,31 @@ public:
>>    void set (const char *var, const char *value);
>>
>>    /* Unset VAR in environment.  */
>> -  void unset (const char *var);
>> +  void unset (const char *var, bool update_unset_list = true);
>
> Can you document the new parameter?
>
> If this parameter is only useful internally, you could make it a
> private method, so that it's not exposed to the outside:
>
> private:
>   void unset (const char *var, bool update_unset_list);
>
> and have the public version with a single parameter call it:
>
>   void gdb_environ::unset (const char *var)
>   {
>     unset (var, true);
>   }

Good idea!  I was trying to avoid adding a new method for that, but
overloading unset seems like a cleaner approach.

>
>> +
>> +  /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables.
>> */
>> +  void clear_user_set_env ();
>
> I wouldn't put the "Iterate through M_USER_UNSET_ENV_LIST" in the
> comment, since that's implementation details.  The function
> documentation should focus on the visible effects or goals of the
> function (i.e. remove the user set/unset variables in that
> environment).

Good point.  I've updated the comment to:

  /* Unset all variables that were previously set by the user, i.e.,
     that were added by calling the SET method.  */
  void clear_user_set_env ();

>> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
>> index b57f58b..0c22d4f 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 work on std::string.  */
>> +
>> +extern int hex2str (const std::string &hex, std::string &str);
>
> The variables passed to this hex parameter are char* pointing inside
> the received packet in server.c.  Therefore, to avoid unnecessary
> copies and construction of string, I'd leave hex as a const char *.
> The return value doesn't seem needed either.  What about this
> signature?
>
> extern std::string hex2str (const char *hex);

Fair enough, I like the idea.  Update accordingly.

>
>> +
>>  /* 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);
>>
>> +/* Like bin2hex, but work on std::strings.  */
>> +
>> +extern int str2hex (const std::string &str, std::string &hex);
>> +
>
> Similar thing here, what we pass to str is a const char*, so it leads
> to an unnecessary std::string construction.  Also, the interface of
> the function is not well-suited to encode arbitrary binary data, which
> could any byte.  One could use
>
>   str2hex (std::string (p, count), hex);
>
> but again why copy the data in the first place? So what about this:
>
> extern std::string bin2hex (const char *bin, int count);

Not sure if it was a typo or if you really meant to propose overloading
the bin2hex method and have another version of it that returns a
std::string, but I like the idea.  I don't think we need to treat the
first parameter as a 'const char *'; TBH, treating it like a 'const
gdb_byte *' (just like the original version does) is more clear.  So I
went ahead and implemented it this way.

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

I personally like using sizeof here and avoiding the call to strlen.

>> +      /* The final form of the environment variable.  FINAL_VAR will
>> +	 hold the 'VAR=VALUE' format.  */
>> +      std::string final_var;
>> +      std::string var_name, var_value;
>> +
>> +      if (remote_debug)
>> +	debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
>> +
>> +      hex2str (p, final_var);
>> +
>> +      if (remote_debug)
>> +	{
>> +	  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 (_("Unknown environment variable '%s' from remote side."),
>
> I don't find this error message very clear, maybe more something like
> "Unexpected format"?

Sure thing.

>
>> +/* Helper function to handle the QEnvironment* packets.  */
>> +
>> +static void
>> +extended_remote_environment_support (struct remote_state *rs)
>> +{
>> +  if (packet_support (PACKET_QEnvironmentReset) != PACKET_DISABLE)
>> +    {
>> +      static const char qenvreset[] = "QEnvironmentReset";
>> +
>> +      putpkt (qenvreset);
>
> Could this be directly
>
>   putpkt ("QEnvironmentReset")?

Yep, totally.

> I haven't looked at everything yet and it's getting late here, I'll
> try to continue tomorrow.

Thanks for the message.  I'll now reply to your other review :-).
Sergio Durigan Junior Aug. 1, 2017, 2:42 a.m. UTC | #5 
On Saturday, July 29 2017, Simon Marchi wrote:

> Hi Sergio,
>
> I took a closer look at set/unset, I have a few comments.

Thanks, Simon.

>> @@ -99,32 +107,104 @@ 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_list.push_back (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.  */
>> +  for (std::vector<const char *>::iterator iter_user_unset
>> +	 = m_user_unset_env_list.begin ();
>> +       iter_user_unset != m_user_unset_env_list.end ();
>> +       ++iter_user_unset)
>> +    if (strcmp (var, *iter_user_unset) == 0)
>> +      {
>> +	void *v = (void *) *iter_user_unset;
>> +
>> +	m_user_unset_env_list.erase (iter_user_unset);
>> +	xfree (v);
>> +	break;
>> +      }
>>  }
>>
>>  /* 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)
>> +    {
>> +      /* No element has been found.  */
>> +      return;
>> +    }
>
> Do we want to support the use case to unset an environment variable
> that is defined on the remote system, but not on the local system?  If
> so, the function should probably not return immediately if the
> variable is not found in the env vector, so that the unset request
> ends up in the list of variables to unset.

I guess I hadn't thought about this use case.  It makes sense to me.  I
will make the proper modifications.

>> +
>> +  std::vector<const char *>::iterator it_user_set_env;
>> +  char *found_var = *it_env;
>> +
>> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
>> +				 m_user_set_env_list.end (),
>> +				 found_var);
>> +  if (it_user_set_env != m_user_set_env_list.end ())
>> +    {
>> +      /* We found (and removed) the element from the user_set_env
>> +	 vector.  */
>> +      m_user_set_env_list.erase (it_user_set_env,
>> m_user_set_env_list.end ());
>> +    }
>> +
>> +  if (update_unset_list)
>> +    {
>> +      bool found_in_unset = false;
>> +
>> +      for (const char *el : m_user_unset_env_list)
>> +	if (strcmp (el, var) == 0)
>> +	  {
>> +	    found_in_unset = true;
>> +	    break;
>> +	  }
>> +
>> +      if (!found_in_unset)
>> +	m_user_unset_env_list.push_back (xstrdup (var));
>> +    }
>> +
>> +  m_environ_vector.erase (it_env);
>> +  xfree (found_var);
>> +}
>
> I have the feeling that we can reduce the amount of boilerplate code
> in the set and unset methods by using std::set instead of std::vector.
> Performance-wise this may not be very good, since for any reasonable
> amount of variables, the vector would probably be more efficient.  But
> its interface makes the code clearer and lighter, in my opinion.  I
> suppose we could always make something with a set-like interface and
> behavior implemented on top of a vector.

I thought about using std::set, but given that I was recently called out
for doing "premature pessimization", I chose to stick with std::vector.
I agree that for some cases std::set would make things easier to
implement/understand.

>
>> +
>> +/* See common/environ.h.  */
>> +
>> +void
>> +gdb_environ::clear_user_set_env ()
>> +{
>> +  std::vector<const char *> copy = m_user_set_env_list;
>> +
>> +  for (const char *var : copy)
>> +    {
>> +      std::string varname (var);
>> +
>> +      varname.erase (varname.find ('='), std::string::npos);
>> +      unset (varname.c_str (), false);
>> +    }
>
> I am not sure having a method like this is correct.  It is used in
> gdbserver when we want to "reset" the environment, that is forget all
> user-made modifications, both set and unset.  To do it correctly, it
> should include restoring the variables that have been unset or
> overwritten.  But as I said in my other mail, I think the simplest
> solution will be to restore the environment from a backup copy of the
> original env.  If we do that, this method won't be needed.
>
> I have prototyped some of my suggestions to make sure they made sense.
> I thought I would push them somewhere, feel free to use whatever you
> want if that's useful to you:
>
> https://github.com/simark/binutils-gdb/commits/remote-env

Thanks, that's very useful.  I have code for most of what you requested,
but I'll use some ideas from your branch.

Cheers,
Simon Marchi Aug. 1, 2017, 9:34 a.m. UTC | #6 
Hi Sergio,

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

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

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

   our_environ = gdb_environ::from_host_environ ();

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

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

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

Sounds good.

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

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

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

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

Thanks,

Simon
Simon Marchi Aug. 1, 2017, 9:53 a.m. UTC | #7 
On 2017-08-01 04:42, Sergio Durigan Junior wrote:
>> I have the feeling that we can reduce the amount of boilerplate code
>> in the set and unset methods by using std::set instead of std::vector.
>> Performance-wise this may not be very good, since for any reasonable
>> amount of variables, the vector would probably be more efficient.  But
>> its interface makes the code clearer and lighter, in my opinion.  I
>> suppose we could always make something with a set-like interface and
>> behavior implemented on top of a vector.
> 
> I thought about using std::set, but given that I was recently called 
> out
> for doing "premature pessimization", I chose to stick with std::vector.
> I agree that for some cases std::set would make things easier to
> implement/understand.

Yeah, the thing with std::set that simplifies the code is its interface, 
not its implementation.  And it would indeed not be a good idea 
performance-wise (both CPU cycles and memory) to use std::set for 
something that would typically contain a handful of elements at the 
most.  So that's why I think using something that has (part of) the 
interface of an std::set but implemented on top of an std::vector would 
be good.  I'll try to prototype something soon.

Simon
Sergio Durigan Junior Aug. 4, 2017, 10:56 p.m. UTC | #8 
On Tuesday, August 01 2017, Simon Marchi wrote:

> Hi Sergio,
>
> On 2017-08-01 04:33, Sergio Durigan Junior wrote:
>>> There is this use case for which the behavior is different between
>>> native and remote, related to unset
>>>
>>> native:
>>>
>>> (gdb inf1) file /usr/bin/env
>>> (gdb inf1) unset environment DISPLAY
>>> (gdb inf1) r  # DISPLAY is not there
>>> (gdb inf1) add-inferior -exec /usr/bin/env
>>> (gdb inf1) inferior 2
>>> (gdb inf2) r  # DISPLAY is there
>>>
>>> remote:
>>>
>>> (gdb inf1) tar ext :1234
>>> (gdb inf1) file /usr/bin/env
>>> (gdb inf1) set remote exec-file /usr/bin/env
>>> (gdb inf1) unset environment DISPLAY
>>> (gdb inf1) r  # DISPLAY is not there
>>> (gdb inf1) add-inferior -exec /usr/bin/env
>>> (gdb inf1) inferior 2
>>> (gdb inf2) set remote exec-file /usr/bin/env
>>> (gdb inf2) r  # DISPLAY is not there
>>>
>>> I think that's because in GDB, we make a new pristine copy of the host
>>> environment for every inferior, which we don't in gdbserver.
>>
>> Thanks for the review, Simon.
>>
>> Yes, you're right, these cases are currently different because of the
>> way we handle the environment on GDB and gdbserver.  On gdbserver we
>> have 'our_environ', which is a global declared at server.c and that is
>> passed to all inferiors being started.
>>
>>> The way I understand the QEnvironmentReset is that the remote agent
>>> (gdbserver) should forget any previous modification to the environment
>>> made using QEnvironmentHexEncoded and QEnvironmentUnset and return the
>>> environment to its original state, when it was launched.  This should
>>> allow supporting the use case above.  To implement that properly, we
>>> would need to keep a copy of gdbserver's initial environment, which we
>>> could revert to when receiving a QEnvironmentReset.
>>
>> Yes, and we already do that on gdbserver; see the 'our_environ' global.
>
> Maybe I'm reading the code wrong, but that's not what I understand.
> gdb_environ is never reset to gdbserver's original state.  So if the
> DISPLAY env var is present in the original environment and is unset
> with a QEnvironmentUnset, a QEnvironmentReset won't make it reappear
> with the current implementation.  But we would want it to be back, to
> support the scenario illustrated above, wouldn't we?
>
> I originally talked about keeping a copy of the initial environment,
> but actually when receiving a QEnvironmentReset, I think gdbserver
> should simply do
>
>   our_environ = gdb_environ::from_host_environ ();

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

>>> In any case, I just want to make sure that the packets we introduce
>>> are not the things that limit us.
>>
>> Sorry, I'm not sure I understood what you have in mind.  Could you
>> explain in what ways we'd be limited by the new packets?
>
> Oh, I just wanted to say that if the gdbserver implementation is not
> perfect yet, it's not the end of the world because that can always
> change.  But the behavior of the RSP packets is more difficult to
> change once it becomes a published interface, so we need to be careful
> that their documented behavior covers all the use cases we want to
> support.  But I know you already know that, so I don't know why I said
> it in the first place :).

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

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

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

Thanks,
Sergio Durigan Junior Aug. 4, 2017, 11:03 p.m. UTC | #9 
On Tuesday, August 01 2017, Simon Marchi wrote:

> On 2017-08-01 04:42, Sergio Durigan Junior wrote:
>>> I have the feeling that we can reduce the amount of boilerplate code
>>> in the set and unset methods by using std::set instead of std::vector.
>>> Performance-wise this may not be very good, since for any reasonable
>>> amount of variables, the vector would probably be more efficient.  But
>>> its interface makes the code clearer and lighter, in my opinion.  I
>>> suppose we could always make something with a set-like interface and
>>> behavior implemented on top of a vector.
>>
>> I thought about using std::set, but given that I was recently called
>> out
>> for doing "premature pessimization", I chose to stick with std::vector.
>> I agree that for some cases std::set would make things easier to
>> implement/understand.
>
> Yeah, the thing with std::set that simplifies the code is its
> interface, not its implementation.  And it would indeed not be a good
> idea performance-wise (both CPU cycles and memory) to use std::set for
> something that would typically contain a handful of elements at the
> most.  So that's why I think using something that has (part of) the
> interface of an std::set but implemented on top of an std::vector
> would be good.  I'll try to prototype something soon.

So, I've been thinking about this implementation over the last few days,
but it's still a bit confuse to me.  My C++-foo is not so good as yours,
so maybe you can give me a hand.

From what I understood initially, your intention was to make a new class
that inherited from std::vector but overloaded/implemented methods to
mimic what a std::set would do.  However, after reading a bit, it
doesn't seem like a good idea to inherit from std::vector (or any std
containers).  Which made me realize that maybe you are talking about
creating a class that encapsulates a std::vector, without inheriting
from it, and that provided the regular .push_back, .insert, .empty,
etc., methods, but in an enhanced way in order to e.g. prevent the
insert of duplicated elements, which is one of the things we miss from
std::set.

Am I in the right direction here?  Also, I started to think...  I don't
envision the user setting thousands of user-set environment variables,
so *maybe* using std::set would be OK-ish for our use case scenario.  I
don't know.  While I understand the concern about premature
pessimization, I'm also not a fan of complicating the implementation
just for a little bit of optimization either.

WDYT?
Simon Marchi Aug. 5, 2017, 6:14 p.m. UTC | #10 
On 2017-08-05 01:03, Sergio Durigan Junior wrote:
> So, I've been thinking about this implementation over the last few 
> days,
> but it's still a bit confuse to me.  My C++-foo is not so good as 
> yours,
> so maybe you can give me a hand.
> 
> From what I understood initially, your intention was to make a new 
> class
> that inherited from std::vector but overloaded/implemented methods to
> mimic what a std::set would do.  However, after reading a bit, it
> doesn't seem like a good idea to inherit from std::vector (or any std
> containers).  Which made me realize that maybe you are talking about
> creating a class that encapsulates a std::vector, without inheriting
> from it, and that provided the regular .push_back, .insert, .empty,
> etc., methods, but in an enhanced way in order to e.g. prevent the
> insert of duplicated elements, which is one of the things we miss from
> std::set.

Do you mean "... we miss from std::vector"?

I also read about inheriting STL containers.  The danger is that they 
don't have virtual destructors.  So let's say we have gdb::set_vector 
inheriting std::vector and try to delete a set_vector instance through a 
vector pointer, the set_vector destructor won't be called.  If 
set_vector is simply adding some methods to the interface (no data 
members, no user defined destructor), I don't know if this is really a 
problem.

But now that I think of it, if we want to make sure this object 
guarantees the properties of a set (like no duplicate elements), we 
would have to hide most of the vector interface and only expose a 
set-like interface.  Otherwise, it would be possible to call push_back 
and insert a duplicate element.  Given that, I'd lean towards creating a 
class that includes a vector and exposes a set-like interface, rather 
than inheriting.

> 
> Am I in the right direction here?  Also, I started to think...  I don't
> envision the user setting thousands of user-set environment variables,
> so *maybe* using std::set would be OK-ish for our use case scenario.  I
> don't know.  While I understand the concern about premature
> pessimization, I'm also not a fan of complicating the implementation
> just for a little bit of optimization either.
> 
> WDYT?

Actually, if we expected the user to set thousands of environment 
variables and needed it to be fast to set and unset variables, it would 
be good to use std::set because of the O(log(N)) 
lookups/insertions/removals, which matters more when you have a lot of 
elements.  But when you just have a few elements, the constant cost is 
more significant.  A vector-based set would have O(N) complexity for 
these operations (at least for insertions and removals, for lookups it 
depends if it is sorted), which would be bad if we had thousands of 
elements.  But since we expect to have just a few, it would likely be 
faster than std::set's constant cost.

But I agree with you that the important thing now is to get the 
algorithm/functionality right.  The typical use case is to have at the 
maximum a few inferiors, which may have at the maximum a few environment 
variables defined.  The defined environment variables are only used when 
doing a "run", which doesn't happen often.  So in the end, I don't think 
it would make a perceptible difference in memory usage or execution time 
to use an std::set.  If it helps to make the code significantly simpler, 
then I think it's worth it.  And we can always revisit it later by 
making a drop-in replacement for std::set.

Simon
Simon Marchi Aug. 7, 2017, 9:29 a.m. UTC | #11 
On 2017-07-27 05:35, Sergio Durigan Junior wrote:
> +# Helper function to re-run to main and breaking at the "break-here"
> +# label.
> +
> +proc rerun_to_main { } {
> +    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"
> +}

Hi Sergio,

I just noticed something out of pure coincidence.  I noticed some TCL 
backtraces when running the testsuite that I hadn't seen before, such 
as:

ERROR: tcl error sourcing 
/home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp.
ERROR: undefined tag "break-here"
     while executing
"error "undefined tag \"$text\"""
     (procedure "gdb_get_line_number" line 36)
     invoked from within
"gdb_get_line_number "break-here""
     (procedure "rerun_to_main" line 8)
     invoked from within
"rerun_to_main"
     (procedure "stepi_out" line 10)
     invoked from within
"stepi_out "stepi""
     (file 
"/home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp" line 
256)
     invoked from within
"source 
/home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp"
     ("uplevel" body line 1)
     invoked from within
"uplevel #0 source 
/home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp"
     invoked from within
"catch "uplevel #0 source $test_file_name""

It turns out I had your test file (share-env-with-gdbserver.exp) still 
lying around in my repo.  The procedure defined above (rerun_to_main) 
overrides one with the same name defined in lib/gdb.exp, breaking the 
tests executed after, that use this proc.

Simon
Sergio Durigan Junior Aug. 12, 2017, 4:19 a.m. UTC | #12 
On Monday, August 07 2017, Simon Marchi wrote:

> On 2017-07-27 05:35, Sergio Durigan Junior wrote:
>> +# Helper function to re-run to main and breaking at the "break-here"
>> +# label.
>> +
>> +proc rerun_to_main { } {
>> +    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"
>> +}
>
> Hi Sergio,

Hey Simon,

I'm back from vacation, sorry about the delay in replying these
messages.

> I just noticed something out of pure coincidence.  I noticed some TCL
> backtraces when running the testsuite that I hadn't seen before, such
> as:
>
> ERROR: tcl error sourcing
> /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp.
> ERROR: undefined tag "break-here"
>     while executing
> "error "undefined tag \"$text\"""
>     (procedure "gdb_get_line_number" line 36)
>     invoked from within
> "gdb_get_line_number "break-here""
>     (procedure "rerun_to_main" line 8)
>     invoked from within
> "rerun_to_main"
>     (procedure "stepi_out" line 10)
>     invoked from within
> "stepi_out "stepi""
>     (file
> "/home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp"
> line 256)
>     invoked from within
> "source
> /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 source
> /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp"
>     invoked from within
> "catch "uplevel #0 source $test_file_name""
>
> It turns out I had your test file (share-env-with-gdbserver.exp) still
> lying around in my repo.  The procedure defined above (rerun_to_main)
> overrides one with the same name defined in lib/gdb.exp, breaking the
> tests executed after, that use this proc.

Ops.  I'll redefine the proc and use another name.  Thanks for catching
that.

Cheers,
Sergio Durigan Junior Aug. 12, 2017, 4:33 a.m. UTC | #13 
On Saturday, August 05 2017, Simon Marchi wrote:

> On 2017-08-05 01:03, Sergio Durigan Junior wrote:
>> So, I've been thinking about this implementation over the last few
>> days,
>> but it's still a bit confuse to me.  My C++-foo is not so good as
>> yours,
>> so maybe you can give me a hand.
>>
>> From what I understood initially, your intention was to make a new
>> class
>> that inherited from std::vector but overloaded/implemented methods to
>> mimic what a std::set would do.  However, after reading a bit, it
>> doesn't seem like a good idea to inherit from std::vector (or any std
>> containers).  Which made me realize that maybe you are talking about
>> creating a class that encapsulates a std::vector, without inheriting
>> from it, and that provided the regular .push_back, .insert, .empty,
>> etc., methods, but in an enhanced way in order to e.g. prevent the
>> insert of duplicated elements, which is one of the things we miss from
>> std::set.
>
> Do you mean "... we miss from std::vector"?

I meant that we miss a few features that std::set has, and we'd like to
have them implemented alongside with what std::vector offers.

> I also read about inheriting STL containers.  The danger is that they
> don't have virtual destructors.  So let's say we have gdb::set_vector
> inheriting std::vector and try to delete a set_vector instance through
> a vector pointer, the set_vector destructor won't be called.  If
> set_vector is simply adding some methods to the interface (no data
> members, no user defined destructor), I don't know if this is really a
> problem.

Right, that's the main issue I've found, too.  As I was/am not sure this
is really an issue for this specific case, I decided to consult you
first.

> But now that I think of it, if we want to make sure this object
> guarantees the properties of a set (like no duplicate elements), we
> would have to hide most of the vector interface and only expose a
> set-like interface.  Otherwise, it would be possible to call push_back
> and insert a duplicate element.  Given that, I'd lean towards creating
> a class that includes a vector and exposes a set-like interface,
> rather than inheriting.

Hm, OK, it's nice to know that I was leaning towards a solution that
crossed your mind as well.

>>
>> Am I in the right direction here?  Also, I started to think...  I don't
>> envision the user setting thousands of user-set environment variables,
>> so *maybe* using std::set would be OK-ish for our use case scenario.  I
>> don't know.  While I understand the concern about premature
>> pessimization, I'm also not a fan of complicating the implementation
>> just for a little bit of optimization either.
>>
>> WDYT?
>
> Actually, if we expected the user to set thousands of environment
> variables and needed it to be fast to set and unset variables, it
> would be good to use std::set because of the O(log(N))
> lookups/insertions/removals, which matters more when you have a lot of
> elements.  But when you just have a few elements, the constant cost is
> more significant.  A vector-based set would have O(N) complexity for
> these operations (at least for insertions and removals, for lookups it
> depends if it is sorted), which would be bad if we had thousands of
> elements.  But since we expect to have just a few, it would likely be
> faster than std::set's constant cost.

You mean std::vector's constant cost, right?

> But I agree with you that the important thing now is to get the
> algorithm/functionality right.  The typical use case is to have at the
> maximum a few inferiors, which may have at the maximum a few
> environment variables defined.  The defined environment variables are
> only used when doing a "run", which doesn't happen often.  So in the
> end, I don't think it would make a perceptible difference in memory
> usage or execution time to use an std::set.  If it helps to make the
> code significantly simpler, then I think it's worth it.  And we can
> always revisit it later by making a drop-in replacement for std::set.

It indeed helps make the code simpler if I use std::set, which is what
I'm using in the last version of the patch.  I'm glad that you
understood and agreed with my point, so I'll go ahead and submit this
version soon (I'll fix the testcase problem first, and run more tests to
make sure everything is OK).

Thanks,
Simon Marchi Aug. 12, 2017, 8:10 a.m. UTC | #14 
On 2017-08-12 06:33, Sergio Durigan Junior wrote:
>> Actually, if we expected the user to set thousands of environment
>> variables and needed it to be fast to set and unset variables, it
>> would be good to use std::set because of the O(log(N))
>> lookups/insertions/removals, which matters more when you have a lot of
>> elements.  But when you just have a few elements, the constant cost is
>> more significant.  A vector-based set would have O(N) complexity for
>> these operations (at least for insertions and removals, for lookups it
>> depends if it is sorted), which would be bad if we had thousands of
>> elements.  But since we expect to have just a few, it would likely be
>> faster than std::set's constant cost.
> 
> You mean std::vector's constant cost, right?

No, I meant std::set's constant cost, although it wasn't clear.  I meant 
the constant hidden by the big-O notation.  The complexity of removing 
from a set may be O(log(N)), but we could also write it as "C1 * 
log(N)", where C1 is a constant.  For a vector, it would take "C2 * N", 
where C2 is a constant.  If C1 is much larger than C2, then using a set 
only starts being interesting with large Ns.  That does a much better 
job at explaining than I do:

http://lafstern.org/matt/col1.pdf

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 9cd1df1..4349cce 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 698bda3..59b081f 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_list = std::move (e.m_user_set_env_list);
+  m_user_unset_env_list = std::move (e.m_user_unset_env_list);
   e.m_environ_vector.clear ();
   e.m_environ_vector.push_back (NULL);
+  e.m_user_set_env_list.clear ();
+  e.m_user_unset_env_list.clear ();
   return *this;
 }
 
@@ -63,6 +67,10 @@  gdb_environ::clear ()
   for (char *v : m_environ_vector)
     xfree (v);
   m_environ_vector.clear ();
+  m_user_set_env_list.clear ();
+  for (const char *v : m_user_unset_env_list)
+    xfree ((void *) v);
+  m_user_unset_env_list.clear ();
   /* Always add the NULL element.  */
   m_environ_vector.push_back (NULL);
 }
@@ -72,7 +80,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 +107,104 @@  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_list.push_back (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.  */
+  for (std::vector<const char *>::iterator iter_user_unset
+	 = m_user_unset_env_list.begin ();
+       iter_user_unset != m_user_unset_env_list.end ();
+       ++iter_user_unset)
+    if (strcmp (var, *iter_user_unset) == 0)
+      {
+	void *v = (void *) *iter_user_unset;
+
+	m_user_unset_env_list.erase (iter_user_unset);
+	xfree (v);
+	break;
+      }
 }
 
 /* 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)
+    {
+      /* No element has been found.  */
+      return;
+    }
+
+  std::vector<const char *>::iterator it_user_set_env;
+  char *found_var = *it_env;
+
+  it_user_set_env = std::remove (m_user_set_env_list.begin (),
+				 m_user_set_env_list.end (),
+				 found_var);
+  if (it_user_set_env != m_user_set_env_list.end ())
+    {
+      /* We found (and removed) the element from the user_set_env
+	 vector.  */
+      m_user_set_env_list.erase (it_user_set_env, m_user_set_env_list.end ());
+    }
+
+  if (update_unset_list)
+    {
+      bool found_in_unset = false;
+
+      for (const char *el : m_user_unset_env_list)
+	if (strcmp (el, var) == 0)
+	  {
+	    found_in_unset = true;
+	    break;
+	  }
+
+      if (!found_in_unset)
+	m_user_unset_env_list.push_back (xstrdup (var));
+    }
+
+  m_environ_vector.erase (it_env);
+  xfree (found_var);
+}
+
+/* See common/environ.h.  */
+
+void
+gdb_environ::clear_user_set_env ()
+{
+  std::vector<const char *> copy = m_user_set_env_list;
+
+  for (const char *var : copy)
+    {
+      std::string varname (var);
+
+      varname.erase (varname.find ('='), std::string::npos);
+      unset (varname.c_str (), false);
+    }
 }
 
 /* See common/environ.h.  */
@@ -134,3 +214,17 @@  gdb_environ::envp () const
 {
   return const_cast<char **> (&m_environ_vector[0]);
 }
+
+/* See common/environ.h.  */
+
+const std::vector<const char *> &
+gdb_environ::user_set_envp () const
+{
+  return m_user_set_env_list;
+}
+
+const std::vector<const char *> &
+gdb_environ::user_unset_envp () const
+{
+  return m_user_unset_env_list;
+}
diff --git a/gdb/common/environ.h b/gdb/common/environ.h
index 0bbb191..2038170 100644
--- a/gdb/common/environ.h
+++ b/gdb/common/environ.h
@@ -41,12 +41,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_list (std::move (e.m_user_set_env_list)),
+      m_user_unset_env_list (std::move (e.m_user_unset_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_environ_vector.push_back (NULL);
+    e.m_user_set_env_list.clear ();
+    e.m_user_unset_env_list.clear ();
   }
 
   /* Move assignment.  */
@@ -68,14 +72,31 @@  public:
   void set (const char *var, const char *value);
 
   /* Unset VAR in environment.  */
-  void unset (const char *var);
+  void unset (const char *var, bool update_unset_list = true);
+
+  /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables.  */
+  void clear_user_set_env ();
 
   /* Return the environment vector represented as a 'char **'.  */
   char **envp () const;
 
+  /* Return the user-set environment vector.  */
+  const std::vector<const char *> &user_set_envp () const;
+
+  /* Return the user-set environment vector.  */
+  const std::vector<const char *> &user_unset_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_set_env_list;
+
+  /* The vector containing the environment variables unset by the
+     user.  */
+  std::vector<const char *> m_user_unset_env_list;
 };
 
 #endif /* defined (ENVIRON_H) */
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index eb85ab5..522c013 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -133,6 +133,28 @@  hex2bin (const char *hex, gdb_byte *bin, int count)
 /* See rsp-low.h.  */
 
 int
+hex2str (const std::string &hex, std::string &str)
+{
+  const char *hex_str = hex.c_str ();
+  int i;
+
+  for (i = 0; i < hex.size (); ++i)
+    {
+      if (hex_str[0] == '\0' || hex_str[1] == '\0')
+	{
+	  /* Hex string is short, or of uneven length.
+	     Return the count that has been converted so far.  */
+	  return i;
+	}
+      str += fromhex (hex_str[0]) * 16 + fromhex (hex_str[1]);
+      hex_str += 2;
+    }
+  return i;
+}
+
+/* See rsp-low.h.  */
+
+int
 bin2hex (const gdb_byte *bin, char *hex, int count)
 {
   int i;
@@ -146,6 +168,21 @@  bin2hex (const gdb_byte *bin, char *hex, int count)
   return i;
 }
 
+/* See rsp-low.h.  */
+
+int
+str2hex (const std::string &str, std::string &hex)
+{
+  int i;
+
+  for (i = 0; i < str.size (); ++i)
+    {
+      hex += tohex ((str[i] >> 4) & 0xf);
+      hex += tohex (str[i] & 0xf);
+    }
+  return i;
+}
+
 /* 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 b57f58b..0c22d4f 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 work on std::string.  */
+
+extern int hex2str (const std::string &hex, std::string &str);
+
 /* 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);
 
+/* Like bin2hex, but work on std::strings.  */
+
+extern int str2hex (const std::string &str, std::string &hex);
+
 /* 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 17b4c69..e902ebb 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 3838351..12dcc99 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -631,6 +631,75 @@  handle_general_set (char *own_buf)
       return;
     }
 
+  if (startswith (own_buf, "QEnvironmentReset"))
+    {
+      our_environ.clear_user_set_env ();
+
+      write_ok (own_buf);
+      return;
+    }
+
+  if (startswith (own_buf, "QEnvironmentHexEncoded:"))
+    {
+      const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:") - 1;
+      /* The final form of the environment variable.  FINAL_VAR will
+	 hold the 'VAR=VALUE' format.  */
+      std::string final_var;
+      std::string var_name, var_value;
+
+      if (remote_debug)
+	debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
+
+      hex2str (p, final_var);
+
+      if (remote_debug)
+	{
+	  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 (_("Unknown environment variable '%s' from remote side."),
+		   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;
+
+      if (remote_debug)
+	debug_printf (_("[QEnvironmentUnset received '%s']\n"), p);
+
+      hex2str (p, varname);
+
+      if (remote_debug)
+	{
+	  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 +2297,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 d363a36..0eaf51c 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,9 @@  enum {
   PACKET_QCatchSyscalls,
   PACKET_QProgramSignals,
   PACKET_QStartupWithShell,
+  PACKET_QEnvironmentHexEncoded,
+  PACKET_QEnvironmentReset,
+  PACKET_QEnvironmentUnset,
   PACKET_qCRC,
   PACKET_qSearch_memory,
   PACKET_vAttach,
@@ -4636,6 +4640,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,
@@ -9583,6 +9593,63 @@  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".  VEC is the corresponding
+   std::vector (from gdb_environ) that contains the variables to be
+   sent.  */
+
+static void
+send_environment_packets (struct remote_state *rs,
+			  const char *action,
+			  const char *packet,
+			  const std::vector<const char *> &vec)
+{
+  for (const char *fullvar : vec)
+    {
+      std::string encoded_fullvar;
+
+      /* Convert the environment variable to an hex string, which
+	 is the best format to be transmitted over the wire.  */
+      str2hex (fullvar, encoded_fullvar);
+
+      xsnprintf (rs->buf, get_remote_packet_size (),
+		 "%s:%s", packet, encoded_fullvar.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, fullvar);
+    }
+}
+
+/* Helper function to handle the QEnvironment* packets.  */
+
+static void
+extended_remote_environment_support (struct remote_state *rs)
+{
+  if (packet_support (PACKET_QEnvironmentReset) != PACKET_DISABLE)
+    {
+      static const char qenvreset[] = "QEnvironmentReset";
+
+      putpkt (qenvreset);
+      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)
+    send_environment_packets (rs, "set", "QEnvironmentHexEncoded",
+			      e->user_set_envp ());
+
+  if (packet_support (PACKET_QEnvironmentUnset) != PACKET_DISABLE)
+    send_environment_packets (rs, "unset", "QEnvironmentUnset",
+			      e->user_unset_envp ());
+}
+
 /* 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
@@ -9623,6 +9690,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)
@@ -14116,6 +14185,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 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..9c5ec4a
--- /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 rerun_to_main { } {
+    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"
+    }
+
+    rerun_to_main
+
+    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"
+
+	rerun_to_main
+
+	# 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"
+
+	rerun_to_main
+
+	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"
+
+	rerun_to_main
+
+	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
+
+    rerun_to_main
+
+    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"
+
+	    rerun_to_main
+
+	    # $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"
+
+	    rerun_to_main
+
+	    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 28b16f8..7915daf 100644
--- a/gdb/unittests/environ-selftests.c
+++ b/gdb/unittests/environ-selftests.c
@@ -38,6 +38,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_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 0);
 
   /* Make sure that there is no other element.  */
   SELF_CHECK (env.get ("PWD") == NULL);
@@ -45,14 +47,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 (strcmp (env.user_set_envp ()[0], "PWD=test") == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 0);
   /* The second element must be NULL.  */
   SELF_CHECK (env.envp ()[1] == NULL);
+  SELF_CHECK (env.user_set_envp ().size () == 1);
   env.unset ("PWD");
   SELF_CHECK (env.envp ()[0] == NULL);
+  SELF_CHECK (env.user_set_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 1);
+  SELF_CHECK (strcmp (env.user_unset_envp ()[0], "PWD") == 0);
 
   /* 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_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 0);
+
   /* Our test environment variable should be present at the
      vector.  */
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
@@ -65,6 +77,8 @@  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_envp ().size () == 1);
+  SELF_CHECK (strcmp (env.user_unset_envp ()[0], "GDB_SELFTEST_ENVIRON") == 0);
 
   /* Re-set the test variable.  */
   env.set ("GDB_SELFTEST_ENVIRON", "1");
@@ -75,6 +89,8 @@  run_tests ()
      variable.  */
   env.clear ();
   SELF_CHECK (env.envp ()[0] == NULL);
+  SELF_CHECK (env.user_set_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 0);
   SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
 
   /* Reinitialize our environ vector using the host environ.  We
@@ -89,6 +105,21 @@  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_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 1);
+  SELF_CHECK (strcmp (env.user_unset_envp ()[0], "GDB_SELFTEST_ENVIRON") == 0);
+  env.set ("GDB_SELFTEST_ENVIRON", "1");
+  SELF_CHECK (env.user_set_envp ().size () == 1);
+  SELF_CHECK (env.user_unset_envp ().size () == 0);
+  env.unset ("GDB_SELFTEST_ENVIRON");
+  SELF_CHECK (env.user_set_envp ().size () == 0);
+  SELF_CHECK (env.user_unset_envp ().size () == 1);
+  SELF_CHECK (strcmp (env.user_unset_envp ()[0], "GDB_SELFTEST_ENVIRON") == 0);
+
   /* Get rid of our test variable.  */
   unsetenv ("GDB_SELFTEST_ENVIRON");
 
@@ -97,6 +128,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_envp ().size () == 1);
+  SELF_CHECK (strcmp (env.user_set_envp ()[0],
+		      "GDB_SELFTEST_ENVIRON_1=aaa") == 0);
 
   env.set ("GDB_SELFTEST_ENVIRON_2", "bbb");
   SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
@@ -104,6 +139,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 (strcmp (env.user_set_envp ()[0],
+		      "GDB_SELFTEST_ENVIRON_2=bbb") == 0);
+  SELF_CHECK (env.user_set_envp ().size () == 1);
 
   env.clear ();
 
@@ -111,11 +151,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_set_envp ()[0], "A=1") == 0);
+  SELF_CHECK (env.user_set_envp ().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_envp ().size () == 0);
+  SELF_CHECK (strcmp (env2.user_set_envp ()[0], "A=1") == 0);
+  SELF_CHECK (env2.user_set_envp ().size () == 1);
   env.set ("B", "2");
   SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
   SELF_CHECK (env.envp ()[1] == NULL);
@@ -125,18 +170,26 @@  run_tests ()
   env.clear ();
   env.set ("A", "1");
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+  SELF_CHECK (strcmp (env.user_set_envp ()[0], "A=1") == 0);
   gdb_environ env3 = std::move (env);
   SELF_CHECK (env.envp ()[0] == NULL);
+  SELF_CHECK (env.user_set_envp ().size () == 0);
   SELF_CHECK (strcmp (env3.get ("A"), "1") == 0);
   SELF_CHECK (env3.envp ()[1] == NULL);
+  SELF_CHECK (strcmp (env3.user_set_envp ()[0], "A=1") == 0);
+  SELF_CHECK (env3.user_set_envp ().size () == 1);
   env.set ("B", "2");
   SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
   SELF_CHECK (env.envp ()[1] == NULL);
+  SELF_CHECK (strcmp (env.user_set_envp ()[0], "B=2") == 0);
+  SELF_CHECK (env.user_set_envp ().size () == 1);
 
   /* Test self-move.  */
   env.clear ();
   env.set ("A", "1");
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+  SELF_CHECK (strcmp (env.user_set_envp ()[0], "A=1") == 0);
+  SELF_CHECK (env.user_set_envp ().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 +201,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_set_envp ()[0], "A=1") == 0);
+  SELF_CHECK (env.user_set_envp ().size () == 1);
 }
 } /* namespace gdb_environ */
 } /* namespace selftests */