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

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

[Tom de Vries]

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?

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

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> So, since a global name in one test-case can leak to another
Tom> test-case, setting a global name in one test-case can result in
Tom> a tcl error in another test-case that reuses the name in a different
Tom> way.

Tom> Warn about leaking a global array from a test-case.

This seems fine to me.

Tom> Also, add a possibility to skip the warning in a given test-case using
Tom> variable gdb_skip_check_global_vars, and use it in gdb.mi/mi2-var-child.exp
Tom> and gdb.mi/mi-var-cp.exp.

Is that because these tests are still buggy in this way, or something
deeper?

Tom

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

[Tom de Vries]

On 22-05-2020 22:15, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> So, since a global name in one test-case can leak to another
> Tom> test-case, setting a global name in one test-case can result in
> Tom> a tcl error in another test-case that reuses the name in a different
> Tom> way.
> 
> Tom> Warn about leaking a global array from a test-case.
> 
> This seems fine to me.
> 
> Tom> Also, add a possibility to skip the warning in a given test-case using
> Tom> variable gdb_skip_check_global_vars, and use it in gdb.mi/mi2-var-child.exp
> Tom> and gdb.mi/mi-var-cp.exp.
> 
> Is that because these tests are still buggy in this way, or something
> deeper?

The former.

Thanks,
- Tom

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

[Andrew Burgess]

* 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

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

[Andrew Burgess]

* 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
+	}
+    }
 
     # Exit first, so that the files are no longer in use.
     gdb_exit

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

[Tom de Vries]

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

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

[Andrew Burgess]

* 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

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

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-02 18:01:39 +0100]:

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

Here's a cleaned up version for consideration.

Thanks,
Andrew

---

commit f21ca01deceab0fbbfc22e960c1ee5178cb4a496
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Jun 2 21:11:10 2020 +0100

    gdb/testsuite: Prevent globals leaking between test scripts
    
    Many of the test scripts create variables in the global namespace,
    these variables will then be present for the following test scripts.
    In most cases this is harmless, but in some cases this can cause
    problems.
    
    For example, if a variable is created as an array in one script, but
    then assigned as a scalar in a different script, this will cause a TCL
    error.
    
    The solution proposed in this patch is to have the GDB test harness
    record a list of all known global variables at the point just before
    we source the test script.  Then, after the test script has run, we
    again iterate over all global variables.  Any variable that was not in
    the original list is deleted.
    
    The assumption here is that no test script should need to create a
    global variable that will outlive the lifetime of the test script
    itself.  With this patch in place all tests currently seem to pass, so
    the assumption seems to hold.
    
    gdb/testsuite/ChangeLog:
    
            * lib/gdb.exp (gdb_known_globals): New global.
            (gdb_setup_known_globals): New proc.
            (gdb_cleanup_globals): New proc.

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 444cea01c36..66de9d6be3f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5094,6 +5094,35 @@ set banned_procedures { strace }
 # if the banned variables and procedures are already traced.
 set banned_traced 0
 
+# Global array that holds the name of all global variables at the time
+# a test script is started.  After the test script has completed any
+# global not in this list is deleted.
+array set gdb_known_globals {}
+
+# Setup the GDB_KNOWN_GLOBALS array with the names of all current
+# global variables.
+proc gdb_setup_known_globals {} {
+    global gdb_known_globals
+
+    array set gdb_known_globals {}
+    foreach varname [info globals] {
+	set gdb_known_globals($varname) 1
+    }
+}
+
+# Cleanup the global namespace.  Any global not in the
+# GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
+# globals from one test script to another.
+proc gdb_cleanup_globals {} {
+    global gdb_known_globals
+
+    foreach varname [info globals] {
+	if {![info exists gdb_known_globals($varname)]} {
+	    uplevel #0 unset $varname
+	}
+    }
+}
+
 proc gdb_init { test_file_name } {
     # Reset the timeout value to the default.  This way, any testcase
     # that changes the timeout value without resetting it cannot affect
@@ -5196,13 +5225,16 @@ 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]
+    gdb_setup_known_globals
+    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 +5260,8 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    gdb_cleanup_globals
 }
 
 global debug_format

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

[Tom de Vries]

On 02-06-2020 22:18, Andrew Burgess wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-02 18:01:39 +0100]:
> 
>> * 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.
> 
> Here's a cleaned up version for consideration.
> 

I ran this through the testsuite.  There seems to be some problems left,
see incomplete dump below.

Thanks,
- Tom

ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp95 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp96 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp96 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: can't read "mi_gdb_prompt": no such variable
    while executing
"expect {
-i exp97 -timeout 10
        -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
            # We have a new format mi startup prompt.  If we are
            # running mi1,..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
variable
ERROR: (timeout) GDB never initialized after 10 seconds.
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.base/jit-elf.exp.
ERROR: can't read "jit_load_address": no such variable
    while executing
"expr $jit_load_address + $jit_load_increment * [expr $i-1]"
    (procedure "compile_and_download_n_jit_so" line 12)
    invoked from within
"compile_and_download_n_jit_so \
                      $jit_solib_basename $jit_solib_srcfile 2"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.base/jit-elf.exp" line 134)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.base/jit-elf.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.base/jit-elf.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
FAIL: gdb.btrace/reconnect.exp: first: record btrace enable
FAIL: gdb.btrace/reconnect.exp: first: stepi 19 (the program is no
longer running)
FAIL: gdb.btrace/reconnect.exp: first: info record
FAIL: gdb.btrace/reconnect.exp: first: disconnect
FAIL: gdb.btrace/reconnect.exp: second: info record
FAIL: gdb.btrace/reconnect.exp: second: record stop
FAIL: gdb.btrace/reconnect.exp: second: disconnect
FAIL: gdb.btrace/reconnect.exp: third: info record
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/derivation.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class \
    "a_instance" "" "class" "A" \
    {
        { field  public "A::value_type a;" }
        { field  public "A::value_type aa;" }
        { method p..."
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/derivation.exp" line 80)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/derivation.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/derivation.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/inherit.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class  "A" "ptype A (FIXME)" "class" "A"  {
            { field public "int a;" }
            { field public "int x;" }
        }"
    (procedure "test_ptype_si" line 8)
    invoked from within
"test_ptype_si"
    (procedure "do_tests" line 10)
    invoked from within
"do_tests"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/inherit.exp" line 722)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/inherit.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/inherit.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/nested-types.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class $name "ptype $name (limit = $limit)" $key  $name
$children"
    ("uplevel" body line 2)
    invoked from within
"uplevel 1 $body"
    invoked from within
"with_timeout_factor 1 {
            cp_test_ptype_class $name "ptype $name (limit = $limit)"
$key  $name $children
        }"
    ("uplevel" body line 1)
    invoked from within
"uplevel [list with_timeout_factor $factor $body]"
    (procedure "with_read1_timeout_factor" line 8)
    invoked from within
"with_read1_timeout_factor $read1_timeout_factor {
            cp_test_ptype_class $name "ptype $name (limit = $limit)"
$key  $name $children
        }"
    (procedure "test_nested_limit" line 33)
    invoked from within
"test_nested_limit -1 false"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/nested-types.exp"
line 317)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/nested-types.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/nested-types.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
FAIL: gdb.cp/no-dmgl-verbose.exp: setting breakpoint at 'f(std::string)'
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/ptype-flags.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class value $name "class" "Holder<int>" $contents  "" {}
$flags"
    (procedure "do_check" line 26)
    invoked from within
"do_check "basic test""
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/ptype-flags.exp" line 65)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/ptype-flags.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/ptype-flags.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class "foo_rr_instance1" "" "class" "foo" \
    {
        { method public "foo(void);" }
        { method public "foo(foo_lval_ref);" }
        { method publ..."
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp"
line 44)
    invoked from within
"source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/virtfunc.exp.
ERROR: can't read "debug_cp_test_ptype_class": no such variable
    while executing
"if {$debug_cp_test_ptype_class} {
        verbose -log $msg
    }"
    (procedure "cp_ptype_class_verbose" line 4)
    invoked from within
"cp_ptype_class_verbose "next line element: \"$elem\"""
    (procedure "cp_support_internal::next_line" line 12)
    invoked from within
"cp_support_internal::next_line $line_queue"
    (procedure "cp_test_ptype_class" line 183)
    invoked from within
"cp_test_ptype_class  "VA" "" "class" "VA"  {
            { field public "int va;" }
        }"
    (procedure "test_ptype_of_classes" line 5)
    invoked from within
"test_ptype_of_classes"
    (procedure "do_tests" line 9)
    invoked from within
"do_tests"
    (file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/virtfunc.exp" line 292)
    invoked from within
"source /data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/virtfunc.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.cp/virtfunc.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""

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

[Tom de Vries]

On 03-06-2020 10:47, Tom de Vries wrote:
> ERROR: can't read "mi_gdb_prompt": no such variable
>     while executing
> "expect {
> -i exp95 -timeout 10
>         -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
>             # We have a new format mi startup prompt.  If we are
>             # running mi1,..."
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
> variable

So, the following happens:
- a test-case imports mi-support.exp using load_lib
- mi-support.exp sets mi_gdb_prompt
- the test-case finishes and the new global mi_gdb_prompt is unset,
  because it was not set before the test-case
- a next test-case imports mi-support.exp using load_lib
- load_lib sees that the file already has been loaded, so it skips it
- mi_gdb_prompt remains unset
- the test-case uses mi_gdb_prompt, and we have an error.

Thanks,
- Tom

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

[Pedro Alves]

On 6/2/20 4:38 PM, Andrew Burgess wrote:

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

Hmm, I thought of this when I saw Tom's detection patch the first time,
but dismissed it, perhaps too quickly, thinking that it wouldn't
be safe.  You mention global state lazily created by test scripts, but I was
thinking of lazy initialized globals within Dejagnu itself, or whatever Tcl
library we might end up using the future, which might need to be carried
over from testcase to testcase.  Imagine, this in a Dejagnu function:

proc fail {} {
   global number_of_fails

   # Hadn't been set before.  (valid since Tcl 8.5; otherwise, imagine it wrapped in 'info exits')
   incr number_of_fails
}

This is obviously not the real "fail", it is just to
show the point, or the risk.  I imagined there might be similar
functions in random Dejagnu boards.

(
Now that I look, it seems like Dejagnu wants to be sure to initialize
all global variables it uses in runtest.exp:

 # Initialize a few global variables used by all tests.
 # `reset_vars' resets several of these, we define them here to document their
 # existence.  In fact, it would be nice if all globals used by some interface
 # of dejagnu proper were documented here.

And I guess we can always initialize some ourselves, if we find that missing.
)

So if people think the benefits outweigh the risks, then I guess
we should give it a go.

BTW, global variables alone aren't the full scope of the
bleeding between testcases.  There's also the case of
testcases overriding procedures, like gdb.base/dbx.exp,
but those are perhaps rare enough that we can continue
handling it "manually" as before.

Thanks,
Pedro Alves

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

[Tom de Vries]

On 03-06-2020 11:38, Tom de Vries wrote:
> On 03-06-2020 10:47, Tom de Vries wrote:
>> ERROR: can't read "mi_gdb_prompt": no such variable
>>     while executing
>> "expect {
>> -i exp95 -timeout 10
>>         -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
>>             # We have a new format mi startup prompt.  If we are
>>             # running mi1,..."
>>     ("uplevel" body line 1)
>>     invoked from within
>> "uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
>> variable
> 
> So, the following happens:
> - a test-case imports mi-support.exp using load_lib
> - mi-support.exp sets mi_gdb_prompt
> - the test-case finishes and the new global mi_gdb_prompt is unset,
>   because it was not set before the test-case
> - a next test-case imports mi-support.exp using load_lib
> - load_lib sees that the file already has been loaded, so it skips it
> - mi_gdb_prompt remains unset
> - the test-case uses mi_gdb_prompt, and we have an error.

This naive solution seems to work.

Perhaps we can override load_lib to do the same, automatically.

Thanks,
- Tom

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 66de9d6be3..e75842c83e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5099,6 +5099,13 @@ set banned_traced 0
 # global not in this list is deleted.
 array set gdb_known_globals {}

+array set gdb_persistent_globals {}
+
+proc gdb_persistent_global { varname } {
+    global gdb_persistent_globals
+    set gdb_persistent_globals($varname) 1
+}
+
 # Setup the GDB_KNOWN_GLOBALS array with the names of all current
 # global variables.
 proc gdb_setup_known_globals {} {
@@ -5114,10 +5121,14 @@ proc gdb_setup_known_globals {} {
 # GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
 # globals from one test script to another.
 proc gdb_cleanup_globals {} {
-    global gdb_known_globals
+    global gdb_known_globals gdb_persistent_globals

     foreach varname [info globals] {
        if {![info exists gdb_known_globals($varname)]} {
+           if { [info exists gdb_persistent_globals($varname)] } {
+               continue
+           }
+           verbose -log "gdb_cleanup_globals: deleting $varname"
            uplevel #0 unset $varname
        }
     }--
diff --git a/gdb/testsuite/lib/mi-support.exp
b/gdb/testsuite/lib/mi-support.exp
index 1e59919ab4..2804cfc308 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -25,6 +25,7 @@ global mi_gdb_prompt
 if ![info exists mi_gdb_prompt] then {
     set mi_gdb_prompt "\[(\]gdb\[)\] \r\n"
 }
+gdb_persistent_global mi_gdb_prompt

 global mi_inferior_tty_name

@@ -39,11 +40,16 @@ global gdb_main_spawn_id
 global mi_spawn_id

 set MIFLAGS "-i=mi"
+gdb_persistent_global MIFLAGS

 set thread_selected_re "=thread-selected,id=\"\[0-9\]+\"\r\n"
+gdb_persistent_global thread_selected_re
 set gdbindex_warning_re "&\"warning: Skipping \[^\r\n\]+ \.gdb_index
section in \[^\r\n\]+\"\r\n(?:&\"\\\\n\"\r\n
)?"
+gdb_persistent_global gdbindex_warning_re
 set library_loaded_re
"=library-loaded\[^\n\]+\"\r\n(?:$gdbindex_warning_re)?"
+gdb_persistent_global library_loaded_re
 set breakpoint_re
"=(?:breakpoint-created|breakpoint-deleted)\[^\n\]+\"\r\n"
+gdb_persistent_global breakpoint_re

 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary

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

[Tom de Vries]

On 03-06-2020 12:09, Tom de Vries wrote:
> On 03-06-2020 11:38, Tom de Vries wrote:
>> On 03-06-2020 10:47, Tom de Vries wrote:
>>> ERROR: can't read "mi_gdb_prompt": no such variable
>>>     while executing
>>> "expect {
>>> -i exp95 -timeout 10
>>>         -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
>>>             # We have a new format mi startup prompt.  If we are
>>>             # running mi1,..."
>>>     ("uplevel" body line 1)
>>>     invoked from within
>>> "uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
>>> variable
>>
>> So, the following happens:
>> - a test-case imports mi-support.exp using load_lib
>> - mi-support.exp sets mi_gdb_prompt
>> - the test-case finishes and the new global mi_gdb_prompt is unset,
>>   because it was not set before the test-case
>> - a next test-case imports mi-support.exp using load_lib
>> - load_lib sees that the file already has been loaded, so it skips it
>> - mi_gdb_prompt remains unset
>> - the test-case uses mi_gdb_prompt, and we have an error.
> 
> This naive solution seems to work.
> 
> Perhaps we can override load_lib to do the same, automatically.
> 

That seems to work, I'll test this through.

Thanks,
- Tom

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 66de9d6be3..bca47b96f2 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -25,6 +25,39 @@ if {$tool == ""} {
     exit 2
 }

+rename load_lib saved_load_lib
+
+array set gdb_persistent_globals {}
+
+proc gdb_persistent_global { varname } {
+    global gdb_persistent_globals
+    set gdb_persistent_globals($varname) 1
+}
+
+proc load_lib { file } {
+    array set known_global {}
+    foreach varname [info globals] {
+       set known_globals($varname) 1
+    }
+
+    set code [catch "saved_load_lib $file" result]
+
+    foreach varname [info globals] {
+       if { ![info exists known_globals($varname)] } {
+           gdb_persistent_global $varname
+       }
+    }
+
+    if {$code == 1} {
+       global errorInfo errorCode
+       return -code error -errorinfo $errorInfo -errorcode $errorCode
$result
+    } elseif {$code > 1} {
+       return -code $code $result
+    }
+
+    return $result
+}
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5114,10 +5147,14 @@ proc gdb_setup_known_globals {} {
 # GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
 # globals from one test script to another.
 proc gdb_cleanup_globals {} {
-    global gdb_known_globals
+    global gdb_known_globals gdb_persistent_globals

     foreach varname [info globals] {
        if {![info exists gdb_known_globals($varname)]} {
+           if { [info exists gdb_persistent_globals($varname)] } {
+               continue
+           }
+           verbose -log "gdb_cleanup_globals: deleting $varname"
            uplevel #0 unset $varname
        }
     }

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

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2020-06-03 12:24:09 +0200]:

> On 03-06-2020 12:09, Tom de Vries wrote:
> > On 03-06-2020 11:38, Tom de Vries wrote:
> >> On 03-06-2020 10:47, Tom de Vries wrote:
> >>> ERROR: can't read "mi_gdb_prompt": no such variable
> >>>     while executing
> >>> "expect {
> >>> -i exp95 -timeout 10
> >>>         -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
> >>>             # We have a new format mi startup prompt.  If we are
> >>>             # running mi1,..."
> >>>     ("uplevel" body line 1)
> >>>     invoked from within
> >>> "uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
> >>> variable
> >>
> >> So, the following happens:
> >> - a test-case imports mi-support.exp using load_lib
> >> - mi-support.exp sets mi_gdb_prompt
> >> - the test-case finishes and the new global mi_gdb_prompt is unset,
> >>   because it was not set before the test-case
> >> - a next test-case imports mi-support.exp using load_lib
> >> - load_lib sees that the file already has been loaded, so it skips it
> >> - mi_gdb_prompt remains unset
> >> - the test-case uses mi_gdb_prompt, and we have an error.

I couldn't work out why my testing was going fine if this problem
exists... then I realised that my testing script was causing the tests
to run in parallel!

I like the load_lib override solution.  I think a solution that avoids
having to place each test into a new namespace would be a good thing
if we can pull it off.

Thanks,
Andrew

> > 
> > This naive solution seems to work.
> > 
> > Perhaps we can override load_lib to do the same, automatically.
> > 
> 
> That seems to work, I'll test this through.
> 
> Thanks,
> - Tom
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 66de9d6be3..bca47b96f2 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -25,6 +25,39 @@ if {$tool == ""} {
>      exit 2
>  }
> 
> +rename load_lib saved_load_lib
> +
> +array set gdb_persistent_globals {}
> +
> +proc gdb_persistent_global { varname } {
> +    global gdb_persistent_globals
> +    set gdb_persistent_globals($varname) 1
> +}
> +
> +proc load_lib { file } {
> +    array set known_global {}
> +    foreach varname [info globals] {
> +       set known_globals($varname) 1
> +    }
> +
> +    set code [catch "saved_load_lib $file" result]
> +
> +    foreach varname [info globals] {
> +       if { ![info exists known_globals($varname)] } {
> +           gdb_persistent_global $varname
> +       }
> +    }
> +
> +    if {$code == 1} {
> +       global errorInfo errorCode
> +       return -code error -errorinfo $errorInfo -errorcode $errorCode
> $result
> +    } elseif {$code > 1} {
> +       return -code $code $result
> +    }
> +
> +    return $result
> +}
> +
>  load_lib libgloss.exp
>  load_lib cache.exp
>  load_lib gdb-utils.exp
> @@ -5114,10 +5147,14 @@ proc gdb_setup_known_globals {} {
>  # GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
>  # globals from one test script to another.
>  proc gdb_cleanup_globals {} {
> -    global gdb_known_globals
> +    global gdb_known_globals gdb_persistent_globals
> 
>      foreach varname [info globals] {
>         if {![info exists gdb_known_globals($varname)]} {
> +           if { [info exists gdb_persistent_globals($varname)] } {
> +               continue
> +           }
> +           verbose -log "gdb_cleanup_globals: deleting $varname"
>             uplevel #0 unset $varname
>         }
>      }
> 

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

[Tom de Vries]

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

On 03-06-2020 14:54, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2020-06-03 12:24:09 +0200]:
> 
>> On 03-06-2020 12:09, Tom de Vries wrote:
>>> On 03-06-2020 11:38, Tom de Vries wrote:
>>>> On 03-06-2020 10:47, Tom de Vries wrote:
>>>>> ERROR: can't read "mi_gdb_prompt": no such variable
>>>>>     while executing
>>>>> "expect {
>>>>> -i exp95 -timeout 10
>>>>>         -re "~\"GNU.*\r\n~\".*$mi_gdb_prompt$" {
>>>>>             # We have a new format mi startup prompt.  If we are
>>>>>             # running mi1,..."
>>>>>     ("uplevel" body line 1)
>>>>>     invoked from within
>>>>> "uplevel $body" TCL READ VARNAME can't read "mi_gdb_prompt": no such
>>>>> variable
>>>>
>>>> So, the following happens:
>>>> - a test-case imports mi-support.exp using load_lib
>>>> - mi-support.exp sets mi_gdb_prompt
>>>> - the test-case finishes and the new global mi_gdb_prompt is unset,
>>>>   because it was not set before the test-case
>>>> - a next test-case imports mi-support.exp using load_lib
>>>> - load_lib sees that the file already has been loaded, so it skips it
>>>> - mi_gdb_prompt remains unset
>>>> - the test-case uses mi_gdb_prompt, and we have an error.
> 
> I couldn't work out why my testing was going fine if this problem
> exists... then I realised that my testing script was causing the tests
> to run in parallel!
> 
> I like the load_lib override solution.  I think a solution that avoids
> having to place each test into a new namespace would be a good thing
> if we can pull it off.
> 

I've tested this with runtest -v, I'm currently re-testing without -v.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Prevent-globals-leaking-between-test-scripts.patch --]
[-- Type: text/x-patch, Size: 6431 bytes --]

gdb/testsuite: Prevent globals leaking between test scripts

Many of the test scripts create variables in the global namespace,
these variables will then be present for the following test scripts.
In most cases this is harmless, but in some cases this can cause
problems.

For example, if a variable is created as an array in one script, but
then assigned as a scalar in a different script, this will cause a TCL
error.

The solution proposed in this patch is to have the GDB test harness
record a list of all known global variables at the point just before
we source the test script.  Then, after the test script has run, we
again iterate over all global variables.  Any variable that was not in
the original list is deleted, unless it was marked as a persistent global
variable using gdb_persistent_global.

The assumption here is that no test script should need to create a
global variable that will outlive the lifetime of the test script
itself.  With this patch in place all tests currently seem to pass, so
the assumption seems to hold.

gdb/testsuite/ChangeLog:

2020-06-03  Andrew Burgess  <andrew.burgess@embecosm.com>
	    Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (gdb_known_globals, gdb_persistent_globals): New global.
	(gdb_persistent_global): New proc.
	(gdb_setup_known_globals): New proc.
	(gdb_cleanup_globals): New proc.
	* lib/gdb.exp (load_lib): New override proc.
	(gdb_stdin_log_init): Set var in_file as persistent global.
	* lib/pascal.exp (gdb_stdin_log_init): Set vars
	pascal_compiler_is_gpc, pascal_compiler_is_fpc, gpc_compiler and
	fpc_compiler as persistent global.
	* lib/tuiterm.exp (spawn): Don't assume gdb_spawn_name is set.

---
 gdb/testsuite/lib/gdb.exp     | 79 ++++++++++++++++++++++++++++++++++++++++++-
 gdb/testsuite/lib/pascal.exp  |  4 +++
 gdb/testsuite/lib/tuiterm.exp |  4 ++-
 3 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 444cea01c3..45e90a55b0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -25,6 +25,44 @@ if {$tool == ""} {
     exit 2
 }
 
+# Variable in which we keep track of globals that are allowed to be live
+# across test-cases.
+array set gdb_persistent_globals {}
+
+# Mark variable with name VARNAME as a persistent global.
+proc gdb_persistent_global { varname } {
+    global gdb_persistent_globals
+    set gdb_persistent_globals($varname) 1
+}
+
+# Override proc load_lib.
+rename load_lib saved_load_lib
+# Run the runtest version of load_lib, and mark all variables that were
+# created by this call as persistent.
+proc load_lib { file } {
+    array set known_global {}
+    foreach varname [info globals] {
+       set known_globals($varname) 1
+    }
+
+    set code [catch "saved_load_lib $file" result]
+
+    foreach varname [info globals] {
+       if { ![info exists known_globals($varname)] } {
+           gdb_persistent_global $varname
+       }
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif {$code > 1} {
+	return -code $code $result
+    }
+
+    return $result
+}
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5094,6 +5132,39 @@ set banned_procedures { strace }
 # if the banned variables and procedures are already traced.
 set banned_traced 0
 
+# Global array that holds the name of all global variables at the time
+# a test script is started.  After the test script has completed any
+# global not in this list is deleted.
+array set gdb_known_globals {}
+
+# Setup the GDB_KNOWN_GLOBALS array with the names of all current
+# global variables.
+proc gdb_setup_known_globals {} {
+    global gdb_known_globals
+
+    array set gdb_known_globals {}
+    foreach varname [info globals] {
+	set gdb_known_globals($varname) 1
+    }
+}
+
+# Cleanup the global namespace.  Any global not in the
+# GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
+# globals from one test script to another.
+proc gdb_cleanup_globals {} {
+    global gdb_known_globals gdb_persistent_globals
+
+    foreach varname [info globals] {
+	if {![info exists gdb_known_globals($varname)]} {
+	    if { [info exists gdb_persistent_globals($varname)] } {
+		continue
+	    }
+	    verbose -log "gdb_cleanup_globals: deleting $varname"
+	    uplevel #0 unset $varname
+	}
+    }
+}
+
 proc gdb_init { test_file_name } {
     # Reset the timeout value to the default.  This way, any testcase
     # that changes the timeout value without resetting it cannot affect
@@ -5196,13 +5267,16 @@ 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]
+    gdb_setup_known_globals
+    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 +5302,8 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    gdb_cleanup_globals
 }
 
 global debug_format
@@ -6944,6 +7020,7 @@ proc gdb_stdin_log_init { } {
 
     set logfile [standard_output_file_with_gdb_instance gdb.in]
     set in_file [open $logfile w]
+    gdb_persistent_global in_file
 }
 
 # Write to the file for logging gdb input.
diff --git a/gdb/testsuite/lib/pascal.exp b/gdb/testsuite/lib/pascal.exp
index aad69a2de0..95e3e637aa 100644
--- a/gdb/testsuite/lib/pascal.exp
+++ b/gdb/testsuite/lib/pascal.exp
@@ -47,6 +47,10 @@ proc pascal_init {} {
     set pascal_compiler_is_fpc 0
     set gpc_compiler [transform gpc]
     set fpc_compiler [transform fpc]
+    gdb_persistent_global pascal_compiler_is_gpc
+    gdb_persistent_global pascal_compiler_is_fpc
+    gdb_persistent_global gpc_compiler
+    gdb_persistent_global fpc_compiler
 
     if ![is_remote host] {
 	if { [info exists env(GPC)] } {
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 8c9f97af6e..8f82ea5f5c 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -27,7 +27,9 @@ proc spawn {args} {
     if { [info exists spawn_out] } {
 	set gdb_spawn_name $spawn_out(slave,name)
     } else {
-	unset gdb_spawn_name
+	if { [info exists gdb_spawn_name] } {
+	    unset gdb_spawn_name
+	}
     }
     return $result
 }

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

[Pedro Alves]

On 6/3/20 4:35 PM, Tom de Vries wrote:
> @@ -47,6 +47,10 @@ proc pascal_init {} {
>      set pascal_compiler_is_fpc 0
>      set gpc_compiler [transform gpc]
>      set fpc_compiler [transform fpc]
> +    gdb_persistent_global pascal_compiler_is_gpc
> +    gdb_persistent_global pascal_compiler_is_fpc
> +    gdb_persistent_global gpc_compiler
> +    gdb_persistent_global fpc_compiler

> @@ -47,6 +47,10 @@ proc pascal_init {} {
>      set pascal_compiler_is_fpc 0
>      set gpc_compiler [transform gpc]
>      set fpc_compiler [transform fpc]
> +    gdb_persistent_global pascal_compiler_is_gpc
> +    gdb_persistent_global pascal_compiler_is_fpc
> +    gdb_persistent_global gpc_compiler
> +    gdb_persistent_global fpc_compiler

Can we make gdb_persistent_global proc also do the "global $var"
in the caller's scope, so that it could be used to replace
the "global" statement?  With that, you would only have to declare
the variable once, and this hunk here would instead be:

diff --git c/gdb/testsuite/lib/pascal.exp w/gdb/testsuite/lib/pascal.exp
index aad69a2de0..ff11864294 100644
--- c/gdb/testsuite/lib/pascal.exp
+++ w/gdb/testsuite/lib/pascal.exp
@@ -33,10 +33,10 @@ set pascal_init_done 0
  
 proc pascal_init {} {
     global pascal_init_done
-    global pascal_compiler_is_gpc
-    global pascal_compiler_is_fpc
-    global gpc_compiler
-    global fpc_compiler
+    gdb_persistent_global pascal_compiler_is_gpc
+    gdb_persistent_global pascal_compiler_is_fpc
+    gdb_persistent_global gpc_compiler
+    gdb_persistent_global fpc_compiler
     global env

Thanks,
Pedro Alves

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

[Tom de Vries]

On 03-06-2020 11:49, Pedro Alves wrote:
> On 6/2/20 4:38 PM, Andrew Burgess wrote:
> 
>> 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?
> 
> Hmm, I thought of this when I saw Tom's detection patch the first time,
> but dismissed it, perhaps too quickly, thinking that it wouldn't
> be safe.  You mention global state lazily created by test scripts, but I was
> thinking of lazy initialized globals within Dejagnu itself, or whatever Tcl
> library we might end up using the future, which might need to be carried
> over from testcase to testcase.  Imagine, this in a Dejagnu function:
> 
> proc fail {} {
>    global number_of_fails
> 
>    # Hadn't been set before.  (valid since Tcl 8.5; otherwise, imagine it wrapped in 'info exits')
>    incr number_of_fails
> }
> 
> This is obviously not the real "fail", it is just to
> show the point, or the risk.  I imagined there might be similar
> functions in random Dejagnu boards.
> 

Agreed, that is a risk.

A less aggressive approach is also possible though: only prevent leaked
global arrays, and leave the rest of the global state unmodified.

> (
> Now that I look, it seems like Dejagnu wants to be sure to initialize
> all global variables it uses in runtest.exp:
> 
>  # Initialize a few global variables used by all tests.
>  # `reset_vars' resets several of these, we define them here to document their
>  # existence.  In fact, it would be nice if all globals used by some interface
>  # of dejagnu proper were documented here.
> 
> And I guess we can always initialize some ourselves, if we find that missing.
> )
>

Right, or, in terms of the latest proposed patch, call
gdb_persistent_global with the varname as argument.

> So if people think the benefits outweigh the risks, then I guess
> we should give it a go.
> 

I guess it's worth a try, it could be easily reverted if it cause
trouble.  If it causes trouble, it will take somebody's time, but OTOH
it will also take time to manually fix up test-cases to prevent the
originally proposed warning.

> BTW, global variables alone aren't the full scope of the
> bleeding between testcases.  There's also the case of
> testcases overriding procedures, like gdb.base/dbx.exp,
> but those are perhaps rare enough that we can continue
> handling it "manually" as before.

AFAICT, that test-case does an effort to undo the override, though I'm
not sure how certain it is that the undo will be executed.

Also, while working on the latest proposed patch, I also ran into
trouble related to the renaming of spawn in lib/tuiterm.exp. I wonder if
to address this we could wrap every tui test-case in
...
tui_env {
   ...
}
...
and letting the tui_env proc deal with renaming and restoring the spawn.
 We could even automate this in gdb_init/gdb_finish by calling
gdb_init_$subdir/gdb_finish_$subdir and handling it there.

Thanks,
- Tom

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

[Tom de Vries]

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

On 04-06-2020 13:16, Pedro Alves wrote:
> On 6/3/20 4:35 PM, Tom de Vries wrote:
>> @@ -47,6 +47,10 @@ proc pascal_init {} {
>>      set pascal_compiler_is_fpc 0
>>      set gpc_compiler [transform gpc]
>>      set fpc_compiler [transform fpc]
>> +    gdb_persistent_global pascal_compiler_is_gpc
>> +    gdb_persistent_global pascal_compiler_is_fpc
>> +    gdb_persistent_global gpc_compiler
>> +    gdb_persistent_global fpc_compiler
> 
>> @@ -47,6 +47,10 @@ proc pascal_init {} {
>>      set pascal_compiler_is_fpc 0
>>      set gpc_compiler [transform gpc]
>>      set fpc_compiler [transform fpc]
>> +    gdb_persistent_global pascal_compiler_is_gpc
>> +    gdb_persistent_global pascal_compiler_is_fpc
>> +    gdb_persistent_global gpc_compiler
>> +    gdb_persistent_global fpc_compiler
> 
> Can we make gdb_persistent_global proc also do the "global $var"
> in the caller's scope, so that it could be used to replace
> the "global" statement?  With that, you would only have to declare
> the variable once, and this hunk here would instead be:

Done, currently re-testing.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Prevent-globals-leaking-between-test-scripts.patch --]
[-- Type: text/x-patch, Size: 6896 bytes --]

gdb/testsuite: Prevent globals leaking between test scripts

Many of the test scripts create variables in the global namespace,
these variables will then be present for the following test scripts.
In most cases this is harmless, but in some cases this can cause
problems.

For example, if a variable is created as an array in one script, but
then assigned as a scalar in a different script, this will cause a TCL
error.

The solution proposed in this patch is to have the GDB test harness
record a list of all known global variables at the point just before
we source the test script.  Then, after the test script has run, we
again iterate over all global variables.  Any variable that was not in
the original list is deleted, unless it was marked as a persistent global
variable using gdb_persistent_global.

The assumption here is that no test script should need to create a
global variable that will outlive the lifetime of the test script
itself.  With this patch in place all tests currently seem to pass, so
the assumption seems to hold.

gdb/testsuite/ChangeLog:

2020-06-03  Andrew Burgess  <andrew.burgess@embecosm.com>
	    Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (gdb_known_globals, gdb_persistent_globals): New global.
	(gdb_persistent_global, gdb_persistent_global_no_decl): New proc.
	(gdb_setup_known_globals): New proc.
	(gdb_cleanup_globals): New proc.
	* lib/gdb.exp (load_lib): New override proc.
	(gdb_stdin_log_init): Set var in_file as persistent global.
	* lib/pascal.exp (gdb_stdin_log_init): Set vars
	pascal_compiler_is_gpc, pascal_compiler_is_fpc, gpc_compiler and
	fpc_compiler as persistent global.
	* lib/tuiterm.exp (spawn): Don't assume gdb_spawn_name is set.

---
 gdb/testsuite/lib/gdb.exp     | 92 ++++++++++++++++++++++++++++++++++++++++++-
 gdb/testsuite/lib/pascal.exp  |  8 ++--
 gdb/testsuite/lib/tuiterm.exp |  4 +-
 3 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 444cea01c3..65dd31f1ab 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -25,6 +25,57 @@ if {$tool == ""} {
     exit 2
 }
 
+# Variable in which we keep track of globals that are allowed to be live
+# across test-cases.
+array set gdb_persistent_globals {}
+
+# Mark variable names in ARG as a persistent global, and declare them as
+# global in the calling context.  Can be used to rewrite "global var_a var_b"
+# into "gdb_persistent_global var_a var_b".
+proc gdb_persistent_global { args } {
+    global gdb_persistent_globals
+    foreach varname $args {
+	uplevel 1 global $varname
+	set gdb_persistent_globals($varname) 1
+    }
+}
+
+# Mark variable names in ARG as a persistent global.
+proc gdb_persistent_global_no_decl { args } {
+    global gdb_persistent_globals
+    foreach varname $args {
+	set gdb_persistent_globals($varname) 1
+    }
+}
+
+# Override proc load_lib.
+rename load_lib saved_load_lib
+# Run the runtest version of load_lib, and mark all variables that were
+# created by this call as persistent.
+proc load_lib { file } {
+    array set known_global {}
+    foreach varname [info globals] {
+       set known_globals($varname) 1
+    }
+
+    set code [catch "saved_load_lib $file" result]
+
+    foreach varname [info globals] {
+       if { ![info exists known_globals($varname)] } {
+           gdb_persistent_global_no_decl $varname
+       }
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif {$code > 1} {
+	return -code $code $result
+    }
+
+    return $result
+}
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5094,6 +5145,38 @@ set banned_procedures { strace }
 # if the banned variables and procedures are already traced.
 set banned_traced 0
 
+# Global array that holds the name of all global variables at the time
+# a test script is started.  After the test script has completed any
+# global not in this list is deleted.
+array set gdb_known_globals {}
+
+# Setup the GDB_KNOWN_GLOBALS array with the names of all current
+# global variables.
+proc gdb_setup_known_globals {} {
+    global gdb_known_globals
+
+    array set gdb_known_globals {}
+    foreach varname [info globals] {
+	set gdb_known_globals($varname) 1
+    }
+}
+
+# Cleanup the global namespace.  Any global not in the
+# GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
+# globals from one test script to another.
+proc gdb_cleanup_globals {} {
+    global gdb_known_globals gdb_persistent_globals
+
+    foreach varname [info globals] {
+	if {![info exists gdb_known_globals($varname)]} {
+	    if { [info exists gdb_persistent_globals($varname)] } {
+		continue
+	    }
+	    uplevel #0 unset $varname
+	}
+    }
+}
+
 proc gdb_init { test_file_name } {
     # Reset the timeout value to the default.  This way, any testcase
     # that changes the timeout value without resetting it cannot affect
@@ -5196,13 +5279,16 @@ 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]
+    gdb_setup_known_globals
+    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 +5314,8 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    gdb_cleanup_globals
 }
 
 global debug_format
@@ -6935,7 +7023,7 @@ proc gdbserver_debug_enabled { } {
 # Open the file for logging gdb input
 
 proc gdb_stdin_log_init { } {
-    global in_file
+    gdb_persistent_global in_file
 
     if {[info exists in_file]} {
       # Close existing file.
diff --git a/gdb/testsuite/lib/pascal.exp b/gdb/testsuite/lib/pascal.exp
index aad69a2de0..ff11864294 100644
--- a/gdb/testsuite/lib/pascal.exp
+++ b/gdb/testsuite/lib/pascal.exp
@@ -33,10 +33,10 @@ set pascal_init_done 0
  
 proc pascal_init {} {
     global pascal_init_done
-    global pascal_compiler_is_gpc
-    global pascal_compiler_is_fpc
-    global gpc_compiler
-    global fpc_compiler
+    gdb_persistent_global pascal_compiler_is_gpc
+    gdb_persistent_global pascal_compiler_is_fpc
+    gdb_persistent_global gpc_compiler
+    gdb_persistent_global fpc_compiler
     global env
  
     if { $pascal_init_done == 1 } {
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 8c9f97af6e..8f82ea5f5c 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -27,7 +27,9 @@ proc spawn {args} {
     if { [info exists spawn_out] } {
 	set gdb_spawn_name $spawn_out(slave,name)
     } else {
-	unset gdb_spawn_name
+	if { [info exists gdb_spawn_name] } {
+	    unset gdb_spawn_name
+	}
     }
     return $result
 }

[PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Tom de Vries]

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

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

On 04-06-2020 13:40, Tom de Vries wrote:
> On 03-06-2020 11:49, Pedro Alves wrote:

>> BTW, global variables alone aren't the full scope of the
>> bleeding between testcases.  There's also the case of
>> testcases overriding procedures, like gdb.base/dbx.exp,
>> but those are perhaps rare enough that we can continue
>> handling it "manually" as before.
> 
> AFAICT, that test-case does an effort to undo the override, though I'm
> not sure how certain it is that the undo will be executed.
> 
> Also, while working on the latest proposed patch, I also ran into
> trouble related to the renaming of spawn in lib/tuiterm.exp. I wonder if
> to address this we could wrap every tui test-case in
> ...
> tui_env {
>    ...
> }
> ...
> and letting the tui_env proc deal with renaming and restoring the spawn.
>  We could even automate this in gdb_init/gdb_finish by calling
> gdb_init_$subdir/gdb_finish_$subdir and handling it there.
> 

That approach turned out to be not precise enough because also
gdb.python/tui-window.exp imports tuiterm.exp.

This fix uses a load_lib override.

Any comments?

Thanks
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Don-t-leak-tuiterm.exp-spawn-override.patch --]
[-- Type: text/x-patch, Size: 3873 bytes --]

[gdb/testsuite] Don't leak tuiterm.exp spawn override

In lib/tuiterm.exp the builtin spawn is overridden by a tui-specific version.

After running the first test-case that imports tuiterm.exp, the override
remains active, so it can cause trouble in subsequent test-cases, even if they
do not import tuiterm.exp.  See f.i. commit c8d4f6dfd9 "[gdb/testsuite] Fix
spawn in tuiterm.exp".

Fix this by:
- adding a mechanism in load_lib that allows a file foo.exp to
  define procs gdb_init_foo and gdb_finish_foo, where gdb_init_foo is executed
  during load_lib, and gdb_finish_foo is executed during gdb_finish.
- using this mechanism to do the spawn override in gdb_init_tuiterm, and
  restoring the original spawn in gdb_finish_tuiterm.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

	* lib/gdb.exp (gdb_finish_hooks): New global var.
	(load_lib): New override proc.
	(gdb_finish): Handle gdb_finish_hooks.
	* lib/tuiterm.exp (spawn): Rename to ...
	(tui_spawn): ... this.
	(toplevel): Move rename of spawn ...
	(gdb_init_tuiterm): ... here.  New proc.
	(gdb_finish_tuiterm): New proc.

---
 gdb/testsuite/lib/gdb.exp     | 42 ++++++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/tuiterm.exp | 17 +++++++++++++++--
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9a0620a2bf..986cfca99f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -25,6 +25,42 @@ if {$tool == ""} {
     exit 2
 }
 
+# List of procs to run in gdb_finish.
+set gdb_finish_hooks [list]
+
+# Override proc load_lib.
+rename load_lib saved_load_lib
+
+# Run overriden load_lib.  For FILE == foo.exp, run gdb_init_foo and schedule
+# gdb_finish_foo, if they exist.
+proc load_lib { file } {
+
+    # Call overridden load_lib version.
+    set code [catch "saved_load_lib $file" result]
+
+    # Do lib-specific initialization.
+    set lib_id [regsub -all {\.exp$} $file ""]
+    if { [info procs "gdb_init_${lib_id}"] != "" } {
+	gdb_init_${lib_id}
+    }
+
+    # Schedule lib-specific finalization.
+    if { [info procs "gdb_finish_${lib_id}"] != "" } {
+	global gdb_finish_hooks
+	lappend gdb_finish_hooks gdb_finish_${lib_id}
+    }
+
+    # Propagate errors.
+    if { $code == 1 } {
+       global errorInfo errorCode
+       return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif { $code > 1 } {
+       return -code $code $result
+    }
+
+    return $result
+}
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5225,6 +5261,12 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    global gdb_finish_hooks
+    foreach gdb_finish_hook $gdb_finish_hooks {
+	$gdb_finish_hook
+    }
+    set gdb_finish_hooks [list]
 }
 
 global debug_format
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 8c9f97af6e..680648564d 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -19,8 +19,7 @@
 # array; but dejagnu doesn't export this globally.  So, we have to
 # wrap spawn with our own function, so that we can capture this value.
 # The value is later used in calls to stty.
-rename spawn builtin_spawn
-proc spawn {args} {
+proc tuiterm_spawn { args } {
     set result [uplevel builtin_spawn $args]
     global gdb_spawn_name
     upvar spawn_out spawn_out
@@ -32,6 +31,20 @@ proc spawn {args} {
     return $result
 }
 
+# Initialize tuiterm.exp environment.
+proc gdb_init_tuiterm { } {
+    # Override spawn with tui_spawn.
+    rename spawn builtin_spawn
+    rename tuiterm_spawn spawn
+}
+
+# Finalize tuiterm.exp environment.
+proc gdb_finish_tuiterm { } {
+    # Restore spawn.
+    rename spawn tuiterm_spawn
+    rename builtin_spawn spawn
+}
+
 namespace eval Term {
     variable _rows
     variable _cols

[committed][gdb/testsuite] Make gdb.base/dbx.exp more robust

[Tom de Vries]

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

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

On 04-06-2020 13:40, Tom de Vries wrote:
>> BTW, global variables alone aren't the full scope of the
>> bleeding between testcases.  There's also the case of
>> testcases overriding procedures, like gdb.base/dbx.exp,
>> but those are perhaps rare enough that we can continue
>> handling it "manually" as before.
> AFAICT, that test-case does an effort to undo the override, though I'm
> not sure how certain it is that the undo will be executed.

I've committed patch below to make sure the undo is executed (and
similar for GDBFLAGS).

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Make-gdb.base-dbx.exp-more-robust.patch --]
[-- Type: text/x-patch, Size: 3977 bytes --]

[gdb/testsuite] Make gdb.base/dbx.exp more robust

Test-case gdb.base/dbx.exp overrides:
- the GDBFLAGS variable
- the gdb_file_cmd proc

There's code at the end of the test-case to restore both, but that's not
guaranteed to be executed.

Fix this by:
- using save_vars to restore GDBFLAGS
- using a new proc with_override to restore gdb_file_cmd

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-06-11  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (with_override): New proc, factored out of ...
	* gdb.base/dbx.exp: ... here.  Use with_override and save_vars.

---
 gdb/testsuite/gdb.base/dbx.exp | 35 ++++++++++++++++------------------
 gdb/testsuite/lib/gdb.exp      | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dbx.exp b/gdb/testsuite/gdb.base/dbx.exp
index 4299c44368..2a53f99a28 100644
--- a/gdb/testsuite/gdb.base/dbx.exp
+++ b/gdb/testsuite/gdb.base/dbx.exp
@@ -147,11 +147,8 @@ proc dbx_reinitialize_dir { subdir } {
 # right sequence of events, allowing gdb_load to do its normal thing? This way
 # remotes and simulators will work, too.
 #
-# [drow 2002-03-30]: We can restore the old gdb_file_cmd afterwards, though.
-set old_gdb_file_cmd_args [info args gdb_file_cmd]
-set old_gdb_file_cmd_body [info body gdb_file_cmd]
 
-proc gdb_file_cmd {arg} {
+proc local_gdb_file_cmd {arg} {
     global loadpath
     global loadfile
     global GDB
@@ -286,24 +283,24 @@ proc test_func { } {
 # Start with a fresh gdb.
 
 gdb_exit
-global GDBFLAGS
-set saved_gdbflags $GDBFLAGS
 
-set GDBFLAGS "$GDBFLAGS --dbx"
-gdb_start
-dbx_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
+with_override gdb_file_cmd local_gdb_file_cmd {
+    save_vars GDBFLAGS {
+	set GDBFLAGS "$GDBFLAGS --dbx"
 
-test_breakpoints
-test_assign
-test_whereis
-gdb_test "file average.c:1" "1\[ \t\]+/. This is a sample program.*"
-test_func
+	gdb_start
+	dbx_reinitialize_dir $srcdir/$subdir
+	gdb_load ${binfile}
 
-#exit and cleanup
-gdb_exit
+	test_breakpoints
+	test_assign
+	test_whereis
+	gdb_test "file average.c:1" "1\[ \t\]+/. This is a sample program.*"
+	test_func
 
-set GDBFLAGS $saved_gdbflags
-eval proc gdb_file_cmd {$old_gdb_file_cmd_args} {$old_gdb_file_cmd_body}
+	#exit and cleanup
+	gdb_exit
+    }
+}
 
 return 0
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9a0620a2bf..51f8a05464 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7200,5 +7200,48 @@ proc hex_in_list { val hexlist } {
     return [expr $index != -1]
 }
 
+# Override proc NAME to proc OVERRIDE for the duration of the execution of
+# BODY.
+
+proc with_override { name override body } {
+    # Implementation note: It's possible to implement the override using
+    # rename, like this:
+    #   rename $name save_$name
+    #   rename $override $name
+    #   set code [catch {uplevel 1 $body} result]
+    #   rename $name $override
+    #   rename save_$name $name
+    # but there are two issues here:
+    # - the save_$name might clash with an existing proc
+    # - the override is no longer available under its original name during
+    #   the override
+    # So, we use this more elaborate but cleaner mechanism.
+
+    # Save the old proc.
+    set old_args [info args $name]
+    set old_body [info body $name]
+
+    # Install the override.
+    set new_args [info args $override]
+    set new_body [info body $override]
+    eval proc $name {$new_args} {$new_body}
+
+    # Execute body.
+    set code [catch {uplevel 1 $body} result]
+
+    # Restore old proc.
+    eval proc $name {$old_args} {$old_body}
+
+    # Return as appropriate.
+    if { $code == 1 } {
+        global errorInfo errorCode
+        return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif { $code > 1 } {
+        return -code $code $result
+    }
+
+    return $result
+}
+
 # Always load compatibility stuff.
 load_lib future.exp

Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust

[Pedro Alves]

On 6/11/20 1:11 PM, Tom de Vries wrote:
> [ was: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array ]
> 
> On 04-06-2020 13:40, Tom de Vries wrote:
>>> BTW, global variables alone aren't the full scope of the
>>> bleeding between testcases.  There's also the case of
>>> testcases overriding procedures, like gdb.base/dbx.exp,
>>> but those are perhaps rare enough that we can continue
>>> handling it "manually" as before.
>> AFAICT, that test-case does an effort to undo the override, though I'm
>> not sure how certain it is that the undo will be executed.
> 
> I've committed patch below to make sure the undo is executed (and
> similar for GDBFLAGS).

Nice.

Thanks,
Pedro Alves

Re: [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This fix uses a load_lib override.

Tom> Any comments?

There was a report recently about gdb overrides like this breaking
with a newer dejagnu.

Maybe we need a gdb_load or something like that, instead of
monkey-patching dejagnu?  Or some other different approach.

I'm not really opposed to this FWIW.  It seems not likely to cause
problems.  Though I thought that about the other override too, so..

Tom

Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust

[Simon Marchi]

On 2020-06-11 8:11 a.m., Tom de Vries wrote:
> [ was: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array ]
> 
> On 04-06-2020 13:40, Tom de Vries wrote:
>>> BTW, global variables alone aren't the full scope of the
>>> bleeding between testcases.  There's also the case of
>>> testcases overriding procedures, like gdb.base/dbx.exp,
>>> but those are perhaps rare enough that we can continue
>>> handling it "manually" as before.
>> AFAICT, that test-case does an effort to undo the override, though I'm
>> not sure how certain it is that the undo will be executed.
> 
> I've committed patch below to make sure the undo is executed (and
> similar for GDBFLAGS).
> 
> Thanks,
> - Tom
> 

Ah!  That reminded me that I had a patch I had forgotten about that did something
similar:

  https://sourceware.org/pipermail/gdb-patches/2020-May/169109.html

I'm fine with your version.  One nit: you have an unnecessary return at the end of
the with_override proc.

Simon

Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust

[Tom de Vries]

On 11-06-2020 16:39, Simon Marchi wrote:
> On 2020-06-11 8:11 a.m., Tom de Vries wrote:
>> [ was: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array ]
>>
>> On 04-06-2020 13:40, Tom de Vries wrote:
>>>> BTW, global variables alone aren't the full scope of the
>>>> bleeding between testcases.  There's also the case of
>>>> testcases overriding procedures, like gdb.base/dbx.exp,
>>>> but those are perhaps rare enough that we can continue
>>>> handling it "manually" as before.
>>> AFAICT, that test-case does an effort to undo the override, though I'm
>>> not sure how certain it is that the undo will be executed.
>>
>> I've committed patch below to make sure the undo is executed (and
>> similar for GDBFLAGS).
>>
>> Thanks,
>> - Tom
>>
> 
> Ah!  That reminded me that I had a patch I had forgotten about that did something
> similar:
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-May/169109.html
> 

Sorry, didn't notice that one.

> I'm fine with your version.

Right, I see the solutions are really the same.

>  One nit: you have an unnecessary return at the end of
> the with_override proc.

Um, if I add this code to a random test-case:
...
diff --git a/gdb/testsuite/gdb.ada/O2_float_param.exp
b/gdb/testsuite/gdb.ada/O2_float_param.exp
index 09ebeec405..31c36c9cde 100644
--- a/gdb/testsuite/gdb.ada/O2_float_param.exp
+++ b/gdb/testsuite/gdb.ada/O2_float_param.exp
@@ -29,3 +29,17 @@ runto "increment"

 gdb_test "frame" \
          "#0\\s+callee\\.increment \\(val(=val@entry)?=99\\.0,
msg=\\.\\.\\.\\).*"
+
+proc foo { } {
+    return 1
+}
+
+proc foo_2 {} {
+    return 2
+}
+
+set res [foo]
+puts "RES: $res"
+
+set res [with_override foo foo_2 foo]
+puts "RES: $res"
...

And run it, I get:
...
RES: 1
RES: 2
...
but if I remove the return at the end of with_override, I get instead:
...
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp ...
RES: 1
RES:
...

So, I'm not sure why you say that it's not necessary.

Thanks,
- Tom

Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust

[Simon Marchi]

On 2020-06-11 10:52 a.m., Tom de Vries wrote:
> On 11-06-2020 16:39, Simon Marchi wrote:
>> On 2020-06-11 8:11 a.m., Tom de Vries wrote:
>>> [ was: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array ]
>>>
>>> On 04-06-2020 13:40, Tom de Vries wrote:
>>>>> BTW, global variables alone aren't the full scope of the
>>>>> bleeding between testcases.  There's also the case of
>>>>> testcases overriding procedures, like gdb.base/dbx.exp,
>>>>> but those are perhaps rare enough that we can continue
>>>>> handling it "manually" as before.
>>>> AFAICT, that test-case does an effort to undo the override, though I'm
>>>> not sure how certain it is that the undo will be executed.
>>>
>>> I've committed patch below to make sure the undo is executed (and
>>> similar for GDBFLAGS).
>>>
>>> Thanks,
>>> - Tom
>>>
>>
>> Ah!  That reminded me that I had a patch I had forgotten about that did something
>> similar:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2020-May/169109.html
>>
> 
> Sorry, didn't notice that one.

No problem, I had forgotten about it and it would probably have fallen
through the cracks actually... we (at least I) really need a way to track
patches.

> So, I'm not sure why you say that it's not necessary.

Ahh sorry, it's early.  I misread the code as:

+    # Return as appropriate.
+    if { $code == 1 } {
+        global errorInfo errorCode
+        return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+        return -code $code $result
+    }
+
+    return $result

All is good then :).

Simon

[committed][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Tom de Vries]

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

On 11-06-2020 15:55, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> This fix uses a load_lib override.
> 
> Tom> Any comments?
> 
> There was a report recently about gdb overrides like this breaking
> with a newer dejagnu.
> 

You're referring to this (
https://sourceware.org/pipermail/gdb/2020-June/048689.html ) ?

> Maybe we need a gdb_load or something like that, instead of
> monkey-patching dejagnu?  Or some other different approach.
> 
> I'm not really opposed to this FWIW.  It seems not likely to cause
> problems.  Though I thought that about the other override too, so..

OK, I dropped the load_lib overload.

Now the idiom is:
...
-load_lib tuiterm.exp
+tuiterm_env
...

I've also tried wrapping the test-case body using the tuiterm_env proc,
like this:
...
tuiterm_env {
  ...
}
...
but ran into curly-braces trouble in gdb.tui/new-layout.exp.

Committed.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Don-t-leak-tuiterm.exp-spawn-override.patch --]
[-- Type: text/x-patch, Size: 9958 bytes --]

[gdb/testsuite] Don't leak tuiterm.exp spawn override

In lib/tuiterm.exp the builtin spawn is overridden by a tui-specific version.

After running the first test-case that imports tuiterm.exp, the override
remains active, so it can cause trouble in subsequent test-cases, even if they
do not import tuiterm.exp.  See f.i. commit c8d4f6dfd9 "[gdb/testsuite] Fix
spawn in tuiterm.exp".

Fix this by:
- adding a variable gdb_finish_hooks which is a list of procs to run during
  gdb_finish
- adding a proc tuiterm_env that is used in test-cases instead of
  "load_lib tuiterm.exp".
- letting tuiterm_env:
  - install the tui-specific spawn version, and
  - use the gdb_finish_hooks to schedule restoring the builtin spawn
    version.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

	* lib/tuiterm.exp (spawn): Rename to ...
	(tui_spawn): ... this.
	(toplevel): Move rename of spawn ...
	(gdb_init_tuiterm): ... here.  New proc.
	(gdb_finish_tuiterm): New proc.
	* lib/gdb.exp (gdb_finish_hooks): New global var.
	(gdb_finish): Handle gdb_finish_hooks.
	(tuiterm_env): New proc.
	* gdb.python/tui-window.exp: Replace load_lib tuiterm.exp with
	tuiterm_env.
	* gdb.tui/basic.exp: Same.
	* gdb.tui/corefile-run.exp: Same.
	* gdb.tui/empty.exp: Same.
	* gdb.tui/list-before.exp: Same.
	* gdb.tui/list.exp: Same.
	* gdb.tui/main.exp: Same.
	* gdb.tui/new-layout.exp: Same.
	* gdb.tui/regs.exp: Same.
	* gdb.tui/resize.exp: Same.
	* gdb.tui/tui-layout-asm-short-prog.exp: Same.
	* gdb.tui/tui-layout-asm.exp: Same.
	* gdb.tui/tui-missing-src.exp: Same.
	* gdb.tui/winheight.exp: Same.

---
 gdb/testsuite/gdb.python/tui-window.exp            |  2 +-
 gdb/testsuite/gdb.tui/basic.exp                    |  2 +-
 gdb/testsuite/gdb.tui/corefile-run.exp             |  2 +-
 gdb/testsuite/gdb.tui/empty.exp                    |  2 +-
 gdb/testsuite/gdb.tui/list-before.exp              |  2 +-
 gdb/testsuite/gdb.tui/list.exp                     |  2 +-
 gdb/testsuite/gdb.tui/main.exp                     |  2 +-
 gdb/testsuite/gdb.tui/new-layout.exp               |  2 +-
 gdb/testsuite/gdb.tui/regs.exp                     |  2 +-
 gdb/testsuite/gdb.tui/resize.exp                   |  2 +-
 .../gdb.tui/tui-layout-asm-short-prog.exp          |  2 +-
 gdb/testsuite/gdb.tui/tui-layout-asm.exp           |  2 +-
 gdb/testsuite/gdb.tui/tui-missing-src.exp          |  2 +-
 gdb/testsuite/gdb.tui/winheight.exp                |  2 +-
 gdb/testsuite/lib/gdb.exp                          | 23 ++++++++++++++++++++++
 gdb/testsuite/lib/tuiterm.exp                      | 17 ++++++++++++++--
 16 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/gdb/testsuite/gdb.python/tui-window.exp b/gdb/testsuite/gdb.python/tui-window.exp
index 1a4feebe22..503823a229 100644
--- a/gdb/testsuite/gdb.python/tui-window.exp
+++ b/gdb/testsuite/gdb.python/tui-window.exp
@@ -16,7 +16,7 @@
 # Test a TUI window implemented in Python.
 
 load_lib gdb-python.exp
-load_lib tuiterm.exp
+tuiterm_env
 
 # This test doesn't care about the inferior.
 standard_testfile py-arch.c
diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index 34e60384c4..3e013a9515 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -15,7 +15,7 @@
 
 # Basic TUI tests.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/corefile-run.exp b/gdb/testsuite/gdb.tui/corefile-run.exp
index 440142f74e..1878770bdc 100644
--- a/gdb/testsuite/gdb.tui/corefile-run.exp
+++ b/gdb/testsuite/gdb.tui/corefile-run.exp
@@ -18,7 +18,7 @@
 #
 # Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/empty.exp b/gdb/testsuite/gdb.tui/empty.exp
index e5d0228062..89f49d6b7f 100644
--- a/gdb/testsuite/gdb.tui/empty.exp
+++ b/gdb/testsuite/gdb.tui/empty.exp
@@ -15,7 +15,7 @@
 
 # Test TUI resizing with empty windows.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile
 
diff --git a/gdb/testsuite/gdb.tui/list-before.exp b/gdb/testsuite/gdb.tui/list-before.exp
index d2f3ef472d..9c5eb5655e 100644
--- a/gdb/testsuite/gdb.tui/list-before.exp
+++ b/gdb/testsuite/gdb.tui/list-before.exp
@@ -15,7 +15,7 @@
 
 # Ensure that "list" before starting the TUI will affect the view.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/list.exp b/gdb/testsuite/gdb.tui/list.exp
index 41cec125d5..b1e59499c8 100644
--- a/gdb/testsuite/gdb.tui/list.exp
+++ b/gdb/testsuite/gdb.tui/list.exp
@@ -15,7 +15,7 @@
 
 # Ensure that "list" will switch to the source view.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/main.exp b/gdb/testsuite/gdb.tui/main.exp
index ae2393e6e9..fd3c2cd815 100644
--- a/gdb/testsuite/gdb.tui/main.exp
+++ b/gdb/testsuite/gdb.tui/main.exp
@@ -15,7 +15,7 @@
 
 # Test that "file" shows "main".
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/new-layout.exp b/gdb/testsuite/gdb.tui/new-layout.exp
index b71de7de5f..3ea4dd4069 100644
--- a/gdb/testsuite/gdb.tui/new-layout.exp
+++ b/gdb/testsuite/gdb.tui/new-layout.exp
@@ -15,7 +15,7 @@
 
 # Test "tui new-layout".
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index e31a47d47b..a9296a7d5f 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -15,7 +15,7 @@
 
 # Simple test of TUI register window.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/resize.exp b/gdb/testsuite/gdb.tui/resize.exp
index 1b3f407a02..fd1b35088a 100644
--- a/gdb/testsuite/gdb.tui/resize.exp
+++ b/gdb/testsuite/gdb.tui/resize.exp
@@ -15,7 +15,7 @@
 
 # Test TUI resizing.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
index 4aa1ba3046..50cb61f0fa 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
@@ -16,7 +16,7 @@
 # Ensure that 'layout asm' can scroll away from the last line of a
 # very short program using a page up sized scroll.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout-asm-short-prog.S
 
diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index 257321fec7..44f7a3a3a4 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -16,7 +16,7 @@
 # Ensure that 'layout asm' before starting the inferior puts us in the
 # asm layout and displays the disassembly for main.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/gdb.tui/tui-missing-src.exp b/gdb/testsuite/gdb.tui/tui-missing-src.exp
index 2d9a851bec..6b5c7fad4c 100644
--- a/gdb/testsuite/gdb.tui/tui-missing-src.exp
+++ b/gdb/testsuite/gdb.tui/tui-missing-src.exp
@@ -25,7 +25,7 @@
 #    layout must show the contents of f2.c.
 # 7. Going back to main() shall result in no contents again.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile
 
diff --git a/gdb/testsuite/gdb.tui/winheight.exp b/gdb/testsuite/gdb.tui/winheight.exp
index 7a17c311ea..8ac55f8472 100644
--- a/gdb/testsuite/gdb.tui/winheight.exp
+++ b/gdb/testsuite/gdb.tui/winheight.exp
@@ -15,7 +15,7 @@
 
 # Test the "winheight" command.
 
-load_lib "tuiterm.exp"
+tuiterm_env
 
 standard_testfile tui-layout.c
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 64e667c20e..e7fce6fee0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -25,6 +25,9 @@ if {$tool == ""} {
     exit 2
 }
 
+# List of procs to run in gdb_finish.
+set gdb_finish_hooks [list]
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5241,6 +5244,12 @@ proc gdb_finish { } {
 	}
 	set banned_traced 0
     }
+
+    global gdb_finish_hooks
+    foreach gdb_finish_hook $gdb_finish_hooks {
+	$gdb_finish_hook
+    }
+    set gdb_finish_hooks [list]
 }
 
 global debug_format
@@ -7259,5 +7268,19 @@ proc with_override { name override body } {
     return $result
 }
 
+# Setup tuiterm.exp environment.  To be used in test-cases instead of
+# "load_lib tuiterm.exp".  Calls initialization function and schedules
+# finalization function.
+proc tuiterm_env { } {
+    load_lib tuiterm.exp
+
+    # Do initialization.
+    tuiterm_env_init
+
+    # Schedule finalization.
+    global gdb_finish_hooks
+    lappend gdb_finish_hooks tuiterm_env_finish
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 8c9f97af6e..44cbc79730 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -19,8 +19,7 @@
 # array; but dejagnu doesn't export this globally.  So, we have to
 # wrap spawn with our own function, so that we can capture this value.
 # The value is later used in calls to stty.
-rename spawn builtin_spawn
-proc spawn {args} {
+proc tuiterm_spawn { args } {
     set result [uplevel builtin_spawn $args]
     global gdb_spawn_name
     upvar spawn_out spawn_out
@@ -32,6 +31,20 @@ proc spawn {args} {
     return $result
 }
 
+# Initialize tuiterm.exp environment.
+proc tuiterm_env_init { } {
+    # Override spawn with tui_spawn.
+    rename spawn builtin_spawn
+    rename tuiterm_spawn spawn
+}
+
+# Finalize tuiterm.exp environment.
+proc tuiterm_env_finish { } {
+    # Restore spawn.
+    rename spawn tuiterm_spawn
+    rename builtin_spawn spawn
+}
+
 namespace eval Term {
     variable _rows
     variable _cols

[committed] gdb/testsuite: Prevent globals leaking between test scripts

[Tom de Vries]

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

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

On 04-06-2020 14:29, Tom de Vries wrote:
> On 04-06-2020 13:16, Pedro Alves wrote:
>> On 6/3/20 4:35 PM, Tom de Vries wrote:
>>> @@ -47,6 +47,10 @@ proc pascal_init {} {
>>>      set pascal_compiler_is_fpc 0
>>>      set gpc_compiler [transform gpc]
>>>      set fpc_compiler [transform fpc]
>>> +    gdb_persistent_global pascal_compiler_is_gpc
>>> +    gdb_persistent_global pascal_compiler_is_fpc
>>> +    gdb_persistent_global gpc_compiler
>>> +    gdb_persistent_global fpc_compiler
>>
>>> @@ -47,6 +47,10 @@ proc pascal_init {} {
>>>      set pascal_compiler_is_fpc 0
>>>      set gpc_compiler [transform gpc]
>>>      set fpc_compiler [transform fpc]
>>> +    gdb_persistent_global pascal_compiler_is_gpc
>>> +    gdb_persistent_global pascal_compiler_is_fpc
>>> +    gdb_persistent_global gpc_compiler
>>> +    gdb_persistent_global fpc_compiler
>>
>> Can we make gdb_persistent_global proc also do the "global $var"
>> in the caller's scope, so that it could be used to replace
>> the "global" statement?  With that, you would only have to declare
>> the variable once, and this hunk here would instead be:
> 
> Done, currently re-testing.

Rebased on current trunk and re-tested. Committed as attached below.

Also, I dropped the tuiterm.exp bit, it's no longer necessary since
commit 8c74a764f2 "[gdb/testsuite] Don't leak tuiterm.exp spawn override".

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Prevent-globals-leaking-between-test-scripts.patch --]
[-- Type: text/x-patch, Size: 6554 bytes --]

gdb/testsuite: Prevent globals leaking between test scripts

Many of the test scripts create variables in the global namespace,
these variables will then be present for the following test scripts.
In most cases this is harmless, but in some cases this can cause
problems.

For example, if a variable is created as an array in one script, but
then assigned as a scalar in a different script, this will cause a TCL
error.

The solution proposed in this patch is to have the GDB test harness
record a list of all known global variables at the point just before
we source the test script.  Then, after the test script has run, we
again iterate over all global variables.  Any variable that was not in
the original list is deleted, unless it was marked as a persistent global
variable using gdb_persistent_global.

The assumption here is that no test script should need to create a
global variable that will outlive the lifetime of the test script
itself.  With this patch in place all tests currently seem to pass, so
the assumption seems to hold.

gdb/testsuite/ChangeLog:

2020-06-03  Andrew Burgess  <andrew.burgess@embecosm.com>
	    Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (gdb_known_globals, gdb_persistent_globals): New global.
	(gdb_persistent_global, gdb_persistent_global_no_decl): New proc.
	(gdb_setup_known_globals): New proc.
	(gdb_cleanup_globals): New proc.
	* lib/gdb.exp (load_lib): New override proc.
	(gdb_stdin_log_init): Set var in_file as persistent global.
	* lib/pascal.exp (gdb_stdin_log_init): Set vars
	pascal_compiler_is_gpc, pascal_compiler_is_fpc, gpc_compiler and
	fpc_compiler as persistent global.

---
 gdb/testsuite/lib/gdb.exp    | 90 +++++++++++++++++++++++++++++++++++++++++++-
 gdb/testsuite/lib/pascal.exp |  8 ++--
 2 files changed, 93 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e7fce6fee0..f502eb157d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -28,6 +28,57 @@ if {$tool == ""} {
 # List of procs to run in gdb_finish.
 set gdb_finish_hooks [list]
 
+# Variable in which we keep track of globals that are allowed to be live
+# across test-cases.
+array set gdb_persistent_globals {}
+
+# Mark variable names in ARG as a persistent global, and declare them as
+# global in the calling context.  Can be used to rewrite "global var_a var_b"
+# into "gdb_persistent_global var_a var_b".
+proc gdb_persistent_global { args } {
+    global gdb_persistent_globals
+    foreach varname $args {
+	uplevel 1 global $varname
+	set gdb_persistent_globals($varname) 1
+    }
+}
+
+# Mark variable names in ARG as a persistent global.
+proc gdb_persistent_global_no_decl { args } {
+    global gdb_persistent_globals
+    foreach varname $args {
+	set gdb_persistent_globals($varname) 1
+    }
+}
+
+# Override proc load_lib.
+rename load_lib saved_load_lib
+# Run the runtest version of load_lib, and mark all variables that were
+# created by this call as persistent.
+proc load_lib { file } {
+    array set known_global {}
+    foreach varname [info globals] {
+       set known_globals($varname) 1
+    }
+
+    set code [catch "saved_load_lib $file" result]
+
+    foreach varname [info globals] {
+       if { ![info exists known_globals($varname)] } {
+           gdb_persistent_global_no_decl $varname
+       }
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
+    } elseif {$code > 1} {
+	return -code $code $result
+    }
+
+    return $result
+}
+
 load_lib libgloss.exp
 load_lib cache.exp
 load_lib gdb-utils.exp
@@ -5094,6 +5145,38 @@ set banned_procedures { strace }
 # if the banned variables and procedures are already traced.
 set banned_traced 0
 
+# Global array that holds the name of all global variables at the time
+# a test script is started.  After the test script has completed any
+# global not in this list is deleted.
+array set gdb_known_globals {}
+
+# Setup the GDB_KNOWN_GLOBALS array with the names of all current
+# global variables.
+proc gdb_setup_known_globals {} {
+    global gdb_known_globals
+
+    array set gdb_known_globals {}
+    foreach varname [info globals] {
+	set gdb_known_globals($varname) 1
+    }
+}
+
+# Cleanup the global namespace.  Any global not in the
+# GDB_KNOWN_GLOBALS array is unset, this ensures we don't "leak"
+# globals from one test script to another.
+proc gdb_cleanup_globals {} {
+    global gdb_known_globals gdb_persistent_globals
+
+    foreach varname [info globals] {
+	if {![info exists gdb_known_globals($varname)]} {
+	    if { [info exists gdb_persistent_globals($varname)] } {
+		continue
+	    }
+	    uplevel #0 unset $varname
+	}
+    }
+}
+
 proc gdb_init { test_file_name } {
     # Reset the timeout value to the default.  This way, any testcase
     # that changes the timeout value without resetting it cannot affect
@@ -5198,6 +5281,8 @@ proc gdb_init { test_file_name } {
 
     set res [default_gdb_init $test_file_name]
 
+    gdb_setup_known_globals
+
     # Dejagnu overrides proc unknown.  The dejagnu version may trigger in a
     # test-case but abort the entire test run.  To fix this, we install a
     # local version here, which reverts dejagnu's override, and restore
@@ -5215,6 +5300,7 @@ proc gdb_finish { } {
     global gdbserver_reconnect_p
     global gdb_prompt
     global cleanfiles
+    global known_globals
 
     # Restore dejagnu's version of proc unknown.
     rename ::unknown ""
@@ -5250,6 +5336,8 @@ proc gdb_finish { } {
 	$gdb_finish_hook
     }
     set gdb_finish_hooks [list]
+
+    gdb_cleanup_globals
 }
 
 global debug_format
@@ -6957,7 +7045,7 @@ proc gdbserver_debug_enabled { } {
 # Open the file for logging gdb input
 
 proc gdb_stdin_log_init { } {
-    global in_file
+    gdb_persistent_global in_file
 
     if {[info exists in_file]} {
       # Close existing file.
diff --git a/gdb/testsuite/lib/pascal.exp b/gdb/testsuite/lib/pascal.exp
index aad69a2de0..ff11864294 100644
--- a/gdb/testsuite/lib/pascal.exp
+++ b/gdb/testsuite/lib/pascal.exp
@@ -33,10 +33,10 @@ set pascal_init_done 0
  
 proc pascal_init {} {
     global pascal_init_done
-    global pascal_compiler_is_gpc
-    global pascal_compiler_is_fpc
-    global gpc_compiler
-    global fpc_compiler
+    gdb_persistent_global pascal_compiler_is_gpc
+    gdb_persistent_global pascal_compiler_is_fpc
+    gdb_persistent_global gpc_compiler
+    gdb_persistent_global fpc_compiler
     global env
  
     if { $pascal_init_done == 1 } {

Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

>> There was a report recently about gdb overrides like this breaking
>> with a newer dejagnu.

Tom> You're referring to this (
Tom> https://sourceware.org/pipermail/gdb/2020-June/048689.html ) ?

Yeah.

Tom> Now the idiom is:
Tom> ...
Tom> -load_lib tuiterm.exp
Tom> +tuiterm_env
Tom> ...

Looks good, thanks.

Tom> I've also tried wrapping the test-case body using the tuiterm_env proc,
Tom> like this:
Tom> ...
Tom> tuiterm_env {
Tom>   ...
Tom> }
Tom> ...
Tom> but ran into curly-braces trouble in gdb.tui/new-layout.exp.

Very unexpected.

Tom

Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Tom de Vries]

On 6/15/20 9:46 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
>>> There was a report recently about gdb overrides like this breaking
>>> with a newer dejagnu.
> 
> Tom> You're referring to this (
> Tom> https://sourceware.org/pipermail/gdb/2020-June/048689.html ) ?
> 
> Yeah.
> 
> Tom> Now the idiom is:
> Tom> ...
> Tom> -load_lib tuiterm.exp
> Tom> +tuiterm_env
> Tom> ...
> 
> Looks good, thanks.
> 
> Tom> I've also tried wrapping the test-case body using the tuiterm_env proc,
> Tom> like this:
> Tom> ...
> Tom> tuiterm_env {
> Tom>   ...
> Tom> }
> Tom> ...
> Tom> but ran into curly-braces trouble in gdb.tui/new-layout.exp.
> 
> Very unexpected.

That's essentially:
...
$ cat ./test.tcl
#!/usr/bin/tclsh

puts "1: {"

proc do_this { body } {
    return [uplevel 1 $body]
}

do_this {
    puts "2: {"
}
...
and:
...
$ ./test.tcl
1: {
missing close-brace
    while executing
"do_this {"
    (file "./test.tcl" line 9)
...

Thanks,
- Tom

Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override

[Andreas Schwab]

On Jun 17 2020, Tom de Vries wrote:

> do_this {
>     puts "2: {"
> }

This is unbalanced.  Since double quotes are not special in brace
enclosed strings, the close brace matches the open brace in the second
line, not the first one.  If you want to include a lone brace in a tcl
string, you need to write it with double quotes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."