From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 2F0933851C2B for ; Tue, 2 Jun 2020 16:31:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2F0933851C2B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id C1683B11A; Tue, 2 Jun 2020 16:31:45 +0000 (UTC) Subject: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20200519163004.GA9045@delia> <20200602153830.GZ3522@embecosm.com> <20200602155211.GA3522@embecosm.com> From: Tom de Vries Autocrypt: addr=tdevries@suse.de; keydata= xsBNBF0ltCcBCADDhsUnMMdEXiHFfqJdXeRvgqSEUxLCy/pHek88ALuFnPTICTwkf4g7uSR7 HvOFUoUyu8oP5mNb4VZHy3Xy8KRZGaQuaOHNhZAT1xaVo6kxjswUi3vYgGJhFMiLuIHdApoc u5f7UbV+egYVxmkvVLSqsVD4pUgHeSoAcIlm3blZ1sDKviJCwaHxDQkVmSsGXImaAU+ViJ5l CwkvyiiIifWD2SoOuFexZyZ7RUddLosgsO0npVUYbl6dEMq2a5ijGF6/rBs1m3nAoIgpXk6P TCKlSWVW6OCneTaKM5C387972qREtiArTakRQIpvDJuiR2soGfdeJ6igGA1FZjU+IsM5ABEB AAHNH1RvbSBkZSBWcmllcyA8dGRldnJpZXNAc3VzZS5kZT7CwKsEEwEIAD4WIQSsnSe5hKbL MK1mGmjuhV2rbOJEoAUCXSW0JwIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAh CRDuhV2rbOJEoBYhBKydJ7mEpsswrWYaaO6FXats4kSgc48H/Ra2lq5p3dHsrlQLqM7N68Fo eRDf3PMevXyMlrCYDGLVncQwMw3O/AkousktXKQ42DPJh65zoXB22yUt8m0g12xkLax98KFJ 5NyUloa6HflLl+wQL/uZjIdNUQaHQLw3HKwRMVi4l0/Jh/TygYG1Dtm8I4o708JS4y8GQxoQ UL0z1OM9hyM3gI2WVTTyprsBHy2EjMOu/2Xpod95pF8f90zBLajy6qXEnxlcsqreMaqmkzKn 3KTZpWRxNAS/IH3FbGQ+3RpWkNGSJpwfEMVCeyK5a1n7yt1podd1ajY5mA1jcaUmGppqx827 8TqyteNe1B/pbiUt2L/WhnTgW1NC1QDOwE0EXSW0JwEIAM99H34Bu4MKM7HDJVt864MXbx7B 1M93wVlpJ7Uq+XDFD0A0hIal028j+h6jA6bhzWto4RUfDl/9mn1StngNVFovvwtfzbamp6+W pKHZm9X5YvlIwCx131kTxCNDcF+/adRW4n8CU3pZWYmNVqhMUiPLxElA6QhXTtVBh1RkjCZQ Kmbd1szvcOfaD8s+tJABJzNZsmO2hVuFwkDrRN8Jgrh92a+yHQPd9+RybW2l7sJv26nkUH5Z 5s84P6894ebgimcprJdAkjJTgprl1nhgvptU5M9Uv85Pferoh2groQEAtRPlCGrZ2/2qVNe9 XJfSYbiyedvApWcJs5DOByTaKkcAEQEAAcLAkwQYAQgAJhYhBKydJ7mEpsswrWYaaO6FXats 4kSgBQJdJbQnAhsMBQkDwmcAACEJEO6FXats4kSgFiEErJ0nuYSmyzCtZhpo7oVdq2ziRKD3 twf7BAQBZ8TqR812zKAD7biOnWIJ0McV72PFBxmLIHp24UVe0ZogtYMxSWKLg3csh0yLVwc7 H3vldzJ9AoK3Qxp0Q6K/rDOeUy3HMqewQGcqrsRRh0NXDIQk5CgSrZslPe47qIbe3O7ik/MC q31FNIAQJPmKXX25B115MMzkSKlv4udfx7KdyxHrTSkwWZArLQiEZj5KG4cCKhIoMygPTA3U yGaIvI/BGOtHZ7bEBVUCFDFfOWJ26IOCoPnSVUvKPEOH9dv+sNy7jyBsP5QxeTqwxC/1ZtNS DUCSFQjqA6bEGwM22dP8OUY6SC94x1G81A9/xbtm9LQxKm0EiDH8KBMLfQ== Message-ID: <6382e582-c2f3-a999-f604-979958d6a064@suse.de> Date: Tue, 2 Jun 2020 18:31:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200602155211.GA3522@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-16.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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 16:31:45 -0000 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. Thanks, - Tom