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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Sergio,

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

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

user-set -> user-unset ?

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

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

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

Here too (but with count * 2).

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

"returns"

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

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

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

hex and decimal are unused

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

git am complains about a trailing whitespace here.

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

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

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

hex and decimal are unused.

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

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

Simon