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

[Tom de Vries <tdevries@suse.de>]

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