[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Tom de Vries
Subject: bug#41824: Dejagnu's unknown proc aborts testsuite run when triggered in test-case
Date: Tue, 16 Jun 2020 17:53:54 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

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
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
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
line 33)
    invoked from within
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
    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.

- Tom

 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 
+    # 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"] != "" } {

reply via email to

[Prev in Thread] Current Thread [Next in Thread]