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

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

* 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.

Still passes regression tests.

Thanks,
Andrew

---

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 444cea01c36..46cb744bac4 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,20 @@ 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
 
     # Exit first, so that the files are no longer in use.
     gdb_exit
@@ -5228,6 +5239,13 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    foreach varname [info globals] {
+	if {![info exists known_globals($varname)]} {
+	    verbose -log "APB: Deleting '$varname'"
+	    uplevel #0 unset $varname
+	}
+    }
 }
 
 global debug_format