From patchwork Mon Jun 19 17:59:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 21111 Received: (qmail 11779 invoked by alias); 19 Jun 2017 17:59:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 11610 invoked by uid 89); 19 Jun 2017 17:59:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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, 19 Jun 2017 17:59:28 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E25780F6B; Mon, 19 Jun 2017 17:59:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4E25780F6B Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4E25780F6B Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 131388208B; Mon, 19 Jun 2017 17:59:32 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: Simon Marchi , GDB Patches Subject: Re: [PATCH v6] C++ify gdb/common/environ.c References: <20170413040455.23996-1-sergiodj@redhat.com> <20170619043531.32394-1-sergiodj@redhat.com> <87k248y3zp.fsf@redhat.com> <8aabc6fabb04f4e3e8b08e6fa1b0eacc@polymtl.ca> <816a5744-b3b4-855c-5f2e-4c9f0d255512@redhat.com> <1cff1a8055c0d770fef7171b8394e86d@polymtl.ca> <7cf7b53f-600a-32f5-c9d0-2f45a8bb2b46@redhat.com> <50ea9e5c05e31e1e459f22901ee86527@polymtl.ca> <9be4fd14-279e-3dde-656d-3ea6b9aab148@redhat.com> Date: Mon, 19 Jun 2017 13:59:31 -0400 In-Reply-To: <9be4fd14-279e-3dde-656d-3ea6b9aab148@redhat.com> (Pedro Alves's message of "Mon, 19 Jun 2017 17:55:48 +0100") Message-ID: <87tw3bx3i4.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes On Monday, June 19 2017, Pedro Alves wrote: > On 06/19/2017 05:26 PM, Simon Marchi wrote: >> On 2017-06-19 17:44, Pedro Alves wrote: >>> If we take the "always push a NULL on construction" approach, and >>> we want moved-from gdb_environs to be valid, then yes. Note how this >>> results in extra heap allocations when e.g., returning a >>> gdb_environ from functions by value, and makes std::vector >>> much less efficient when it decides it needs to reallocate/move >>> elements. Representing the empty state with a cleared internal >>> vector would avoid this. >> >> Given the move case, since the goal is to be efficient, then yeah I >> would agree >> that it would make sense to make a little bit of efforts to avoid >> allocating >> memory for an objects we are almost certainly throwing away. >> >> But still, in order to leave environ objects in a valid state after a >> move and >> to pedantically comply with the STL spec which says that the vector is >> left in >> an unspecified state, shouldn't we do a .clear () on the moved-from >> vector after >> the move? > > See accepted answer at: > > https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty > > So the only case where it'd be needed would be in op=, and iff the > vectors had different allocators, which is not the case here. > So no, it's not necessary. But I'd be fine with calling it. > >> >>> Note BTW, that we need to be careful with self-move leaving the >>> *this object in a valid state. >> >> Should we just do >> >> if (&other == this) >> return *this; > > Might not be necessary if without that the object ends up > valid anyway. But what you wrote is a safe bet. So, what do you guys think about the patch below, which applies on top of the original? Thanks, diff --git a/gdb/common/environ.c b/gdb/common/environ.c index 09860c9..944276c 100644 --- a/gdb/common/environ.c +++ b/gdb/common/environ.c @@ -25,13 +25,18 @@ gdb_environ & gdb_environ::operator= (gdb_environ &&e) { - clear (); + if (&e == this) + return *this; + m_environ_vector = std::move (e.m_environ_vector); + e.m_environ_vector.clear (); + e.m_environ_vector.push_back (NULL); return *this; } /* Create a gdb_environ object using the host's environment variables. */ + gdb_environ gdb_environ::from_host_environ () { extern char **environ; diff --git a/gdb/common/environ.h b/gdb/common/environ.h index b882bc7..054813c 100644 --- a/gdb/common/environ.h +++ b/gdb/common/environ.h @@ -67,9 +67,7 @@ public: char **envp () const; private: - /* A vector containing the environment variables. This is useful - for when we need to obtain a 'char **' with all the existing - variables. */ + /* A vector containing the environment variables. */ std::vector m_environ_vector; }; diff --git a/gdb/infcmd.c b/gdb/infcmd.c index ee0754d..defa7b0 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -2151,12 +2151,11 @@ environment_info (char *var, int from_tty) { char **envp = current_inferior ()->environment.envp (); - if (envp != NULL) - for (int idx = 0; envp[idx] != NULL; ++idx) - { - puts_filtered (envp[idx]); - puts_filtered ("\n"); - } + for (int idx = 0; envp[idx] != NULL; ++idx) + { + puts_filtered (envp[idx]); + puts_filtered ("\n"); + } } } diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c index 0bcc6cc..8670d58 100644 --- a/gdb/unittests/environ-selftests.c +++ b/gdb/unittests/environ-selftests.c @@ -27,36 +27,58 @@ namespace gdb_environ_tests { static void run_tests () { + /* Set a test environment variable. This will be unset at the end + of this function. */ if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0) - error ("Could not set environment variable for testing."); + error (_("Could not set environment variable for testing.")); gdb_environ env; + /* When the vector is initialized, there should always be one NULL + element in it. */ SELF_CHECK (env.envp ()[0] == NULL); + /* Make sure that there is no other element. */ SELF_CHECK (env.get ("PWD") == NULL); + + /* Check if unset followed by a set in an empty vector works. */ env.set ("PWD", "test"); + SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0); + /* The second element must be NULL. */ + SELF_CHECK (env.envp ()[1] == NULL); env.unset ("PWD"); + SELF_CHECK (env.envp ()[0] == NULL); + /* Initialize the environment vector using the host's environ. */ env = gdb_environ::from_host_environ (); + /* Our test environment variable should be present at the + vector. */ SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0); + /* Set our test variable to another value. */ env.set ("GDB_SELFTEST_ENVIRON", "test"); SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "test") == 0); + /* And unset our test variable. The variable still exists in the + host's environment, but doesn't exist in our vector. */ env.unset ("GDB_SELFTEST_ENVIRON"); SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL); + /* Re-set the test variable. */ env.set ("GDB_SELFTEST_ENVIRON", "1"); SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0); + /* When we clear our environ vector, there should be only one + element on it (NULL), and we shouldn't be able to get our test + variable. */ env.clear (); - SELF_CHECK (env.envp ()[0] == NULL); - SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL); + /* Reinitialize our environ vector using the host environ. We + should be able to see one (and only one) instance of the test + variable. */ env = gdb_environ::from_host_environ (); char **envp = env.envp (); int num_found = 0; @@ -64,11 +86,14 @@ run_tests () for (size_t i = 0; envp[i] != NULL; ++i) if (strcmp (envp[i], "GDB_SELFTEST_ENVIRON=1") == 0) ++num_found; - SELF_CHECK (num_found == 1); + /* Get rid of our test variable. */ unsetenv ("GDB_SELFTEST_ENVIRON"); + /* Test the case when we set a variable A, then set a variable B, + then unset A, and make sure that we cannot find A in the environ + vector, but can still find B. */ env.set ("GDB_SELFTEST_ENVIRON_1", "aaa"); SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0); @@ -78,6 +103,21 @@ 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); + + env.clear (); + + /* Test that after a std::move the moved-from object is left at a + valid state (i.e., its only element is NULL). */ + env.set ("A", "1"); + SELF_CHECK (strcmp (env.get ("A"), "1") == 0); + 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); + env.set ("B", "2"); + SELF_CHECK (strcmp (env.get ("B"), "2") == 0); + SELF_CHECK (env.envp ()[1] == NULL); } } /* namespace gdb_environ */ } /* namespace selftests */