From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by sourceware.org (Postfix) with ESMTPS id 5B66C3851C26 for ; Tue, 2 Jun 2020 17:01:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5B66C3851C26 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-wm1-x344.google.com with SMTP id q25so3902377wmj.0 for ; Tue, 02 Jun 2020 10:01:42 -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=w8W2MQbueclED/EdGVDf+dmK+3CcuebQnzJF3C86Cms=; b=RoYgusx3Dmz4wb/S7HjfL3nVbH0oTPxbwZn9NV57WKQA7TcwOLlyyLEvsVqh9ekAat dyfPorf0i2pkVwyQc9Wu1XjEWY2AfVDQ93oetOlHBTftx+OHSD5V67ArjkyoXbAOlp4f 0lWRY55kkCBq7ZvPlQ3Jz63vIEauyImX4ygt4Nfe0Y7Edg4tV1ojBpCKcTsy3zAP4XAj LexXl/OJqqDvDFlwgftkactSMQqyM6/ebqtwSLF81E7dB+rDBVSdnmKMLApm5PaOG3+i ruGKjoYt9z0dE+GkmaHUoY22kccDGpozhyOlEguaOvn4/GpZa9CVbuTKPH82XpcI7ILU fWbA== 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=w8W2MQbueclED/EdGVDf+dmK+3CcuebQnzJF3C86Cms=; b=VOB1bO/0IeHE70N9KoYWWOA/hJsiipznxlFCFA7lgOhcGmzL3XExgyNRfdlSdMpySy 4h2jYUoZrHv8lN2ZIID0AXHTVdOxDNL/WRXftAjLwm+W+csZyzZOLtuHEMQ08kuH233w f+PxsQIOb4v+rh403CfOX7oZZ+33bm43urcH85kr68PxoA3fTP2MpKq2BeO7zvdWPJoz CMU6snRcPXJRF8Gk3rsVGjol9AOo6ZmpN63ctc9YqXAY9ZpJ1XuXUlSn0pENcUj9pSUK /XrC4zCW8uG6AsrZB/TguNiUOKpqnrvK6o7USJEJYQYmnr8crur4wyxVNwIxUhJdZqc1 KsZg== X-Gm-Message-State: AOAM530J5X+5hcDn9TBx4bJzIHy7pUKmgjPAzW8J12bVj3R4Bbar7+0a TH2bRIYwtaEEzkP4jjm3YDydfg== X-Google-Smtp-Source: ABdhPJzO3Pgqw165GbddTSxe5e3EPWbTf01GP+GboPRMzvyHQRXCHofMFlTlGmgVsX3sA3DlTvlSCA== X-Received: by 2002:a05:600c:2313:: with SMTP id 19mr5266534wmo.51.1591117301303; Tue, 02 Jun 2020 10:01:41 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id u7sm4582506wrm.23.2020.06.02.10.01.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jun 2020 10:01:40 -0700 (PDT) Date: Tue, 2 Jun 2020 18:01:39 +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: <20200602170139.GB3522@embecosm.com> References: <20200519163004.GA9045@delia> <20200602153830.GZ3522@embecosm.com> <20200602155211.GA3522@embecosm.com> <6382e582-c2f3-a999-f604-979958d6a064@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6382e582-c2f3-a999-f604-979958d6a064@suse.de> X-Operating-System: Linux/5.5.17-200.fc31.x86_64 (x86_64) X-Uptime: 18:00:11 up 43 days, 7:35, 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 17:01:46 -0000 * 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. Still passes regression tests. Thanks, Andrew --- diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 444cea01c36..46cb744bac4 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,20 @@ 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 # Exit first, so that the files are no longer in use. gdb_exit @@ -5228,6 +5239,13 @@ proc gdb_finish { } { } set banned_traced 0 } + + foreach varname [info globals] { + if {![info exists known_globals($varname)]} { + verbose -log "APB: Deleting '$varname'" + uplevel #0 unset $varname + } + } } global debug_format