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