bug#41824: Dejagnu's unknown proc aborts testsuite run when triggered in test-case

[Tom de Vries]

On 6/16/20 6:05 AM, Jacob Bachmeyer wrote:
> Tom de Vries wrote:
>> Hi,
>>
>> the gdb testsuite uses the dejagnu framework.
>>   
> 
> Apologies for the delay in responding; I took a few days to think about
> this issue because it requires a careful balance, as silently continuing
> after aborting a testcase could cause errors to go unnoticed in large
> testsuites.
> 
>> If I add a call to foobar at the end of a test-case in that testsuite,
>> and I do a run of the testsuite, then the test-case aborts, and
>> subsequently the testsuite run aborts, that is, no further tests are run.
>>
>> While it is as expected that the test-case aborts, it's not desirable
>> that the testsuite run aborts.
>>   
> 
> The main problem I have with this is that calling an unknown procedure
> is presumably a very serious error.  Aborting the entire run is a good
> way to ensure that the problem is noticed, but perhaps with a testsuite
> as large (and long-running) as the GDB testsuite has become, continuing
> may also be reasonable to salvage at least some results.
> 
> Where in the GDB testsuite is this a problem?  When is it appropriate to
> continue after such a serious error?

It's not a problem in a specific test-case, but a generic problem. If
somebody makes a typo in a test-case and checks it in, there's no need
to abort the test run.

> How to ensure that the error is
> not buried in the logs amidst the normal output from other tests and is
> actually noticed?
> 

I monitor ERROR and WARNING in my test runs, so I didn't think of this,
but indeed, those are not tracked in the summary, so that could be a
problem, agreed.

>> We've installed a hack in ${tool}_init/${tool}_finish to workaround this
>> in the gdb testsuite (
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=26783bce15adc0316f9167a216519cebcf1ccac3
>>
>> ).
>>
>> It would preferable though if this problem was addressed in dejagnu,
>> such that we can eventually remove the hack.
>>   
> 
> That hack (if not reverted entirely) needs to be made conditional on the
> actual existence of ::tcl_unknown as soon as possible -- the existence
> of ::tcl_unknown (kept to allow Tcl autoloading to work) is very much an
> internal implementation detail, and it *will* be moved out of the global
> namespace and eventually *will* cease to exist entirely in the
> interpreters that run testcases.  Really, tcl_unknown is not supposed to
> be there and relying on it introduces a latent bug into the GDB testsuite.
> 
> I am sorry, and I will seek to work with you to fix these problems with
> a minimum of breakage, but I have to put my foot down on this:  I
> *cannot* treat every internal symbol as long-term stable API.  That hack
> will *not* be supported long-term.  Any GDB releases that include it
> *will* break with some as-yet-undetermined future DejaGnu release.  I
> have to draw a line somewhere or I may as well declare DejaGnu
> unmaintained and frozen at 1.6.2 forever.  I have no inclination to do
> the latter.
> 

Ack, makes sense.

>> A possible solution to this problem could be to make the exiting on
>> error (which is done in dejagnu's version of proc unknown in
>> framework.exp) optional.
>>   
> 
> This is obviously a good solution, but leads to the obvious question of
> how (command-line option?  (Make normally aborts on error, but has a
> --keep-going option.)  configuration variable?) that should be
> determined, or if exit-on-unknown-procedure should ever be a default. 
> Could simply reporting an UNRESOLVED test (indicating that the testcase
> script aborted due to calling an undefined command) be a better option? 
> It would show up in the summary report at the end.
> 
> At first, I was planning to schedule this for the 1.7 series, but I
> checked and runtest.exp:runtest already catches Tcl errors, so fixing
> this does not require a significant change to the normal control flow. 
> Would simply reporting an "UNRESOLVED:  testcase $file aborted on
> unknown command '$what'" result, then propagating the Tcl error in the
> ::unknown proc be a workable solution?

I think so, yes.  I've tried that out in attached gdb patch, which also
solves the reliance on ::tcl_unknown.

Then when running into the unknown command in the test-case, we have in
gdb.sum:
...
Running
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp
...
PASS: gdb.ada/O2_float_param.exp: compilation foo.adb
PASS: gdb.ada/O2_float_param.exp: frame
UNRESOLVED: gdb.ada/O2_float_param.exp: testcase aborted on unknown
command 'foo'
ERROR: tcl error sourcing
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp.
ERROR: invalid command name "foo"
    while executing
"::gdb_unknown foo"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::gdb_unknown $args"
    (procedure "::unknown" line 4)
    invoked from within
"foo"
    (file
"/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
line 33)
    invoked from within
"source
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.ada/O2_float_param.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""

                === gdb Summary ===

# of expected passes            2
# of unresolved testcases       1
...

I get similar results if I disable the hack in the gdb testsuite, and
use attached runtest.exp demonstrator patch.

Note btw that in both cases I didn't put $file in the unresolved
message, because that's already present in pf_prefix, but that might not
always be true.

Thanks,
- Tom

fix

---
 gdb/testsuite/lib/gdb.exp | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f502eb157d..5cb9e7907c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5177,6 +5177,13 @@ proc gdb_cleanup_globals {} {
     }
 }
 
+# Create gdb_unknown, a copy tcl's ::unknown.
+set temp [interp create]
+set old_args [interp eval $temp "info args ::unknown"]
+set old_body [interp eval $temp "info body ::unknown"]
+interp delete $temp
+eval proc gdb_unknown {$old_args} {$old_body}
+
 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
@@ -5285,12 +5292,13 @@ proc gdb_init { test_file_name } {
 
     # 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.
+    # local version here, which instead calls unresolved, and then propagates 
the
+    # error to tcl's unknown. In gdb_finish, we restore dejagnu's version.
     rename ::unknown ::dejagnu_unknown
     proc unknown { args } {
-       # Dejagnu saves the original version in ::tcl_unknown, use it.
-       return [uplevel 1 ::tcl_unknown $args]
+       # Restore ::unknown.
+       unresolved "testcase aborted on unknown command '$args'"
+       return [uplevel 1 ::gdb_unknown $args]
     }
 
     return $res

--- runtest.exp.orig    2020-06-16 17:36:15.038208916 +0200
+++ runtest.exp 2020-06-16 17:38:19.189251033 +0200
@@ -1431,6 +1431,11 @@
     return $found
 }
 
+proc test_case_unknown { args } {
+    unresolved "testcase aborted on unknown command '$args'"
+    return [uplevel 1 ::tcl_unknown $args]
+}
+
 #
 # Source the testcase in TEST_FILE_NAME.
 #
@@ -1457,6 +1462,9 @@
            }
        }
 
+       rename ::unknown dejagnu_unknown
+       rename ::test_case_unknown ::unknown
+       
        if { [catch "uplevel #0 source $test_file_name"] == 1 } {
            # If we have a Tcl error, propagate the exit status so
            # that 'make' (if it invokes runtest) notices the error.
@@ -1476,6 +1484,9 @@
            }
        }
 
+       rename ::unknown ::test_case_unknown 
+       rename dejagnu_unknown ::unknown 
+
        if {[info exists tool]} {
            if { [info procs "${tool}_finish"] != "" } {
                ${tool}_finish