Package: dejagnu;
Reported by: Tom de Vries <tdevries <at> suse.de>
Date: Fri, 12 Jun 2020 08:36:01 UTC
Owned by: jcb62281 <at> gmail.com
Severity: normal
Done: Jacob Bachmeyer <jcb62281 <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Tom de Vries <tdevries <at> suse.de> To: jcb62281 <at> gmail.com Cc: 41824 <at> debbugs.gnu.org Subject: bug#41824: Dejagnu's unknown proc aborts testsuite run when triggered in test-case Date: Tue, 16 Jun 2020 17:53:54 +0200
[Message part 1 (text/plain, inline)]
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
[0001-fix.patch (text/x-patch, inline)]
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.patch (text/x-patch, inline)]
--- 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.