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 79E0D3851C28 for ; Thu, 4 Jun 2020 11:41:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 79E0D3851C28 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 D006EAC52; Thu, 4 Jun 2020 11:41:05 +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> <72b3c10c-d316-9a0e-13e9-56dee7c765a7@redhat.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: <5d6277ac-76ea-5f47-ab9e-7da58fbddd6e@suse.de> Date: Thu, 4 Jun 2020 13:40:59 +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: <72b3c10c-d316-9a0e-13e9-56dee7c765a7@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, 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 11:41:04 -0000 On 03-06-2020 11:49, Pedro Alves wrote: > On 6/2/20 4:38 PM, Andrew Burgess wrote: > >> 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? > > Hmm, I thought of this when I saw Tom's detection patch the first time, > but dismissed it, perhaps too quickly, thinking that it wouldn't > be safe. You mention global state lazily created by test scripts, but I was > thinking of lazy initialized globals within Dejagnu itself, or whatever Tcl > library we might end up using the future, which might need to be carried > over from testcase to testcase. Imagine, this in a Dejagnu function: > > proc fail {} { > global number_of_fails > > # Hadn't been set before. (valid since Tcl 8.5; otherwise, imagine it wrapped in 'info exits') > incr number_of_fails > } > > This is obviously not the real "fail", it is just to > show the point, or the risk. I imagined there might be similar > functions in random Dejagnu boards. > Agreed, that is a risk. A less aggressive approach is also possible though: only prevent leaked global arrays, and leave the rest of the global state unmodified. > ( > Now that I look, it seems like Dejagnu wants to be sure to initialize > all global variables it uses in runtest.exp: > > # Initialize a few global variables used by all tests. > # `reset_vars' resets several of these, we define them here to document their > # existence. In fact, it would be nice if all globals used by some interface > # of dejagnu proper were documented here. > > And I guess we can always initialize some ourselves, if we find that missing. > ) > Right, or, in terms of the latest proposed patch, call gdb_persistent_global with the varname as argument. > So if people think the benefits outweigh the risks, then I guess > we should give it a go. > I guess it's worth a try, it could be easily reverted if it cause trouble. If it causes trouble, it will take somebody's time, but OTOH it will also take time to manually fix up test-cases to prevent the originally proposed warning. > BTW, global variables alone aren't the full scope of the > bleeding between testcases. There's also the case of > testcases overriding procedures, like gdb.base/dbx.exp, > but those are perhaps rare enough that we can continue > handling it "manually" as before. AFAICT, that test-case does an effort to undo the override, though I'm not sure how certain it is that the undo will be executed. Also, while working on the latest proposed patch, I also ran into trouble related to the renaming of spawn in lib/tuiterm.exp. I wonder if to address this we could wrap every tui test-case in ... tui_env { ... } ... and letting the tui_env proc deal with renaming and restoring the spawn. We could even automate this in gdb_init/gdb_finish by calling gdb_init_$subdir/gdb_finish_$subdir and handling it there. Thanks, - Tom