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

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

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

Thanks,
Andrew




> 
> Thanks,
> - Tom
> 
> [gdb/testsuite] Warn about leaked global array
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-05-18  Tom de Vries  <tdevries@suse.de>
> 
> 	PR testsuite/25996
> 	* lib/gdb.exp (global_array_exists, global_unset, save_global_vars)
> 	(check_global_vars): New proc.
> 	(gdb_init): Call save_global_vars.
> 	(gdb_finish): Call check_global_vars.
> 	* gdb.mi/mi-var-cp.exp: Set gdb_skip_check_global_vars.
> 	* gdb.mi/mi2-var-child.exp: Same.
> 
> ---
>  gdb/testsuite/gdb.mi/mi-var-cp.exp     |  3 ++
>  gdb/testsuite/gdb.mi/mi2-var-child.exp |  3 ++
>  gdb/testsuite/lib/gdb.exp              | 94 ++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.mi/mi-var-cp.exp b/gdb/testsuite/gdb.mi/mi-var-cp.exp
> index 8c6a624f80..dc32ddc346 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-cp.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-cp.exp
> @@ -23,6 +23,9 @@ if [mi_gdb_start] {
>      continue
>  }
>  
> +# FIXME.  See check_global_vars in lib/gdb.exp.
> +set gdb_skip_check_global_vars 1
> +
>  standard_testfile .cc
>  
>  if [get_compiler_info "c++"] {
> diff --git a/gdb/testsuite/gdb.mi/mi2-var-child.exp b/gdb/testsuite/gdb.mi/mi2-var-child.exp
> index e32858fbd3..b96ac41815 100644
> --- a/gdb/testsuite/gdb.mi/mi2-var-child.exp
> +++ b/gdb/testsuite/gdb.mi/mi2-var-child.exp
> @@ -27,6 +27,9 @@ if [mi_gdb_start] {
>      continue
>  }
>  
> +# FIXME.  See check_global_vars in lib/gdb.exp.
> +set gdb_skip_check_global_vars 1
> +
>  standard_testfile var-cmd.c
>  
>  if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index f7d20bd94f..a0d558c943 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5048,6 +5048,98 @@ proc standard_testfile {args} {
>      }
>  }
>  
> +# Returns 1 if GLOBALVARNAME is a global array.
> +
> +proc global_array_exists { globalvarname } {
> +    # Introduce local alias of global variable $globalvarname.  Using
> +    # "global $globalvarname" instead is simpler, but this may result in a
> +    # clash with local name "globalvarname".
> +    upvar #0 $globalvarname globalvar
> +    return [array exists globalvar]
> +}
> +
> +# Unset global variable GLOBALVARNAME.
> +
> +proc global_unset { globalvarname } {
> +    # Introduce local alias of global variable $globalvarname.
> +    upvar #0 $globalvarname globalvar
> +    unset globalvar
> +}
> +
> +# Save global vars to variable gdb_global_vars.
> +
> +proc save_global_vars { test_file_name } {
> +    # Save for the warning.
> +    global gdb_test_file_name
> +    set gdb_test_file_name $test_file_name
> +
> +    # Sample state before running test.
> +    global gdb_global_vars
> +    set gdb_global_vars [info globals]
> +
> +    global gdb_skip_check_global_vars
> +    if { [info exists gdb_skip_check_global_vars]} {
> +	unset gdb_skip_check_global_vars
> +    }
> +}
> +
> +# Check global variables not in gdb_global_vars.
> +
> +proc check_global_vars { } {
> +    global gdb_skip_check_global_vars
> +    if { [info exists gdb_skip_check_global_vars]} {
> +	return
> +    }
> +    # Sample state after running test.
> +    global gdb_global_vars
> +    set vars [info globals]
> +
> +    set skip [list "expect_out" "spawn_out" "auto_index"]
> +
> +    foreach var $vars {
> +	if { ![global_array_exists $var] } {
> +	    continue
> +	}
> +
> +	set found [lsearch -exact $gdb_global_vars $var]
> +	if { $found != -1 } {
> +	    # Already present before running test.
> +	    continue
> +	}
> +
> +	set found [lsearch -exact $skip $var]
> +	if { $found != -1 } {
> +	    continue
> +	}
> +
> +	# 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 the test-case.
> +	#
> +	# For a description on how to fix this, see "Wrapping test-cases in
> +	# Tcl namespaces" in gdb/testsuite/README.
> +	global gdb_test_file_name
> +	warning "$gdb_test_file_name.exp defined global array $var"
> +
> +	# If the variable remains set, we won't warn for the next test where
> +	# it's leaked, so unset.
> +	global_unset $var
> +    }
> +}
> +
>  # The default timeout used when testing GDB commands.  We want to use
>  # the same timeout as the default dejagnu timeout, unless the user has
>  # already provided a specific value (probably through a site.exp file).
> @@ -5177,10 +5269,12 @@ proc gdb_init { test_file_name } {
>      global gdb_instances
>      set gdb_instances 0
>  
> +    save_global_vars $test_file_name
>      return [default_gdb_init $test_file_name]
>  }
>  
>  proc gdb_finish { } {
> +    check_global_vars
>      global gdbserver_reconnect_p
>      global gdb_prompt
>      global cleanfiles