From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id 6F85D386F82A for ; Tue, 2 Jun 2020 20:18:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6F85D386F82A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x431.google.com with SMTP id x14so4713874wrp.2 for ; Tue, 02 Jun 2020 13:18:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=IIAXNeaAimOBoR7PMv75cUjgegoGLaWFHgDLY2Mo4kE=; b=Ry8K1ZhuMxU4texLTcvYsz/NRH7E51P32NrC2FXSU2yjMRvMviTRq6h8vat6j8lWbP X4ThiOwMDp5aSTaQmvP1/OPDuuNIwCL1wU9E6J1LQCl39Lc1lRFfy7BTzTzzLO+iv+2I EeIgDt6aNjArmxRxbwZruoCKieM84AnGsT4WscQsZ71PYWv82Id4rLZGZmE3Cj/Gtbup 1wZ0LKiVhZpclBWx+Cz5wTZ/2fqeDeVzZ4bWsA8Y23h7s2D/tQ8yDnYwJJF8Lfnmyqqp NT7C/GOMVrFIYGWfH0/pJ8MVMZoJF3Yb0dAhu1AZKjcv0CD0/YmdazUx9UvfHSqhw/BV +Z6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=IIAXNeaAimOBoR7PMv75cUjgegoGLaWFHgDLY2Mo4kE=; b=hsETz1TOZry+nURB0Wj+/wn1pa9opXm2T60Dc9jdc/UQ/XPOm2gg8moTtUR8My1f+b WvKDFIod6rWnMpJYwXW4qR2UsYQN7DO1EcLFFlyeIlfbjvMHKE7N29KQslV9mXIJljjH 0XE2RAxuHOmRKoGWcE6LeD2DZE+saV/Q1o+Ni4EiFzR3zPeahUs46mhFRV7IL8D7XFTh pSpkYjGbjRi2cGS7xOuag30cjduXWoRtJcYmpNpVkYXZansmBhElkGstc390xB8XucyL 5C+CX8mn2K9XnJmcoY5paY508wnotwJQIpbMheZJqwBFVhFevHiJLYAOp/ibP90bm2T8 7PZA== X-Gm-Message-State: AOAM530b31IW/B57PWPxsqQdmJ2c3RPwEwza5ZD50fEJWkdoa1828D54 v28YVh1/0tWASYmBnOo8l0qAVA== X-Google-Smtp-Source: ABdhPJx5xy8mNHBZWVy40a2cT27k72AuIfIB/7V4HJpeW8pXgyvU+xi0kRMwG8t0oji5W5jk18yN7g== X-Received: by 2002:adf:c98a:: with SMTP id f10mr16274251wrh.329.1591129111380; Tue, 02 Jun 2020 13:18:31 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id 5sm205768wrr.5.2020.06.02.13.18.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jun 2020 13:18:30 -0700 (PDT) Date: Tue, 2 Jun 2020 21:18:29 +0100 From: Andrew Burgess To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array Message-ID: <20200602201829.GC3522@embecosm.com> References: <20200519163004.GA9045@delia> <20200602153830.GZ3522@embecosm.com> <20200602155211.GA3522@embecosm.com> <6382e582-c2f3-a999-f604-979958d6a064@suse.de> <20200602170139.GB3522@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200602170139.GB3522@embecosm.com> X-Operating-System: Linux/5.5.17-200.fc31.x86_64 (x86_64) X-Uptime: 21:17:42 up 43 days, 10:52, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Jun 2020 20:18:34 -0000 * Andrew Burgess [2020-06-02 18:01:39 +0100]: > * Tom de Vries [2020-06-02 18:31:39 +0200]: > > > On 02-06-2020 17:52, Andrew Burgess wrote: > > > * Andrew Burgess [2020-06-02 16:38:30 +0100]: > > > > > >> * Tom de Vries [2020-05-19 18:30:06 +0200]: > > >> > > >>> Hi, > > >>> > > >>> A variable name cannot be used both as scalar and array without an > > >>> intermediate unset. Trying to do so will result in tcl errors, for > > >>> example, for: > > >>> ... > > >>> set var "bla" > > >>> set var(1) "bla" > > >>> ... > > >>> we get: > > >>> ... > > >>> can't set "var(1)": variable isn't array > > >>> ... > > >>> and for the reverse statement order we get: > > >>> ... > > >>> can't set "var": variable is array > > >>> ... > > >>> > > >>> So, since a global name in one test-case can leak to another > > >>> test-case, setting a global name in one test-case can result in > > >>> a tcl error in another test-case that reuses the name in a different > > >>> way. > > >>> > > >>> Warn about leaking a global array from a test-case. > > >>> > > >>> Also, add a possibility to skip the warning in a given test-case using > > >>> variable gdb_skip_check_global_vars, and use it in gdb.mi/mi2-var-child.exp > > >>> and gdb.mi/mi-var-cp.exp. > > >>> > > >>> Tested on x86_64-linux. > > >>> > > >>> Any comments? > > >> > > >> If we're going to add code to loop over all globals anyway, then why > > >> not, instead of warning about bad cases, and then wrapping tests in a > > >> namespace, just have this code "fix" the leaked globals by deleting > > >> them? > > >> > > >> My thinking is that any global that exists when we start a test should > > >> continue to exist at the end of the test. Any other global should > > >> just be unset when the test script finishes. > > >> > > >> If there really is some global state that is lazily created by a > > >> particular test script then (a) this seems like a bug anyway, and (b) > > >> this is easy to fix by giving it an earlier creation / initialisation. > > >> > > >> In this way, folk can just write test scripts, dump their junk all > > >> over the global namespace as they like, and we'll just clean up for > > >> them. > > >> > > >> Thoughts? > > > > > > Here's a really quick patch implementing the idea above. It needs > > > cleaning up and commenting, etc, but it passes the testsuite with no > > > regressions, and taking a look at its debug output, I can see it > > > deleting some of the problem global arrays that are causing issues. > > > > > > What do you think of this approach? > > > > > > Thanks, > > > Andrew > > > > > > ---- > > > > > > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > > > index 444cea01c36..c983c9cc172 100644 > > > --- a/gdb/testsuite/lib/gdb.exp > > > +++ b/gdb/testsuite/lib/gdb.exp > > > @@ -5094,7 +5094,11 @@ set banned_procedures { strace } > > > # if the banned variables and procedures are already traced. > > > set banned_traced 0 > > > > > > +array set known_globals {} > > > + > > > proc gdb_init { test_file_name } { > > > + global known_globals > > > + > > > # Reset the timeout value to the default. This way, any testcase > > > # that changes the timeout value without resetting it cannot affect > > > # the timeout used in subsequent testcases. > > > @@ -5196,13 +5200,27 @@ proc gdb_init { test_file_name } { > > > global gdb_instances > > > set gdb_instances 0 > > > > > > - return [default_gdb_init $test_file_name] > > > + set result [default_gdb_init $test_file_name] > > > + > > > + foreach varname [info globals] { > > > + set known_globals($varname) 1 > > > + } > > > + > > > + return $result > > > } > > > > > > proc gdb_finish { } { > > > global gdbserver_reconnect_p > > > global gdb_prompt > > > global cleanfiles > > > + global known_globals > > > + > > > + foreach varname [info globals] { > > > + if {![info exists known_globals($varname)]} { > > > + verbose -log "APB: Deleting '$varname'" > > > + upvar 0 unset $varname > > > + } > > > + } > > > > I'm not against the approach as such (I remember also trying this out > > initially). > > > > I don't think this use of upvar does any unset though. > > *cough* yeah, I knew that. > > Fixed, and _confirmed_ that it's deleting the globals this time, not > just reporting what it would delete. Here's a cleaned up version for consideration. Thanks, Andrew --- commit f21ca01deceab0fbbfc22e960c1ee5178cb4a496 Author: Andrew Burgess Date: Tue Jun 2 21:11:10 2020 +0100 gdb/testsuite: Prevent globals leaking between test scripts Many of the test scripts create variables in the global namespace, these variables will then be present for the following test scripts. In most cases this is harmless, but in some cases this can cause problems. For example, if a variable is created as an array in one script, but then assigned as a scalar in a different script, this will cause a TCL error. The solution proposed in this patch is to have the GDB test harness record a list of all known global variables at the point just before we source the test script. Then, after the test script has run, we again iterate over all global variables. Any variable that was not in the original list is deleted. The assumption here is that no test script should need to create a global variable that will outlive the lifetime of the test script itself. With this patch in place all tests currently seem to pass, so the assumption seems to hold. gdb/testsuite/ChangeLog: * lib/gdb.exp (gdb_known_globals): New global. (gdb_setup_known_globals): New proc. (gdb_cleanup_globals): New proc. diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 444cea01c36..66de9d6be3f 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5094,6 +5094,35 @@ set banned_procedures { strace } # if the banned variables and procedures are already traced. set banned_traced 0 +# Global array that holds the name of all global variables at the time +# a test script is started. After the test script has completed any +# global not in this list is deleted. +array set gdb_known_globals {} + +# Setup the GDB_KNOWN_GLOBALS array with the names of all current +# global variables. +proc gdb_setup_known_globals {} { + global gdb_known_globals + + array set gdb_known_globals {} + foreach varname [info globals] { + set gdb_known_globals($varname) 1 + } +} + +# Cleanup the global namespace. Any global not in the +# GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak" +# globals from one test script to another. +proc gdb_cleanup_globals {} { + global gdb_known_globals + + foreach varname [info globals] { + if {![info exists gdb_known_globals($varname)]} { + uplevel #0 unset $varname + } + } +} + proc gdb_init { test_file_name } { # Reset the timeout value to the default. This way, any testcase # that changes the timeout value without resetting it cannot affect @@ -5196,13 +5225,16 @@ proc gdb_init { test_file_name } { global gdb_instances set gdb_instances 0 - return [default_gdb_init $test_file_name] + set result [default_gdb_init $test_file_name] + gdb_setup_known_globals + return $result } proc gdb_finish { } { global gdbserver_reconnect_p global gdb_prompt global cleanfiles + global known_globals # Exit first, so that the files are no longer in use. gdb_exit @@ -5228,6 +5260,8 @@ proc gdb_finish { } { } set banned_traced 0 } + + gdb_cleanup_globals } global debug_format