[PATCH][gdb/testsuite] Move code from gdb_init to default_gdb_init

Tom de Vries
2020-06-18T10:16:44Z

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

[ was: Re: [PATCH][gdb/testsuite] Don't abort testrun for invalid
command in test-case ]
On 6/17/20 4:19 PM, Pedro Alves wrote:
> On 6/17/20 3:14 PM, Tom de Vries wrote:
>> On 6/16/20 2:47 PM, Pedro Alves wrote:
>>> On 6/11/20 11:55 PM, Tom de Vries wrote:
>>>
>>>> Hmm, what is the distinction between gdb_init and default_gdb_init?
>>>>
>>>> All the other uses in gdb.exp of pattern foo/default_foo have an
>>>> implementation:
>>>> ...
>>>> proc foo {} {
>>>> [default_foo]
>>>> }
>>>> ...
>>>> but gdb_init is much more than that. Why is that different?
>>>>
>>>
>>> I don't know. I guess it shouldn't. I guess people (including me) added to
>>> gdb_init over time without realizing they were breaking the pattern. Maybe nobody
>>> notices it because whatever is overriding gdb_init renames the original one and
>>> then calls it.
>>
>> Hmm, thanks for the explanation. I feel we need to improve this
>> situation somehow, but I'm not sure yet how.
>
> Move stuff in gdb_init to default_gdb_init, and add a comment to these
> functions to not add stuff in there? That seems like the obvious.
> I'm not sure why we have that pattern in the first place though, given
> that you can rename/override functions in tcl anyhow. This all predates me.

How about this patch then?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Move-code-from-gdb_init-to-default_gdb_init.patch --]
[-- Type: text/x-patch, Size: 11031 bytes --]

[gdb/testsuite] Move code from gdb_init to default_gdb_init

If a baseboard file wants to override a proc foo, but also use the original
proc, it'll have to do something like:
...
rename foo save_foo
proc foo { } {
...
set res [save_foo]
...
return res
}
...
This adds a new proc named save_foo, which introduces the risk of clashing with
an existing proc.

There's a pattern in the gdb testsuite procs, that facilitates this override:
...
proc default_foo { } {
...
}

proc foo { } {
return [default_foo]
}
...
such that in a baseboard file we don't need the rename:
...
proc foo { } {
...
set res [default_foo]
...
return res
}
...

The exception to the pattern though is gdb_init, which has a default_gdb_init
counterpart, but contains much more code than just the call to
default_gdb_init.

Fix this by moving all but the call to default_gdb_init to default_gdb_init.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

* lib/gdb.exp (gdb_init): Move all but call to default_gdb_init to ...
(default_gdb_init): ... here.

---
gdb/testsuite/lib/gdb.exp | 246 ++++++++++++++++++++++++----------------------
1 file changed, 127 insertions(+), 119 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 02867fb5bd..7b7484c031 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4902,6 +4902,7 @@ proc gdb_continue { function } {
return [gdb_test "continue" ".*Breakpoint $decimal, $function .*" "continue to $function"]
}

+# Default implementation of gdb_init.
proc default_gdb_init { test_file_name } {
global gdb_wrapper_initialized
global gdb_wrapper_target
@@ -4909,6 +4910,107 @@ proc default_gdb_init { test_file_name } {
global cleanfiles
global pf_prefix

+ # 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.
+ global gdb_test_timeout
+ global timeout
+ set timeout $gdb_test_timeout
+
+ if { [regexp ".*gdb\.reverse\/.*" $test_file_name]
+ && [target_info exists gdb_reverse_timeout] } {
+ set timeout [target_info gdb_reverse_timeout]
+ }
+
+ # If GDB_INOTIFY is given, check for writes to '.'. This is a
+ # debugging tool to help confirm that the test suite is
+ # parallel-safe. You need "inotifywait" from the
+ # inotify-tools package to use this.
+ global GDB_INOTIFY inotify_pid
+ if {[info exists GDB_INOTIFY] && ![info exists inotify_pid]} {
+ global outdir tool inotify_log_file
+
+ set exclusions {outputs temp gdb[.](log|sum) cache}
+ set exclusion_re ([join $exclusions |])
+
+ set inotify_log_file [standard_temp_file inotify.out]
+ set inotify_pid [exec inotifywait -r -m -e move,create,delete . \
+ --exclude $exclusion_re \
+ |& tee -a $outdir/$tool.log $inotify_log_file &]
+
+ # Wait for the watches; hopefully this is long enough.
+ sleep 2
+
+ # Clear the log so that we don't emit a warning the first time
+ # we check it.
+ set fd [open $inotify_log_file w]
+ close $fd
+ }
+
+ # Block writes to all banned variables, and invocation of all
+ # banned procedures...
+ global banned_variables
+ global banned_procedures
+ global banned_traced
+ if (!$banned_traced) {
+ foreach banned_var $banned_variables {
+ global "$banned_var"
+ trace add variable "$banned_var" write error
+ }
+ foreach banned_proc $banned_procedures {
+ global "$banned_proc"
+ trace add execution "$banned_proc" enter error
+ }
+ set banned_traced 1
+ }
+
+ # We set LC_ALL, LC_CTYPE, and LANG to C so that we get the same
+ # messages as expected.
+ setenv LC_ALL C
+ setenv LC_CTYPE C
+ setenv LANG C
+
+ # Don't let a .inputrc file or an existing setting of INPUTRC mess up
+ # the test results. Even if /dev/null doesn't exist on the particular
+ # platform, the readline library will use the default setting just by
+ # failing to open the file. OTOH, opening /dev/null successfully will
+ # also result in the default settings being used since nothing will be
+ # read from this file.
+ setenv INPUTRC "/dev/null"
+
+ # This disables style output, which would interfere with many
+ # tests.
+ setenv TERM "dumb"
+
+ # Ensure that GDBHISTFILE and GDBHISTSIZE are removed from the
+ # environment, we don't want these modifications to the history
+ # settings.
+ unset -nocomplain ::env(GDBHISTFILE)
+ unset -nocomplain ::env(GDBHISTSIZE)
+
+ # Initialize GDB's pty with a fixed size, to make sure we avoid pagination
+ # during startup. See "man expect" for details about stty_init.
+ global stty_init
+ set stty_init "rows 25 cols 80"
+
+ # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
+ # grep. Clear GREP_OPTIONS to make the behavior predictable,
+ # especially having color output turned on can cause tests to fail.
+ setenv GREP_OPTIONS ""
+
+ # Clear $gdbserver_reconnect_p.
+ global gdbserver_reconnect_p
+ set gdbserver_reconnect_p 1
+ unset gdbserver_reconnect_p
+
+ # Clear $last_loaded_file
+ global last_loaded_file
+ unset -nocomplain last_loaded_file
+
+ # Reset GDB number of instances
+ global gdb_instances
+ set gdb_instances 0
+
set cleanfiles {}

gdb_clear_suppressed
@@ -4942,6 +5044,20 @@ proc default_gdb_init { test_file_name } {
if [info exists use_gdb_stub] {
unset use_gdb_stub
}
+
+ gdb_setup_known_globals
+
+ if { [info procs ::gdb_tcl_unknown] != "" } {
+ # 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
+ # dejagnu's version in gdb_finish.
+ rename ::unknown ::dejagnu_unknown
+ proc unknown { args } {
+ # Use tcl's unknown.
+ return [uplevel 1 ::gdb_tcl_unknown $args]
+ }
+ }
}

# Return a path using GDB_PARALLEL.
@@ -5188,127 +5304,19 @@ if { [interp eval $temp "info procs ::unknown"] != "" } {
interp delete $temp
unset temp

-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
- # the timeout used in subsequent testcases.
- global gdb_test_timeout
- global timeout
- set timeout $gdb_test_timeout
-
- if { [regexp ".*gdb\.reverse\/.*" $test_file_name]
- && [target_info exists gdb_reverse_timeout] } {
- set timeout [target_info gdb_reverse_timeout]
- }
-
- # If GDB_INOTIFY is given, check for writes to '.'. This is a
- # debugging tool to help confirm that the test suite is
- # parallel-safe. You need "inotifywait" from the
- # inotify-tools package to use this.
- global GDB_INOTIFY inotify_pid
- if {[info exists GDB_INOTIFY] && ![info exists inotify_pid]} {
- global outdir tool inotify_log_file
-
- set exclusions {outputs temp gdb[.](log|sum) cache}
- set exclusion_re ([join $exclusions |])
-
- set inotify_log_file [standard_temp_file inotify.out]
- set inotify_pid [exec inotifywait -r -m -e move,create,delete . \
- --exclude $exclusion_re \
- |& tee -a $outdir/$tool.log $inotify_log_file &]
-
- # Wait for the watches; hopefully this is long enough.
- sleep 2
-
- # Clear the log so that we don't emit a warning the first time
- # we check it.
- set fd [open $inotify_log_file w]
- close $fd
- }
-
- # Block writes to all banned variables, and invocation of all
- # banned procedures...
- global banned_variables
- global banned_procedures
- global banned_traced
- if (!$banned_traced) {
- foreach banned_var $banned_variables {
- global "$banned_var"
- trace add variable "$banned_var" write error
- }
- foreach banned_proc $banned_procedures {
- global "$banned_proc"
- trace add execution "$banned_proc" enter error
- }
- set banned_traced 1
- }
-
- # We set LC_ALL, LC_CTYPE, and LANG to C so that we get the same
- # messages as expected.
- setenv LC_ALL C
- setenv LC_CTYPE C
- setenv LANG C
-
- # Don't let a .inputrc file or an existing setting of INPUTRC mess up
- # the test results. Even if /dev/null doesn't exist on the particular
- # platform, the readline library will use the default setting just by
- # failing to open the file. OTOH, opening /dev/null successfully will
- # also result in the default settings being used since nothing will be
- # read from this file.
- setenv INPUTRC "/dev/null"
-
- # This disables style output, which would interfere with many
- # tests.
- setenv TERM "dumb"
-
- # Ensure that GDBHISTFILE and GDBHISTSIZE are removed from the
- # environment, we don't want these modifications to the history
- # settings.
- unset -nocomplain ::env(GDBHISTFILE)
- unset -nocomplain ::env(GDBHISTSIZE)
-
- # Initialize GDB's pty with a fixed size, to make sure we avoid pagination
- # during startup. See "man expect" for details about stty_init.
- global stty_init
- set stty_init "rows 25 cols 80"
-
- # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
- # grep. Clear GREP_OPTIONS to make the behavior predictable,
- # especially having color output turned on can cause tests to fail.
- setenv GREP_OPTIONS ""
-
- # Clear $gdbserver_reconnect_p.
- global gdbserver_reconnect_p
- set gdbserver_reconnect_p 1
- unset gdbserver_reconnect_p
-
- # Clear $last_loaded_file
- global last_loaded_file
- unset -nocomplain last_loaded_file
-
- # Reset GDB number of instances
- global gdb_instances
- set gdb_instances 0
-
- set res [default_gdb_init $test_file_name]
-
- gdb_setup_known_globals
-
- if { [info procs ::gdb_tcl_unknown] != "" } {
- # 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
- # dejagnu's version in gdb_finish.
- rename ::unknown ::dejagnu_unknown
- proc unknown { args } {
- # Use tcl's unknown.
- return [uplevel 1 ::gdb_tcl_unknown $args]
- }
- }
-
- return $res
+# Gdb implementation of ${tool}_init. Called right before executing the
+# test-case.
+# Overridable function -- you can override this function in your
+# baseboard file.
+proc gdb_init { args } {
+ # A baseboard file overriding this proc and calling the default version
+ # should behave the same as this proc. So, don't add code here, but to
+ # the default version instead.
+ return [default_gdb_init {*}$args]
}

+# Gdb implementation of ${tool}_finish. Called right after executing the
+# test-case.
proc gdb_finish { } {
global gdbserver_reconnect_p
global gdb_prompt