GNU bug report logs - #41918
[PATCH] Propagate error value of auto-loaded command

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41918 in the body.
You can then email your comments to 41918 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tom de Vries <tdevries <at> suse.de>
To: bug-dejagnu <at> gnu.org
Subject: [PATCH] Propagate error value of auto-loaded command
Date: Wed, 17 Jun 2020 14:34:14 +0200

[Message part 1 (text/plain, inline)]

Hi,

I think I found a bug in proc unknown in lib/framework.exp.

Patch describing the problem and fixing it attached below.

Thanks,
- Tom

[0002-Propagate-error-value-of-auto-loaded-command.patch (text/x-patch, inline)]

Propagate error value of auto-loaded command

Consider a library file foo.tcl:
...
proc foo { } {
    throw {ARITH DIVZERO {divide by zero}} {divide by zero}
}
...
and a test-case test.tcl:
...
\#!/usr/bin/tclsh

auto_mkindex lib *.tcl

lappend auto_path [pwd]/lib

foo
...
which gives us:
...
divide by zero
    while executing
"throw {ARITH DIVZERO {divide by zero}} {divide by zero}"
    (procedure "foo" line 2)
    invoked from within
"foo"
    (file "./test.tcl" line 7)
...

When overriding the ::unknown command using:
...
rename ::unknown ::tcl_unknown
proc unknown args {
    if {[catch {uplevel 1 ::tcl_unknown $args} msg]} {
        puts "ERROR: proc \"$args\" does not exist: $msg"
        exit
    } else {
        return $msg
    }
}
...
we have instead:
...
$ ./test.tcl
ERROR: proc "foo" does not exist: divide by zero
...

This can be fixed by testing for the specific error code, and otherwise
propagating the error:
...
proc unknown args {
    set code [catch {uplevel 1 ::tcl_unknown $args} msg]
    if { $code == 1 } {
        global errorInfo errorCode
        if { [lindex errorCode 0] eq "TCL"
             && [lindex errorCode 1] eq "LOOKUP"
             && [lindex errorCode 2] eq "COMMAND"
             && [lindex errorCode 3] eq [lindex $args 0] } {
            puts "ERROR: proc \"$args\" does not exist: $msg"
            exit
        }

        return -code error -errorinfo $errorInfo -errorcode $errorCode $msg
    }

    return -code $code $msg
}
...

Fix unknown in lib/framework.exp accordingly.

ChangeLog:

2020-06-17  Tom de Vries  <tdevries <at> suse.de>

	* lib/framework.exp (unknown): Propagate error value of auto-loaded
	command.

---
 lib/framework.exp | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/lib/framework.exp b/lib/framework.exp
index c9875d2..1347cc1 100644
--- a/lib/framework.exp
+++ b/lib/framework.exp
@@ -258,24 +258,32 @@ proc isnative { } {
 
 rename ::unknown ::tcl_unknown
 proc unknown args {
-    if {[catch {uplevel 1 ::tcl_unknown $args} msg]} {
+    set code [catch {uplevel 1 ::tcl_unknown $args} msg]
+    if { $code  == 1 } {
 	global errorCode
 	global errorInfo
 	global exit_status
-
-	clone_output "ERROR: (DejaGnu) proc \"$args\" does not exist."
-	if {[info exists errorCode]} {
-	    send_error "The error code is $errorCode\n"
-	}
-	if {[info exists errorInfo]} {
-	    send_error "The info on the error is:\n$errorInfo\n"
+	if { [lindex errorCode 0] eq "TCL"
+	     && [lindex errorCode 1] eq "LOOKUP"
+	     && [lindex errorCode 2] eq "COMMAND"
+	     && [lindex errorCode 3] eq [lindex $args 0] } {
+	    clone_output "ERROR: (DejaGnu) proc \"$args\" does not exist."
+	    if {[info exists errorCode]} {
+		send_error "The error code is $errorCode\n"
+	    }
+	    if {[info exists errorInfo]} {
+		send_error "The info on the error is:\n$errorInfo\n"
+	    }
+	    set exit_status 2
+	    log_and_exit
 	}
-	set exit_status 2
-	log_and_exit
-    } else {
-	# Propagate return value.
-	return $msg
+
+	# Propagate error
+	return -code error -errorinfo $errorInfo -errorcode $errorCode $msg
     }
+
+    # Propagate return value.
+    return -code $code $msg
 }
 
 # Print output to stdout (or stderr) and to log file

Message #8 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Jacob Bachmeyer <jcb62281 <at> gmail.com>
To: Tom de Vries <tdevries <at> suse.de>
Cc: 41918 <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Wed, 17 Jun 2020 18:19:32 -0500

Tom de Vries wrote:
> Hi,
>
> I think I found a bug in proc unknown in lib/framework.exp.
>
> Patch describing the problem and fixing it attached below.
>   
I found a similar issue while patching bug #41824; please check whether 
that patch addresses this issue adequately.

-- Jacob

Message #11 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Tom de Vries <tdevries <at> suse.de>
To: jcb62281 <at> gmail.com
Cc: 41918 <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Thu, 18 Jun 2020 09:00:03 +0200

[Message part 1 (text/plain, inline)]

On 6/18/20 1:19 AM, Jacob Bachmeyer wrote:
> Tom de Vries wrote:
>> Hi,
>>
>> I think I found a bug in proc unknown in lib/framework.exp.
>>
>> Patch describing the problem and fixing it attached below.
>>   
> I found a similar issue while patching bug #41824; please check whether
> that patch addresses this issue adequately.

AFAICT, it does not.  Dejagnu test-case attached.

Thanks,
- Tom

[0002-Add-abort-throw.exp.patch (text/x-patch, inline)]

Add abort-throw.exp

Todo: Don't create a tclIndex in the source dir.

runtest.log:
...
Running abort-throw.exp ...
PASS: running abort-throw.exp
UNRESOLVED: testcase 'abort-throw.exp' aborted at call to unknown command 'foo'

                ===  Summary ===

\# of expected passes            1
\# of unresolved testcases       1
WARNING: No tool specified
WARNING: Couldn't find tool config file for unix, using default.
ERROR: (DejaGnu) proc "foo" does not exist.
The error code is ARITH DIVZERO {divide by zero}
The info on the error is:
divide by zero
    while executing
"throw {ARITH DIVZERO {divide by zero}} {divide by zero}"
    (procedure "foo" line 2)
    invoked from within
"::tcl_unknown foo"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::tcl_unknown $args"
FAIL: continue after throw
...

---
 testsuite/runtest.main/abort.exp                   |  5 ++++
 .../abort/testsuite/abort.test/abort-throw.exp     | 31 ++++++++++++++++++++++
 .../abort/testsuite/abort.test/lib/foo.tcl         |  3 +++
 3 files changed, 39 insertions(+)

diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
index c5f7014..c65d539 100644
--- a/testsuite/runtest.main/abort.exp
+++ b/testsuite/runtest.main/abort.exp
@@ -50,6 +50,11 @@ set tests {
 	"PASS: running abort-undef.exp.*\
 	*UNRESOLVED: .* aborted at call to unknown command.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "continue after throw"
+	"abort-throw.exp simple.exp"
+	"PASS: running abort-throw.exp.*\
+	*PASS: simple test.*\
+	*expected passes\[ \t\]+2\n" }
     { "stop at abort without --keep_going"
 	"abort-undef.exp simple.exp"
 	"PASS: running abort-undef.exp.*\
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/abort-throw.exp b/testsuite/runtest.main/abort/testsuite/abort.test/abort-throw.exp
new file mode 100644
index 0000000..3e7c1f3
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/abort-throw.exp
@@ -0,0 +1,31 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# DejaGnu is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# Invoke a divide-by-zero command, in an auto-loaded proc.
+
+pass "running abort-throw.exp"
+
+file delete $srcdir/$subdir/tclIndex
+
+auto_mkindex $srcdir/$subdir lib/*.tcl
+
+lappend auto_path $srcdir/$subdir
+
+foo
+
+fail "script did not abort"
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/lib/foo.tcl b/testsuite/runtest.main/abort/testsuite/abort.test/lib/foo.tcl
new file mode 100644
index 0000000..d623172
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/lib/foo.tcl
@@ -0,0 +1,3 @@
+proc foo { } {
+    throw {ARITH DIVZERO {divide by zero}} {divide by zero}
+}

Message #14 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Jacob Bachmeyer <jcb62281 <at> gmail.com>
To: Tom de Vries <tdevries <at> suse.de>
Cc: 41918 <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Thu, 18 Jun 2020 19:00:18 -0500

[Message part 1 (text/plain, inline)]

Tom de Vries wrote:
> On 6/18/20 1:19 AM, Jacob Bachmeyer wrote:
>   
>> Tom de Vries wrote:
>>     
>>> Hi,
>>>
>>> I think I found a bug in proc unknown in lib/framework.exp.
>>>
>>> Patch describing the problem and fixing it attached below.
>>>   
>>>       
>> I found a similar issue while patching bug #41824; please check whether
>> that patch addresses this issue adequately.
>>     
>
> AFAICT, it does not.  Dejagnu test-case attached.
>   

That test is incorrect:  it does not specify the --keep_going option, 
but expects runtest to continue after a test script aborts with a Tcl error.
I have attached a fixed patch (0004) that avoids modifying the source 
tree by setting up autoloading in the build tree.

I have also attached another patch (0005) that fixes an inconsistency in 
DejaGnu's handling of Tcl errors in test scripts.  Previously, DejaGnu 
would abort the test run if an undefined procedure is called, but would 
only issue an ERROR message and continue with the next test script for 
all other Tcl errors.  This patch solves that by ensuring that DejaGnu 
will abort on any uncaught Tcl error in a test script unless the 
--keep_going option is given on the command line.  An UNRESOLVED result 
appears in the log in any case if a test script aborts due to an error.

Note that --keep_going is *not* expected to be normally used.  The 
--keep_going option "papers over" serious errors in the testsuite, and a 
testsuite that produces Tcl errors is broken, period.  This is the 
reason that there is no supported means for a testsuite to set --keep_going.


-- Jacob

[0004-Add-tests-for-handling-arithmetic-errors-in-auto-loa.patch (text/plain, inline)]

From cbba4dbb8d52c5b0f32e803cf8587f276ee1ec86 Mon Sep 17 00:00:00 2001
From: Jacob Bachmeyer <jcb62281+dev <at> gmail.com>
Date: Thu, 18 Jun 2020 17:32:48 -0500
Subject: [PATCH 4/5] Add tests for handling arithmetic errors in auto-loaded procedures

---
 ChangeLog                                          |   14 +++++++
 testsuite/runtest.main/abort.exp                   |   11 ++++++
 .../abort/testsuite/abort.test/abort-al-dbz.exp    |   38 ++++++++++++++++++++
 3 files changed, 63 insertions(+), 0 deletions(-)
 create mode 100644 testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp

diff --git a/ChangeLog b/ChangeLog
index 17bb158..f60a023 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2020-06-18  Jacob Bachmeyer  <jcb62281+dev <at> gmail.com>
+
+	PR 41824 / PR 41918
+
+	Thanks to Tom de Vries for raising these concerns and offering the
+	initial patch that was rewritten to produce this.
+
+	* testsuite/runtest.main/abort.exp: Add tests to verify handling
+	of arithmetic errors (divide-by-zero) in an auto-loaded procedure
+	called from a test script.
+
+	* testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp:
+	New file.
+
 2020-06-17  Jacob Bachmeyer  <jcb62281+dev <at> gmail.com>
 
 	PR 41824
diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
index c5f7014..864f1e0 100644
--- a/testsuite/runtest.main/abort.exp
+++ b/testsuite/runtest.main/abort.exp
@@ -50,6 +50,16 @@ set tests {
 	"PASS: running abort-undef.exp.*\
 	*UNRESOLVED: .* aborted at call to unknown command.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "stop at auto-loaded divide-by-zero without --keep_going"
+	"abort-al-dbz.exp simple.exp"
+	"PASS: running abort-al-dbz.exp.*\
+	*UNRESOLVED: .* aborted at .*\
+	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "continue after auto-loaded divide-by-zero with --keep_going"
+	"--keep_going abort-al-dbz.exp simple.exp"
+	"PASS: running abort-al-dbz.exp.*\
+	*PASS: simple test.*\
+	*expected passes\[ \t\]+2\n" }
     { "stop at abort without --keep_going"
 	"abort-undef.exp simple.exp"
 	"PASS: running abort-undef.exp.*\
@@ -76,3 +86,4 @@ foreach t $tests {
 }
 
 file delete -force $tmpdir
+file delete -force [testsuite file -object -test abort testsuite abort.test lib]
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp b/testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp
new file mode 100644
index 0000000..df55a9a
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# DejaGnu is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# Cause a divide-by-zero error in an auto-loaded procedure.
+
+pass "running abort-al-dbz.exp"
+
+set fd [open [testsuite file -object -test lib foo.tcl] w]
+puts $fd {proc throw_arith_error_div_by_zero { } {
+    expr { 1 / 0 }
+}
+}
+close $fd
+
+auto_mkindex \
+    [testsuite file -object -test lib] \
+    [testsuite file -object -test lib/*.tcl]
+
+lappend auto_path [testsuite file -object -test lib]
+
+throw_arith_error_div_by_zero
+
+fail "script did not abort"
-- 
1.7.4.1

[0005-Use-consistent-behavior-for-Tcl-errors-in-test-scrip.patch (text/plain, inline)]

From d45310cd257d399b8208fa9907f7c9f2f4ac7eda Mon Sep 17 00:00:00 2001
From: Jacob Bachmeyer <jcb62281+dev <at> gmail.com>
Date: Thu, 18 Jun 2020 18:52:33 -0500
Subject: [PATCH 5/5] Use consistent behavior for Tcl errors in test scripts

---
 ChangeLog                        |   15 ++++++++++-
 NEWS                             |    3 ++
 lib/framework.exp                |   46 +++++++++++++++++++++----------------
 runtest.exp                      |    6 +++++
 testsuite/runtest.main/abort.exp |    9 ++++---
 5 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f60a023..e9f1664 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,11 +3,22 @@
 	PR 41824 / PR 41918
 
 	Thanks to Tom de Vries for raising these concerns and offering the
-	initial patch that was rewritten to produce this.
+	initial testsuite patch that led to these changes.
+
+	* NEWS: Add item for consistent abort-on-error handling.
+
+	* lib/framework.exp (unknown): Always link global variables.  Tidy.
+	Silently propagate errors raised in autoloaded procedures and move
+	the UNRESOLVED result and aborting the test run to...
+	* runtest.exp (runtest): Report an UNRESOLVED result if a test
+	script aborts due to a Tcl error.  Link global errorCode and
+	report its value if an error occurs.  For consistency, abort the
+	test run on any Tcl error in a test script instead of only when
+	calling an undefined procedure.
 
 	* testsuite/runtest.main/abort.exp: Add tests to verify handling
 	of arithmetic errors (divide-by-zero) in an auto-loaded procedure
-	called from a test script.
+	called from a test script.  Adjust other patterns.
 
 	* testsuite/runtest.main/abort/testsuite/abort.test/abort-al-dbz.exp:
 	New file.
diff --git a/NEWS b/NEWS
index 4354422..619f0ff 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,9 @@ Changes since 1.6.2:
    the default of reading "site.exp".  See the manual for details.
 X. runtest now accepts a --keep_going option to continue with other test
    scripts after a test script invokes an undefined command.
+X. Unless the --keep_going option is used, runtest now aborts if a test
+   script fails with any Tcl error.  Previously, only calling an undefined
+   procedure would cause the test run to abort.
 3. A utility procedure relative_filename has been added.  This procedure
    computes a relative file name to a given destination from a given base.
 4. The utility procedure 'grep' now accepts a '-n' option that
diff --git a/lib/framework.exp b/lib/framework.exp
index db6e661..0850595 100644
--- a/lib/framework.exp
+++ b/lib/framework.exp
@@ -258,36 +258,42 @@ proc isnative { } {
 
 rename ::unknown ::tcl_unknown
 proc unknown { args } {
+    global errorCode
+    global errorInfo
+    global exit_status
+
     set code [catch {uplevel 1 ::tcl_unknown $args} msg]
     if { $code != 0 } {
-	global errorCode
-	global errorInfo
-	global exit_status
-
 	set ret_cmd [list return -code $code]
 
-	clone_output "ERROR: (DejaGnu) proc \"$args\" does not exist."
-	if {[info exists errorCode]} {
+	# If the command now exists, then it was autoloaded.  We are here,
+	# therefore invoking the autoloaded command raised an error.
+	# Silently propagate errors from autoloaded procedures, but
+	# complain noisily about undefined commands.
+	set have_it_now [llength [info commands [lindex $args 0]]]
+
+	if { ! $have_it_now } {
+	    clone_output "ERROR: (DejaGnu) proc \"$args\" does not exist."
+	    set exit_status 2
+	}
+
+	if { [info exists errorCode] } {
 	    lappend ret_cmd -errorcode $errorCode
-	    send_error "The error code is $errorCode\n"
+	    if { ! $have_it_now } {
+		send_error "The error code is $errorCode\n"
+	    }
 	}
-	if {[info exists errorInfo]} {
-	    # omitting errorInfo from the propagated error makes this code
+	if { [info exists errorInfo] } {
+	    # omitting errorInfo from the propagated error makes this proc
 	    # invisible with the backtrace pointing directly to the problem
-	    send_error "The info on the error is:\n$errorInfo\n"
+	    if { ! $have_it_now } {
+		send_error "The info on the error is:\n$errorInfo\n"
+	    }
 	}
-	set exit_status 2
-
-	set unresolved_msg "testcase '[uplevel info script]' aborted"
-	append unresolved_msg " at call to unknown command '$args'"
-	unresolved $unresolved_msg
 
 	lappend ret_cmd $msg
-	if { $::dejagnu::opt::keep_going } {
-	    eval $ret_cmd
-	} else {
-	    log_and_exit
-	}
+
+	eval $ret_cmd
     } else {
 	# Propagate return value.
 	return $msg
diff --git a/runtest.exp b/runtest.exp
index 028ad5b..245c536 100644
--- a/runtest.exp
+++ b/runtest.exp
@@ -1562,6 +1562,7 @@ proc runtest { test_file_name } {
     global bug_id
     global test_result
     global errcnt
+    global errorCode
     global errorInfo
     global tool
     global testdir
@@ -1596,10 +1597,15 @@ proc runtest { test_file_name } {
 	    # increments `errcnt'.  If we do call perror we'd have to
 	    # reset errcnt afterwards.
 	    clone_output "ERROR: tcl error sourcing $test_file_name."
+	    if {[info exists errorCode]} {
+		clone_output "ERROR: tcl error code $errorCode"
+	    }
 	    if {[info exists errorInfo]} {
 		clone_output "ERROR: $errorInfo"
 		unset errorInfo
 	    }
+	    unresolved "testcase '$test_file_name' aborted due to Tcl error"
+	    if { ! $::dejagnu::opt::keep_going } { log_and_exit }
 	}
 
 	if {[info exists tool]} {
diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
index 864f1e0..b352b56 100644
--- a/testsuite/runtest.main/abort.exp
+++ b/testsuite/runtest.main/abort.exp
@@ -48,27 +48,28 @@ set tests {
     { "abort on undefined command"
 	"abort-undef.exp"
 	"PASS: running abort-undef.exp.*\
-	*UNRESOLVED: .* aborted at call to unknown command.*\
+	*UNRESOLVED: .* aborted.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
     { "stop at auto-loaded divide-by-zero without --keep_going"
 	"abort-al-dbz.exp simple.exp"
 	"PASS: running abort-al-dbz.exp.*\
-	*UNRESOLVED: .* aborted at .*\
+	*UNRESOLVED: .* aborted.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
     { "continue after auto-loaded divide-by-zero with --keep_going"
 	"--keep_going abort-al-dbz.exp simple.exp"
 	"PASS: running abort-al-dbz.exp.*\
+	*UNRESOLVED: .* aborted.*\
 	*PASS: simple test.*\
 	*expected passes\[ \t\]+2\n" }
     { "stop at abort without --keep_going"
 	"abort-undef.exp simple.exp"
 	"PASS: running abort-undef.exp.*\
-	*UNRESOLVED: .* aborted at call to unknown command.*\
+	*UNRESOLVED: .* aborted.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
     { "continue after abort with --keep_going"
 	"--keep_going abort-undef.exp simple.exp"
 	"PASS: running abort-undef.exp.*\
-	*UNRESOLVED: .* aborted at call to unknown command.*\
+	*UNRESOLVED: .* aborted.*\
 	*PASS: simple test.*\
 	*expected passes\[ \t\]+2\n.*unresolved testcases\[ \t\]+1\n" }
 }
-- 
1.7.4.1

Message #17 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Tom de Vries <tdevries <at> suse.de>
To: jcb62281 <at> gmail.com
Cc: 41918 <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Fri, 19 Jun 2020 14:36:58 +0200

[Message part 1 (text/plain, inline)]

On 6/19/20 2:00 AM, Jacob Bachmeyer wrote:
> Tom de Vries wrote:
>> On 6/18/20 1:19 AM, Jacob Bachmeyer wrote:
>>  
>>> Tom de Vries wrote:
>>>    
>>>> Hi,
>>>>
>>>> I think I found a bug in proc unknown in lib/framework.exp.
>>>>
>>>> Patch describing the problem and fixing it attached below.
>>>>         
>>> I found a similar issue while patching bug #41824; please check whether
>>> that patch addresses this issue adequately.
>>>     
>>
>> AFAICT, it does not.  Dejagnu test-case attached.
>>   
> 
> That test is incorrect:  it does not specify the --keep_going option,
> but expects runtest to continue after a test script aborts with a Tcl
> error.

Well, yes, it's respecting the difference in handling of proc-not-found
vs other tcl errors that is present in current dejagnu.

Since:
- your implementation of --keep_going did not modify this, and
- your description of keep-going in NEWS explictly respected that
  difference,
I had no reason to deviate from that.

Now that you've decided to change the handling of proc-not-found vs
other tcl errors in patch 0005, obviously the test-case needs to be
updated to accomodate for that.

> I have attached a fixed patch (0004) that avoids modifying the source
> tree by setting up autoloading in the build tree.
> 

Ack, thanks.

> I have also attached another patch (0005) that fixes an inconsistency in
> DejaGnu's handling of Tcl errors in test scripts.  Previously, DejaGnu
> would abort the test run if an undefined procedure is called, but would
> only issue an ERROR message and continue with the next test script for
> all other Tcl errors.  This patch solves that by ensuring that DejaGnu
> will abort on any uncaught Tcl error in a test script unless the
> --keep_going option is given on the command line.  An UNRESOLVED result
> appears in the log in any case if a test script aborts due to an error.
> 

I couldn't apply this cleanly, so instead I'm now looking at branch PR41918.

I see that for abort-undef.exp we have:
...
ERROR: (DejaGnu) proc "bogus_command 1 2 3 4" does not exist.
The error code is TCL LOOKUP COMMAND bogus_command
The info on the error is:
invalid command name "bogus_command"
    while executing
"::tcl_unknown bogus_command 1 2 3 4"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::tcl_unknown $args"
ERROR: tcl error sourcing
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp.
ERROR: tcl error code TCL LOOKUP COMMAND bogus_command
ERROR: invalid command name "bogus_command"
    while executing
"bogus_command 1 2 3 4"
    (file
"/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
line 23)
    invoked from within
"source
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
...

Should we error twice on this? FWIW, just removing ::unknown seems to be
the easiest way to fix that.

Anyway, I have two patches attached:
- one that rewords the NEWS entries into something more intuitive for me
- one that adds a test-case abort-dbz.exp, a copy of
  abort-al-dbz.exp, but without the auto-load bit.

Thanks,
- Tom

[0007-Reword-NEWS-entries.patch (text/x-patch, inline)]

Reword NEWS entries

---
 NEWS | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 619f0ff..abae60b 100644
--- a/NEWS
+++ b/NEWS
@@ -7,11 +7,10 @@ Changes since 1.6.2:
    should use this proc. The 'is_remote' proc is deprecated.
 2. runtest now accepts --local_init and --global_init options to override
    the default of reading "site.exp".  See the manual for details.
+X. runtest now aborts if a test script fails with any Tcl error.  Previously,
+   only calling an undefined procedure would cause the test run to abort.
 X. runtest now accepts a --keep_going option to continue with other test
-   scripts after a test script invokes an undefined command.
-X. Unless the --keep_going option is used, runtest now aborts if a test
-   script fails with any Tcl error.  Previously, only calling an undefined
-   procedure would cause the test run to abort.
+   scripts after a test script fails with a Tcl error.
 3. A utility procedure relative_filename has been added.  This procedure
    computes a relative file name to a given destination from a given base.
 4. The utility procedure 'grep' now accepts a '-n' option that

[0008-Add-abort.test-abort-dbz.exp.patch (text/x-patch, inline)]

Add abort.test/abort-dbz.exp

---
 testsuite/runtest.main/abort.exp                   | 11 ++++++++
 .../abort/testsuite/abort.test/abort-dbz.exp       | 29 ++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/testsuite/runtest.main/abort.exp b/testsuite/runtest.main/abort.exp
index b352b56..59a934c 100644
--- a/testsuite/runtest.main/abort.exp
+++ b/testsuite/runtest.main/abort.exp
@@ -50,6 +50,17 @@ set tests {
 	"PASS: running abort-undef.exp.*\
 	*UNRESOLVED: .* aborted.*\
 	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "stop at divide-by-zero without --keep_going"
+	"abort-dbz.exp simple.exp"
+	"PASS: running abort-dbz.exp.*\
+	*UNRESOLVED: .* aborted.*\
+	*expected passes\[ \t\]+1\n.*unresolved testcases\[ \t\]+1\n" }
+    { "continue after divide-by-zero with --keep_going"
+	"--keep_going abort-dbz.exp simple.exp"
+	"PASS: running abort-dbz.exp.*\
+	*UNRESOLVED: .* aborted.*\
+	*PASS: simple test.*\
+	*expected passes\[ \t\]+2\n" }
     { "stop at auto-loaded divide-by-zero without --keep_going"
 	"abort-al-dbz.exp simple.exp"
 	"PASS: running abort-al-dbz.exp.*\
diff --git a/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp b/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp
new file mode 100644
index 0000000..711347d
--- /dev/null
+++ b/testsuite/runtest.main/abort/testsuite/abort.test/abort-dbz.exp
@@ -0,0 +1,29 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+#
+# This file is part of DejaGnu.
+#
+# DejaGnu is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# DejaGnu is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with DejaGnu; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+# Cause a divide-by-zero error.
+
+pass "running abort-dbz.exp"
+
+proc throw_arith_error_div_by_zero { } {
+    expr { 1 / 0 }
+}
+
+throw_arith_error_div_by_zero
+
+fail "script did not abort"

Message #20 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Jacob Bachmeyer <jcb62281 <at> gmail.com>
To: Tom de Vries <tdevries <at> suse.de>
Cc: 41918 <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Fri, 19 Jun 2020 19:32:12 -0500

Tom de Vries wrote:
> On 6/19/20 2:00 AM, Jacob Bachmeyer wrote:
>   
>> Tom de Vries wrote:
>>     
>>> On 6/18/20 1:19 AM, Jacob Bachmeyer wrote:
>>>  
>>>       
>>>> Tom de Vries wrote:
>>>>    
>>>>         
>>>>> Hi,
>>>>>
>>>>> I think I found a bug in proc unknown in lib/framework.exp.
>>>>>
>>>>> Patch describing the problem and fixing it attached below.
>>>>>         
>>>>>           
>>>> I found a similar issue while patching bug #41824; please check whether
>>>> that patch addresses this issue adequately.
>>>>     
>>>>         
>>> AFAICT, it does not.  Dejagnu test-case attached.
>>>   
>>>       
>> That test is incorrect:  it does not specify the --keep_going option,
>> but expects runtest to continue after a test script aborts with a Tcl
>> error.
>>     
>
> Well, yes, it's respecting the difference in handling of proc-not-found
> vs other tcl errors that is present in current dejagnu.
>
> Since:
> - your implementation of --keep_going did not modify this, and
> - your description of keep-going in NEWS explictly respected that
>   difference,
> I had no reason to deviate from that.
>
> Now that you've decided to change the handling of proc-not-found vs
> other tcl errors in patch 0005, obviously the test-case needs to be
> updated to accomodate for that.
>
>   

That handling was actually very inconsistent:  an error in a regular 
testcase aborts the script, but testing (almost) silently continued, but 
an error if auto-loading had been used aborted the entire test run.  
That difference in handling proc-not-found and other Tcl errors was a 
long-standing bug in DejaGnu.  Any Tcl error means that the results are 
not really valid, so almost silently continuing is wrong and was wrong.

>> I have also attached another patch (0005) that fixes an inconsistency in
>> DejaGnu's handling of Tcl errors in test scripts.  Previously, DejaGnu
>> would abort the test run if an undefined procedure is called, but would
>> only issue an ERROR message and continue with the next test script for
>> all other Tcl errors.  This patch solves that by ensuring that DejaGnu
>> will abort on any uncaught Tcl error in a test script unless the
>> --keep_going option is given on the command line.  An UNRESOLVED result
>> appears in the log in any case if a test script aborts due to an error.
>>
>>     
>
> I couldn't apply this cleanly, so instead I'm now looking at branch PR41918.
>   

Odd, but that is why I pushed that branch.

> I see that for abort-undef.exp we have:
> ...
> ERROR: (DejaGnu) proc "bogus_command 1 2 3 4" does not exist.
> The error code is TCL LOOKUP COMMAND bogus_command
> The info on the error is:
> invalid command name "bogus_command"
>     while executing
> "::tcl_unknown bogus_command 1 2 3 4"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel 1 ::tcl_unknown $args"
> ERROR: tcl error sourcing
> /home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp.
> ERROR: tcl error code TCL LOOKUP COMMAND bogus_command
> ERROR: invalid command name "bogus_command"
>     while executing
> "bogus_command 1 2 3 4"
>     (file
> "/home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
> line 23)
>     invoked from within
> "source
> /home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 source
> /home/vries/dejagnu/devel/src/testsuite/runtest.main/abort/testsuite/abort.test/abort-undef.exp"
>     invoked from within
> "catch "uplevel #0 source $test_file_name""
> ...
>
> Should we error twice on this? FWIW, just removing ::unknown seems to be
> the easiest way to fix that.
>   

I have since realized that DejaGnu's ::unknown seems to be about useless 
other than producing that first error.  I am thinking about removing it 
entirely now that runtest aborts after catching a Tcl error from a test 
script.

> Anyway, I have two patches attached:
> - one that rewords the NEWS entries into something more intuitive for me
>   
Agreed; the NEWS entries were written separately and the later change 
did make the first one wrong.  Thanks.

> - one that adds a test-case abort-dbz.exp, a copy of
>   abort-al-dbz.exp, but without the auto-load bit.
>   
Agreed; that was a missing case in the testsuite.  Thanks.



-- Jacob

Message #23 received at 41918 <at> debbugs.gnu.org (full text, mbox):

From: Jacob Bachmeyer <jcb62281 <at> gmail.com>
To: 41824 <at> debbugs.gnu.org, 41918 <at> debbugs.gnu.org
Subject: Branch PR41918 containing fixes for these issues is into final testing
Date: Tue, 30 Jun 2020 18:54:51 -0500

The temporary branch PR41918 (which earlier absorbed the PR41824 
temporary branch) is now merged onto the testing queue ahead of master.  
After these patches pass full testing, master will be fast-forwarded to 
include them.

-- Jacob

Message #30 received at 41918-done <at> debbugs.gnu.org (full text, mbox):

From: Jacob Bachmeyer <jcb62281 <at> gmail.com>
To: Tom de Vries <tdevries <at> suse.de>
Cc: 41918-done <at> debbugs.gnu.org
Subject: Re: bug#41918: [PATCH] Propagate error value of auto-loaded command
Date: Wed, 08 Jul 2020 18:53:49 -0500

The fix for this bug was rolled into a larger bugfix branch that has 
landed on master and will be included in the upcoming 1.6.3 release.

-- Jacob

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.