* [PATCH 3/3][gdb/testsuite] Warn about leaked global array @ 2020-05-19 16:30 Tom de Vries 2020-05-22 20:15 ` Tom Tromey 2020-06-02 15:38 ` Andrew Burgess 0 siblings, 2 replies; 30+ messages in thread From: Tom de Vries @ 2020-05-19 16:30 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-05-19 16:30 [PATCH 3/3][gdb/testsuite] Warn about leaked global array Tom de Vries @ 2020-05-22 20:15 ` Tom Tromey 2020-06-02 13:08 ` Tom de Vries 2020-06-02 15:38 ` Andrew Burgess 1 sibling, 1 reply; 30+ messages in thread From: Tom Tromey @ 2020-05-22 20:15 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-05-22 20:15 ` Tom Tromey @ 2020-06-02 13:08 ` Tom de Vries 0 siblings, 0 replies; 30+ messages in thread From: Tom de Vries @ 2020-06-02 13:08 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-05-19 16:30 [PATCH 3/3][gdb/testsuite] Warn about leaked global array Tom de Vries 2020-05-22 20:15 ` Tom Tromey @ 2020-06-02 15:38 ` Andrew Burgess 2020-06-02 15:52 ` Andrew Burgess 2020-06-03 9:49 ` [PATCH 3/3][gdb/testsuite] Warn about leaked global array Pedro Alves 1 sibling, 2 replies; 30+ messages in thread From: Andrew Burgess @ 2020-06-02 15:38 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 15:38 ` Andrew Burgess @ 2020-06-02 15:52 ` Andrew Burgess 2020-06-02 16:31 ` Tom de Vries 2020-06-03 9:49 ` [PATCH 3/3][gdb/testsuite] Warn about leaked global array Pedro Alves 1 sibling, 1 reply; 30+ messages in thread From: Andrew Burgess @ 2020-06-02 15:52 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 15:52 ` Andrew Burgess @ 2020-06-02 16:31 ` Tom de Vries 2020-06-02 17:01 ` Andrew Burgess 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-02 16:31 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 16:31 ` Tom de Vries @ 2020-06-02 17:01 ` Andrew Burgess 2020-06-02 20:18 ` Andrew Burgess 0 siblings, 1 reply; 30+ messages in thread From: Andrew Burgess @ 2020-06-02 17:01 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 17:01 ` Andrew Burgess @ 2020-06-02 20:18 ` Andrew Burgess 2020-06-03 8:47 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Andrew Burgess @ 2020-06-02 20:18 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 20:18 ` Andrew Burgess @ 2020-06-03 8:47 ` Tom de Vries 2020-06-03 9:38 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-03 8:47 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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"" ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 8:47 ` Tom de Vries @ 2020-06-03 9:38 ` Tom de Vries 2020-06-03 10:09 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-03 9:38 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 9:38 ` Tom de Vries @ 2020-06-03 10:09 ` Tom de Vries 2020-06-03 10:24 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-03 10:09 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 10:09 ` Tom de Vries @ 2020-06-03 10:24 ` Tom de Vries 2020-06-03 12:54 ` Andrew Burgess 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-03 10:24 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 } } ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 10:24 ` Tom de Vries @ 2020-06-03 12:54 ` Andrew Burgess 2020-06-03 15:35 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Andrew Burgess @ 2020-06-03 12:54 UTC (permalink / raw) To: Tom de Vries; +Cc: gdb-patches * 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 > } > } > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 12:54 ` Andrew Burgess @ 2020-06-03 15:35 ` Tom de Vries 2020-06-04 11:16 ` Pedro Alves 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-03 15:35 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches [-- 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 } ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 15:35 ` Tom de Vries @ 2020-06-04 11:16 ` Pedro Alves 2020-06-04 12:29 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Pedro Alves @ 2020-06-04 11:16 UTC (permalink / raw) To: Tom de Vries, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-04 11:16 ` Pedro Alves @ 2020-06-04 12:29 ` Tom de Vries 2020-06-12 13:11 ` [committed] gdb/testsuite: Prevent globals leaking between test scripts Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-04 12:29 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess; +Cc: gdb-patches [-- 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 } ^ permalink raw reply [flat|nested] 30+ messages in thread
* [committed] gdb/testsuite: Prevent globals leaking between test scripts 2020-06-04 12:29 ` Tom de Vries @ 2020-06-12 13:11 ` Tom de Vries 0 siblings, 0 replies; 30+ messages in thread From: Tom de Vries @ 2020-06-12 13:11 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess; +Cc: gdb-patches [-- 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 } { ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-02 15:38 ` Andrew Burgess 2020-06-02 15:52 ` Andrew Burgess @ 2020-06-03 9:49 ` Pedro Alves 2020-06-04 11:40 ` Tom de Vries 1 sibling, 1 reply; 30+ messages in thread From: Pedro Alves @ 2020-06-03 9:49 UTC (permalink / raw) To: Andrew Burgess, Tom de Vries; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array 2020-06-03 9:49 ` [PATCH 3/3][gdb/testsuite] Warn about leaked global array Pedro Alves @ 2020-06-04 11:40 ` Tom de Vries 2020-06-05 10:06 ` [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override Tom de Vries 2020-06-11 12:11 ` [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust Tom de Vries 0 siblings, 2 replies; 30+ messages in thread From: Tom de Vries @ 2020-06-04 11:40 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-04 11:40 ` Tom de Vries @ 2020-06-05 10:06 ` Tom de Vries 2020-06-11 13:55 ` Tom Tromey 2020-06-11 12:11 ` [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust Tom de Vries 1 sibling, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-05 10:06 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess; +Cc: gdb-patches, Tom Tromey [-- 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-05 10:06 ` [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override Tom de Vries @ 2020-06-11 13:55 ` Tom Tromey 2020-06-12 11:36 ` [committed][gdb/testsuite] " Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom Tromey @ 2020-06-11 13:55 UTC (permalink / raw) To: Tom de Vries; +Cc: Pedro Alves, Andrew Burgess, gdb-patches, 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-11 13:55 ` Tom Tromey @ 2020-06-12 11:36 ` Tom de Vries 2020-06-15 19:46 ` Tom Tromey 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-12 11:36 UTC (permalink / raw) To: Tom Tromey; +Cc: Pedro Alves, Andrew Burgess, gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-12 11:36 ` [committed][gdb/testsuite] " Tom de Vries @ 2020-06-15 19:46 ` Tom Tromey 2020-06-17 14:55 ` Tom de Vries 0 siblings, 1 reply; 30+ messages in thread From: Tom Tromey @ 2020-06-15 19:46 UTC (permalink / raw) To: Tom de Vries; +Cc: Tom Tromey, Pedro Alves, Andrew Burgess, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-15 19:46 ` Tom Tromey @ 2020-06-17 14:55 ` Tom de Vries 2020-06-17 15:28 ` Andreas Schwab 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-17 14:55 UTC (permalink / raw) To: Tom Tromey; +Cc: Pedro Alves, Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Don't leak tuiterm.exp spawn override 2020-06-17 14:55 ` Tom de Vries @ 2020-06-17 15:28 ` Andreas Schwab 0 siblings, 0 replies; 30+ messages in thread From: Andreas Schwab @ 2020-06-17 15:28 UTC (permalink / raw) To: Tom de Vries; +Cc: Tom Tromey, Pedro Alves, gdb-patches 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." ^ permalink raw reply [flat|nested] 30+ messages in thread
* [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust 2020-06-04 11:40 ` Tom de Vries 2020-06-05 10:06 ` [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override Tom de Vries @ 2020-06-11 12:11 ` Tom de Vries 2020-06-11 12:16 ` Pedro Alves 2020-06-11 14:39 ` Simon Marchi 1 sibling, 2 replies; 30+ messages in thread From: Tom de Vries @ 2020-06-11 12:11 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess; +Cc: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust 2020-06-11 12:11 ` [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust Tom de Vries @ 2020-06-11 12:16 ` Pedro Alves 2020-06-11 14:39 ` Simon Marchi 1 sibling, 0 replies; 30+ messages in thread From: Pedro Alves @ 2020-06-11 12:16 UTC (permalink / raw) To: Tom de Vries, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust 2020-06-11 12:11 ` [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust Tom de Vries 2020-06-11 12:16 ` Pedro Alves @ 2020-06-11 14:39 ` Simon Marchi 2020-06-11 14:52 ` Tom de Vries 1 sibling, 1 reply; 30+ messages in thread From: Simon Marchi @ 2020-06-11 14:39 UTC (permalink / raw) To: Tom de Vries, Pedro Alves, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust 2020-06-11 14:39 ` Simon Marchi @ 2020-06-11 14:52 ` Tom de Vries 2020-06-11 14:59 ` Simon Marchi 0 siblings, 1 reply; 30+ messages in thread From: Tom de Vries @ 2020-06-11 14:52 UTC (permalink / raw) To: Simon Marchi, Pedro Alves, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust 2020-06-11 14:52 ` Tom de Vries @ 2020-06-11 14:59 ` Simon Marchi 0 siblings, 0 replies; 30+ messages in thread From: Simon Marchi @ 2020-06-11 14:59 UTC (permalink / raw) To: Tom de Vries, Pedro Alves, Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-06-17 15:28 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-19 16:30 [PATCH 3/3][gdb/testsuite] Warn about leaked global array Tom de Vries 2020-05-22 20:15 ` Tom Tromey 2020-06-02 13:08 ` Tom de Vries 2020-06-02 15:38 ` Andrew Burgess 2020-06-02 15:52 ` Andrew Burgess 2020-06-02 16:31 ` Tom de Vries 2020-06-02 17:01 ` Andrew Burgess 2020-06-02 20:18 ` Andrew Burgess 2020-06-03 8:47 ` Tom de Vries 2020-06-03 9:38 ` Tom de Vries 2020-06-03 10:09 ` Tom de Vries 2020-06-03 10:24 ` Tom de Vries 2020-06-03 12:54 ` Andrew Burgess 2020-06-03 15:35 ` Tom de Vries 2020-06-04 11:16 ` Pedro Alves 2020-06-04 12:29 ` Tom de Vries 2020-06-12 13:11 ` [committed] gdb/testsuite: Prevent globals leaking between test scripts Tom de Vries 2020-06-03 9:49 ` [PATCH 3/3][gdb/testsuite] Warn about leaked global array Pedro Alves 2020-06-04 11:40 ` Tom de Vries 2020-06-05 10:06 ` [PATCH][gdb/testsuite] Don't leak tuiterm.exp spawn override Tom de Vries 2020-06-11 13:55 ` Tom Tromey 2020-06-12 11:36 ` [committed][gdb/testsuite] " Tom de Vries 2020-06-15 19:46 ` Tom Tromey 2020-06-17 14:55 ` Tom de Vries 2020-06-17 15:28 ` Andreas Schwab 2020-06-11 12:11 ` [committed][gdb/testsuite] Make gdb.base/dbx.exp more robust Tom de Vries 2020-06-11 12:16 ` Pedro Alves 2020-06-11 14:39 ` Simon Marchi 2020-06-11 14:52 ` Tom de Vries 2020-06-11 14:59 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).