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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-02 18:01:39 +0100]:

> * Tom de Vries <tdevries@suse.de> [2020-06-02 18:31:39 +0200]:
> 
> > On 02-06-2020 17:52, Andrew Burgess wrote:
> > > * 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
> > > +	}
> > > +    }
> > 
> > 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.

Here's a cleaned up version for consideration.

Thanks,
Andrew

---

commit f21ca01deceab0fbbfc22e960c1ee5178cb4a496
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Jun 2 21:11:10 2020 +0100

    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.
    
    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:
    
            * lib/gdb.exp (gdb_known_globals): New global.
            (gdb_setup_known_globals): New proc.
            (gdb_cleanup_globals): New proc.

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 444cea01c36..66de9d6be3f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5094,6 +5094,35 @@ 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
+
+    foreach varname [info globals] {
+	if {![info exists gdb_known_globals($varname)]} {
+	    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 +5225,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 +5260,8 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    gdb_cleanup_globals
 }
 
 global debug_format