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

[Tom de Vries <tdevries@suse.de>]

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.

Thanks,
- Tom