[PATCH][gdb/testsuite] Warn about leaked global array

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

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

[ was: Re: [PATCH v8 6/6] gdb/infrun: handle already-exited threads when
attempting to stop ]

On 15-05-2020 19:17, Tom de Vries wrote:
> On 15-05-2020 17:46, Pedro Alves wrote:
>> Curious -- is there a reason for the underscores?
>>
> 
> Yeah, I'm trying to avoid this:
> ...
> $ cat test.tcl
> #!/usr/bin/tclsh
> 
> proc foo { var } {
>     global $var
> }
> 
> foo var
> $ ./test.tcl
> variable "var" already exists
>     while executing
> "global $var"
>     (procedure "foo" line 2)
>     invoked from within
> "foo var"
>     (file "./test.tcl" line 7)
> ...

I managed to find a better way of dealing with this, using "upvar #0".
I've also added more comments to the patch.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Warn-about-leaked-global-array.patch --]
[-- Type: text/x-patch, Size: 4585 bytes --]

[gdb/testsuite] Warn about leaked global array

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.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-05-18  Tom de Vries  <tdevries@suse.de>

	* 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/testsuite/lib/gdb.exp | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f7d20bd94f..285736ee92 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5048,6 +5048,95 @@ 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]
+}
+
+# Check global variables not in gdb_global_vars.
+
+proc check_global_vars { } {
+    # Sample state after running test.
+    global gdb_global_vars
+    set vars [info globals]
+
+    # I'm not sure these two should actually be global, but at least there
+    # seems to be no harm in having these as globals, given that we don't
+    # expect to reuse these names as scalars.
+    set skip [list "expect_out" "spawn_out"]
+
+    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.
+	# The way to fix this is to wrap the test-case in a namespace and to
+	# change the global variable into a namespace variable:
+	# namespace eval $testfile {
+	#     variable var
+	#     ...
+	# }
+	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 +5266,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