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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, August 24 2017, Simon Marchi wrote:

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

Thanks for the review, Simon!

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

Fixed.

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

Done.

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

Done.

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

Fixed.

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

Hm, good point.  Fixed.

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

Removed.

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

Removed.

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

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

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

Removed.

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

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

I'll submit v4 soon.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/