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 7DEE2388F051 for ; Thu, 4 Jun 2020 12:29:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7DEE2388F051 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 D43ACAC4A; Thu, 4 Jun 2020 12:29:32 +0000 (UTC) Subject: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array To: Pedro Alves , Andrew Burgess Cc: gdb-patches@sourceware.org References: <20200519163004.GA9045@delia> <20200602153830.GZ3522@embecosm.com> <20200602155211.GA3522@embecosm.com> <6382e582-c2f3-a999-f604-979958d6a064@suse.de> <20200602170139.GB3522@embecosm.com> <20200602201829.GC3522@embecosm.com> <191ae1d7-3363-08a9-b9fe-f006f9186708@suse.de> <20200603125412.GD3522@embecosm.com> <002b67f7-6966-5c91-a995-59556fb8afba@suse.de> 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: <2f239c51-4e7f-72ad-081c-4a06d528a7d2@suse.de> Date: Thu, 4 Jun 2020 14:29:28 +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: Content-Type: multipart/mixed; boundary="------------BD8B8E485B874204C8442296" Content-Language: en-US X-Spam-Status: No, score=-16.4 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: Thu, 04 Jun 2020 12:29:32 -0000 This is a multi-part message in MIME format. --------------BD8B8E485B874204C8442296 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 04-06-2020 13:16, Pedro Alves wrote: > On 6/3/20 4:35 PM, Tom de Vries wrote: >> @@ -47,6 +47,10 @@ proc pascal_init {} { >> set pascal_compiler_is_fpc 0 >> set gpc_compiler [transform gpc] >> set fpc_compiler [transform fpc] >> + gdb_persistent_global pascal_compiler_is_gpc >> + gdb_persistent_global pascal_compiler_is_fpc >> + gdb_persistent_global gpc_compiler >> + gdb_persistent_global fpc_compiler > >> @@ -47,6 +47,10 @@ proc pascal_init {} { >> set pascal_compiler_is_fpc 0 >> set gpc_compiler [transform gpc] >> set fpc_compiler [transform fpc] >> + gdb_persistent_global pascal_compiler_is_gpc >> + gdb_persistent_global pascal_compiler_is_fpc >> + gdb_persistent_global gpc_compiler >> + gdb_persistent_global fpc_compiler > > Can we make gdb_persistent_global proc also do the "global $var" > in the caller's scope, so that it could be used to replace > the "global" statement? With that, you would only have to declare > the variable once, and this hunk here would instead be: Done, currently re-testing. Thanks, - Tom --------------BD8B8E485B874204C8442296 Content-Type: text/x-patch; charset=UTF-8; name="0001-gdb-testsuite-Prevent-globals-leaking-between-test-scripts.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename*0="0001-gdb-testsuite-Prevent-globals-leaking-between-test-scri"; filename*1="pts.patch" 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, unless it was marked as a persistent global variable using gdb_persistent_global. 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: 2020-06-03 Andrew Burgess Tom de Vries * lib/gdb.exp (gdb_known_globals, gdb_persistent_globals): New global. (gdb_persistent_global, gdb_persistent_global_no_decl): New proc. (gdb_setup_known_globals): New proc. (gdb_cleanup_globals): New proc. * lib/gdb.exp (load_lib): New override proc. (gdb_stdin_log_init): Set var in_file as persistent global. * lib/pascal.exp (gdb_stdin_log_init): Set vars pascal_compiler_is_gpc, pascal_compiler_is_fpc, gpc_compiler and fpc_compiler as persistent global. * lib/tuiterm.exp (spawn): Don't assume gdb_spawn_name is set. --- gdb/testsuite/lib/gdb.exp | 92 ++++++++++++++++++++++++++++++++++++++++++- gdb/testsuite/lib/pascal.exp | 8 ++-- gdb/testsuite/lib/tuiterm.exp | 4 +- 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 444cea01c3..65dd31f1ab 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -25,6 +25,57 @@ if {$tool == ""} { exit 2 } +# Variable in which we keep track of globals that are allowed to be live +# across test-cases. +array set gdb_persistent_globals {} + +# Mark variable names in ARG as a persistent global, and declare them as +# global in the calling context. Can be used to rewrite "global var_a var_b" +# into "gdb_persistent_global var_a var_b". +proc gdb_persistent_global { args } { + global gdb_persistent_globals + foreach varname $args { + uplevel 1 global $varname + set gdb_persistent_globals($varname) 1 + } +} + +# Mark variable names in ARG as a persistent global. +proc gdb_persistent_global_no_decl { args } { + global gdb_persistent_globals + foreach varname $args { + set gdb_persistent_globals($varname) 1 + } +} + +# Override proc load_lib. +rename load_lib saved_load_lib +# Run the runtest version of load_lib, and mark all variables that were +# created by this call as persistent. +proc load_lib { file } { + array set known_global {} + foreach varname [info globals] { + set known_globals($varname) 1 + } + + set code [catch "saved_load_lib $file" result] + + foreach varname [info globals] { + if { ![info exists known_globals($varname)] } { + gdb_persistent_global_no_decl $varname + } + } + + if {$code == 1} { + global errorInfo errorCode + return -code error -errorinfo $errorInfo -errorcode $errorCode $result + } elseif {$code > 1} { + return -code $code $result + } + + return $result +} + load_lib libgloss.exp load_lib cache.exp load_lib gdb-utils.exp @@ -5094,6 +5145,38 @@ 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 gdb_persistent_globals + + foreach varname [info globals] { + if {![info exists gdb_known_globals($varname)]} { + if { [info exists gdb_persistent_globals($varname)] } { + continue + } + 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 +5279,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 +5314,8 @@ proc gdb_finish { } { } set banned_traced 0 } + + gdb_cleanup_globals } global debug_format @@ -6935,7 +7023,7 @@ proc gdbserver_debug_enabled { } { # Open the file for logging gdb input proc gdb_stdin_log_init { } { - global in_file + gdb_persistent_global in_file if {[info exists in_file]} { # Close existing file. diff --git a/gdb/testsuite/lib/pascal.exp b/gdb/testsuite/lib/pascal.exp index aad69a2de0..ff11864294 100644 --- a/gdb/testsuite/lib/pascal.exp +++ b/gdb/testsuite/lib/pascal.exp @@ -33,10 +33,10 @@ set pascal_init_done 0 proc pascal_init {} { global pascal_init_done - global pascal_compiler_is_gpc - global pascal_compiler_is_fpc - global gpc_compiler - global fpc_compiler + gdb_persistent_global pascal_compiler_is_gpc + gdb_persistent_global pascal_compiler_is_fpc + gdb_persistent_global gpc_compiler + gdb_persistent_global fpc_compiler global env if { $pascal_init_done == 1 } { diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp index 8c9f97af6e..8f82ea5f5c 100644 --- a/gdb/testsuite/lib/tuiterm.exp +++ b/gdb/testsuite/lib/tuiterm.exp @@ -27,7 +27,9 @@ proc spawn {args} { if { [info exists spawn_out] } { set gdb_spawn_name $spawn_out(slave,name) } else { - unset gdb_spawn_name + if { [info exists gdb_spawn_name] } { + unset gdb_spawn_name + } } return $result } --------------BD8B8E485B874204C8442296--