From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116111 invoked by alias); 28 Aug 2017 21:25:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 115876 invoked by uid 89); 28 Aug 2017 21:24:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=fear X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 28 Aug 2017 21:24:35 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9389D7EA85; Mon, 28 Aug 2017 21:24:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9389D7EA85 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 37CB35D748; Mon, 28 Aug 2017 21:24:32 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves , Eli Zaretskii Subject: Re: [PATCH v3] Implement the ability to set/unset environment variables to GDBserver when starting the inferior References: <20170629194106.23070-1-sergiodj@redhat.com> <20170813061929.27676-1-sergiodj@redhat.com> <73bf6c70a8a12b4291385374867db9ba@polymtl.ca> Date: Mon, 28 Aug 2017 21:25:00 -0000 In-Reply-To: <73bf6c70a8a12b4291385374867db9ba@polymtl.ca> (Simon Marchi's message of "Thu, 24 Aug 2017 21:25:07 +0200") Message-ID: <87a82j9yi8.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00511.txt.bz2 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 &user_set_env () const; >> + >> + /* Return the user-set environment vector. */ > > user-set -> user-unset ? Fixed. >> + const std::set &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 m_environ_vector; >> + >> + /* The environment variables explicitly set by the user. */ >> + std::set m_user_set_env; >> + >> + /* The environment variables explicitly unset by the user. */ >> + std::set 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 >> . >> + >> +# 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/