[gdb/testsuite] Warn about leaked global array

Message ID 5c3d667e-7a88-0de6-6a19-6b3c4a9130e4@suse.de
State New
Headers show
Series
  • [gdb/testsuite] Warn about leaked global array
Related show

Commit Message

Tom de Vries May 18, 2020, 6:18 a.m.
[ 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

Comments

Kevin Buettner via Gdb-patches May 18, 2020, 10:41 a.m. | #1
On 5/18/20 7:18 AM, Tom de Vries wrote:

> I managed to find a better way of dealing with this, using "upvar #0".

> I've also added more comments to the patch.


Cool.

> +# 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"]


They can be local or global.  See man expect:

 Expect takes a rather liberal view of scoping. In particular, variables read 
 by commands specific to the Expect program will be sought first from the local
 scope, and if not found, in the global scope. For example, this obviates the need to
 place "global timeout" in every procedure you   write that uses expect. On the other hand,
 variables written are always in the local scope (unless a "global" command has been issued).
 The most common problem this causes is when spawn is executed in a procedure.
 Outside the procedure, spawn_id no longer exists, so the spawned process is no
 longer accessible simply because of scoping. Add a "global spawn_id" to such a procedure. 

For example, mi_gdb_test does:

proc mi_gdb_test { args } {
    global verbose
    global mi_gdb_prompt
    global GDB expect_out
               ^^^^^^^^^^
    global inferior_exited_re async
    upvar timeout timeout

and then gdb.mi/mi-basics.exp does:

proc test_path_specification {} {
...
    global expect_out

...
    mi_gdb_test "-environment-path" "\\\^done,path=\"(.*)\"" "environment-path"
    set orig_path $expect_out(3,string)
...

So making expect_out global allows gdb.mi/mi-basics.exp to reference it.

We could alternatively switch mi_gdb_test (and whatever other procedure
does a similar thing) to doing "upvar expect_out expect_out" and
get rid of "global expect_out".  Not sure it's worth the bother.

> +	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


Tabs / spaces above.

Thanks,
Pedro Alves
Tom de Vries May 19, 2020, 4:34 p.m. | #2
On 18-05-2020 12:41, Pedro Alves wrote:
> On 5/18/20 7:18 AM, Tom de Vries wrote:

> 

>> I managed to find a better way of dealing with this, using "upvar #0".

>> I've also added more comments to the patch.

> 

> Cool.

> 

>> +# 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"]

> 

> They can be local or global.  See man expect:

> 

>  Expect takes a rather liberal view of scoping. In particular, variables read 

>  by commands specific to the Expect program will be sought first from the local

>  scope, and if not found, in the global scope. For example, this obviates the need to

>  place "global timeout" in every procedure you   write that uses expect. On the other hand,

>  variables written are always in the local scope (unless a "global" command has been issued).

>  The most common problem this causes is when spawn is executed in a procedure.

>  Outside the procedure, spawn_id no longer exists, so the spawned process is no

>  longer accessible simply because of scoping. Add a "global spawn_id" to such a procedure. 

> 

> For example, mi_gdb_test does:

> 

> proc mi_gdb_test { args } {

>     global verbose

>     global mi_gdb_prompt

>     global GDB expect_out

>                ^^^^^^^^^^

>     global inferior_exited_re async

>     upvar timeout timeout

> 

> and then gdb.mi/mi-basics.exp does:

> 

> proc test_path_specification {} {

> ...

>     global expect_out

> 

> ...

>     mi_gdb_test "-environment-path" "\\\^done,path=\"(.*)\"" "environment-path"

>     set orig_path $expect_out(3,string)

> ...

> 

> So making expect_out global allows gdb.mi/mi-basics.exp to reference it.

> 

> We could alternatively switch mi_gdb_test (and whatever other procedure

> does a similar thing) to doing "upvar expect_out expect_out" and

> get rid of "global expect_out".  Not sure it's worth the bother.

> 


OK, that's helpful, thanks for the info.

>> +	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

> 

> Tabs / spaces above.

> 


Fixed, and resubmitted as part of a patch series that also includes
warning fixes here (
https://sourceware.org/pipermail/gdb-patches/2020-May/168771.html ).

Also, fixing the warnings turned out to be more complicated than I
thought, so I ended up moving the bit about how to fix the warning into
a gdb/testsuite/README section (part of patch 2/3).

Thanks,
- Tom

Patch

[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