Re: [PATCH][gdb/testsuite] Don't abort testrun for invalid command in test-case

Tom de Vries
2020-06-11T16:25:54Z

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

On 11-06-2020 17:31, Pedro Alves wrote:
> On 6/11/20 3:35 PM, Tom de Vries wrote:
>> Hi,
>>
>> Say we add a call to foobar at the end of a test-case, and run the
>> test-suite. We'll run into a dejagnu error:
>> ...
>> ERROR: (DejaGnu) proc "foobar" does not exist.
>> ...
>> and the test-suite run is aborted.
>>
>> It's reasonable that the test-case is aborted, but it's not reasonable that
>> the testsuite run is aborted.
>>
>> Problems in one test-case should not leak into other test-cases, and they
>> generally don't. The exception is the "invalid command name" problem due to
>> an override of ::unknown in dejagnu's framework.exp.
>>
>> Fix this by limiting the scope of dejagnu's ::unknown override.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>
> We should ask the DejaGnu folks to make the exiting on error
> optional, so that in a future version we can drop the hack.
>

Agreed.

> What about, say, an error due to calling a tcl proc with the wrong
> arguments? "unknown" isn't called for that. Any idea how to
> catch those?
>

If I do:
...
diff --git a/gdb/testsuite/gdb.ada/O2_float_param.exp
b/gdb/testsuite/gdb.ada/O2_float_param.exp
index 09ebeec405..afbf6c5ace 100644
--- a/gdb/testsuite/gdb.ada/O2_float_param.exp
+++ b/gdb/testsuite/gdb.ada/O2_float_param.exp
@@ -29,3 +29,9 @@ runto "increment"

gdb_test "frame" \
"#0\\s+callee\\.increment \\(val(=val@entry)?=99\\.0,
msg=\\.\\.\\.\\).*"
+
+proc foo { a b c } {
+
+}
+
+foo
...
and run the test-suite, I get:
...
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp ...
ERROR: tcl error sourcing
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp.
ERROR: wrong # args: should be "foo a b c"
while executing
"foo"
(file
"/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
line 37)
invoked from within
"source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
("uplevel" body line 1)
invoked from within
"uplevel #0 source
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
invoked from within
"catch "uplevel #0 source $test_file_name""
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/access_tagged_param.exp
...
Running
/data/gdb_versions/devel/src/gdb/testsuite/gdb.ada/access_to_packed_array.exp
...
...
...

So, the error is caught in proc runtest:
...
if { [catch "uplevel #0 source $test_file_name"] == 1 } {
...

>> +# Save the unknown proc defined by dejagnu.
>> +rename ::unknown ::dejagnu_unknown
>> +
>> +# Override the unknown proc with a gdb-local version.
>> +proc unknown { args } {
>> + set script [info script]
>> +
>> + set script [file dirname $script]
>> + set subdir3 [file tail $script]
>> +
>> + set script [file dirname $script]
>> + set subdir2 [file tail $script]
>> +
>> + set script [file dirname $script]
>> + set subdir1 [file tail $script]
>> +
>> + if { $subdir1 == "gdb"
>> + && $subdir2 == "testsuite"
>> + && [regexp {^gdb[.]} $subdir3] } {
>> + # If we're executing a gdb test-case, skip dejagnu_unknown to prevent
>> + # it from exiting and aborting the entire test run.
>> + return [uplevel 1 ::tcl_unknown $args]
>> + }
>> +
>
> I'd think it would be cleaner to override unknown in gdb_init,
> and restore in gdb_finish. No need for filename matching that way.
> Like below. Any reason you didn't go for this instead?
>

I don't think it's cleaner, because we run default_gdb_init, as well the
bit of proc runtest between ${tool}_init a ${tool}_finish with the
altered ::unknown.

Alternatively, we could override source to detect the precise point that
runtest hands control to the test-case, as attached. But there's still
filename matching. [ Note btw, that this approach changes the scope of
the fix slightly. ]

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Don-t-abort-testrun-for-invalid-command-in-test-case.patch --]
[-- Type: text/x-patch, Size: 2035 bytes --]

[gdb/testsuite] Don't abort testrun for invalid command in test-case

Say we add a call to foobar at the end of a test-case, and run the
test-suite. We'll run into a dejagnu error:
...
ERROR: (DejaGnu) proc "foobar" does not exist.
...
and the test-suite run is aborted.

It's reasonable that the test-case is aborted, but it's not reasonable that
the testsuite run is aborted.

Problems in one test-case should not leak into other test-cases, and they
generally don't. The exception is the "invalid command name" problem due to
an override of ::unknown in dejagnu's framework.exp.

Fix this by limiting the scope of dejagnu's ::unknown override.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

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

PR testsuite/26110
* lib/gdb.exp (gdb_unknown): New proc.
(::tcl_source): Rename from ::source.
(source): New proc.

---
gdb/testsuite/lib/gdb.exp | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 51f8a05464..ae6d561a10 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7243,5 +7243,35 @@ proc with_override { name override body } {
return $result
}

+proc gdb_unknown { args } {
+ # Skip dejagnu_unknown to prevent it from exiting and aborting the entire
+ # test run.
+ return [uplevel 1 ::tcl_unknown $args]
+}
+
+rename ::source ::tcl_source
+
+proc source { arg } {
+ set script $arg
+
+ set script [file dirname $script]
+ set subdir3 [file tail $script]
+
+ set script [file dirname $script]
+ set subdir2 [file tail $script]
+
+ set script [file dirname $script]
+ set subdir1 [file tail $script]
+
+ if { $subdir1 == "gdb"
+ && $subdir2 == "testsuite"
+ && [regexp {^gdb[.]} $subdir3] } {
+ return \
+ [uplevel 1 with_override ::unknown gdb_unknown \"::tcl_source $arg\"]
+ }
+
+ return [uplevel 1 ::tcl_source $arg]
+}
+
# Always load compatibility stuff.
load_lib future.exp