Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array

Andrew Burgess
2020-06-02T15:52:14Z

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-02 16:38:30 +0100]:

> * Tom de Vries <tdevries@suse.de> [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
+ }
+ }

# Exit first, so that the files are no longer in use.
gdb_exit