Tcl Source Code

View Ticket
Login
Ticket UUID: 2868499
Title: TIP #348: Substituted errorStack
Type: Patch Version: TIP Implementation
Submitter: ferrieux Created on: 2009-09-27 23:57:21
Subsystem: None Assigned To: dgp
Priority: 9 Immediate Severity:
Status: Closed Last Modified: 2010-05-03 06:26:41
Resolution: Accepted Closed By: dgp
    Closed on: 2010-04-05 22:14:29
Description:
Here is a crude but functional implementation of TIP #348.
The variable ::errorStack, comes to life when defined (which is not the case by default, to avoid any performance hit).
While unwinding, it is populated by substituted argslists as described in the TIP.
It is automatically reset (to the empty list)  whenever ::errorInfo is. (But this leaves it enabled. To disable it, unset it.)

(Among crudities: uses ::errorStack instead of ::tcl::errorStack, doesn't pay attention to code style, no test cases.
Also, it inserts two fields in the private interp structure, right in the middle, not at the end. ABI hazard, don't forget to 'make clean' after applying  ;-)
User Comments: ferrieux added on 2010-05-03 06:26:41:

allow_comments - 0

ferrieux added on 2010-05-03 06:26:40:
Note that patch 2995655 now has the proposed "inner contexts" extension.

dgp added on 2010-04-06 05:14:29:

allow_comments - 1

ferrieux added on 2010-04-06 02:51:27:
Committed to HEAD.

dgp added on 2010-04-01 00:48:08:
I scanned the Tcl sources for errorinfo/errorInfo
and only found one thing to mention:

dgpI think we can and should demand that any background error handler wishing to make use of [info errorstack]....
dgp... must be one created using the [interp bgerror] interface.
dgpso while we continue to offer legacy support by setting ::errorInfo and ::errorCode before invoking ::bgerror ....
dgp... we do not go to the trouble of making [info errorstack] follow suit.

Otherwise, most return options relevant code
in Tcl itseld is using utility routines like
Tcl_Get|SetReturnOptions() and
Tcl_Save|RestoreInterpState() so they
end up working with the new -errorstack
automatically.

ferrieux added on 2010-04-01 00:11:59:
Summary from talk on the chat:
To get to the "inner context" (ie the just-executed command/instruction when the error first bursts, in the first T_LCI call in a row), two families of cases exist:
 
    - error generated by a bytecode instruction: based on a stack arity table (+ special casing of INST_EXPAND_*) we can reconstruct the objv[] out of the stack contents (not yet popped). What's nice is that TEBC has a single call site of T_LCI.

  - error generated in one of the many non-compiled error cases from tclBasic.c. These are trickier since there is no uniform way of reaching the objv[]. In some cases we might fall back to just displaying the static source.

I believe the compiled case is top priority, because it applies to most code anyway.
(would have _loved_ the arity suffix like INST_LOAD_SCALAR1 to be generalized...)

dgp added on 2010-03-31 22:53:11:
Updated patch makes tiny improvement
to T_LCI.

FWIW, I'd make some different choices
in the -errorcode values, but I'll defer to
whatever scheme dkf has been building.

Still want to do something of an audit of
places where -errorinfo gets tinkered with
to check whether -errorstack needs parallel
treatment.  That can happen post-commit.
To the extent that more fixes are needed they
will be fixes of fairly obsure bugs.

dgp added on 2010-03-31 22:51:08:

File Added - 368969: 2868499.patch

dgp added on 2010-03-31 22:45:33:

File Deleted - 368110:

ferrieux added on 2010-03-31 21:18:03:
Thanks, will look at these in detail when polishing the tests.
Just updated the patch to handle interp state Save/Restore/Discard and end-of-interp-life, and also the check for even-sized -errorstack value.

ferrieux added on 2010-03-31 21:16:27:

File Added - 368958: errorstack15.patch

dgp added on 2010-03-31 01:57:32:

File Added - 368850: stack.tcl

dgp added on 2010-03-31 01:56:09:
Attached a demo script that puts
the new feature through its paces
with many commands that push
CallFrames.  Could be a starting
point for coverage tests for the
test suite.

dgp added on 2010-03-30 01:52:14:
Since the revised structure of the -errorstack
value demands that it be a list with an even number
of elements, that check should be added to 
[return -errorstack $value].

ferrieux added on 2010-03-30 01:47:32:
Yes. Will shortly replace them by a glob match with a trailing star.

dgp added on 2010-03-30 01:45:14:
Several of the results in the new/revised tests are
checking for {UP 1} as the last element of the errorstack.
This is exposing details of the tcltest framework.  This is
likely only a theoretical problem, but more robust tests
won't do this as protection again possible coding changes
in future versions of the tcltest package.

ferrieux added on 2010-03-26 04:46:50:
Uploaded errorstack14.patch implementing solution "fast, flat, out-of-band".

   CALL {foo x y} UP 1 CALL {bar z} CALL {gnu t}

Docs and minimal tests updated accordingly.

ferrieux added on 2010-03-26 04:45:09:

File Added - 368223: errorstack14.patch

ferrieux added on 2010-03-25 05:02:58:
Re syncing errorinfo and errorstack, I believe you're asking too much for little gain. The current approach is to record the (valuable) dynamic objv[] when it's available *and* we manage to be called. We manage to be called by the T_LCI "vehicle", which is nicer than the duplicated code that was in place before, but any higher frequency would be much more complex. Delaying the recording of the objv[] after its initial availability amounts to shifting the list at the end, I don't see the point.

ferrieux added on 2010-03-25 04:40:14:
Re the relative levels: this is both intentional and hard to avoid.

- Intentional, because only a relative level allows unambiguous interpretation  of the scopes when the error stack is not taken from toplevel (eg in a deep [catch]), hence the lat item is not at level 1. {uplevel N} simply means you need to skip N (non-uplevel) items on the right to find the scope.

- Hard to avoid, because the exact nature of the scope-shifting operation can be very varied, and distinguishing absolute and relative uplevels would imply re-parsing the text or just-run bytecode (eeek).

dgp added on 2010-03-25 03:45:34:
There remains an offset in the timing of appends
to -errorinfo and -errorstack:

proc myEval script {
    if {[catch {uplevel 1 $script} m o] == 1}  {
        puts EI:[dict get $o -errorinfo]\n
        puts ES:[dict get $o -errorstack]
    }
}
proc foo {} {error bar}
myEval foo

OUTPUT:
EI:bar
    while executing
"error bar"
    (procedure "foo" line 1)
    invoked from within
"foo"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 $script"

ES:foo {uplevel 1} {myEval foo}

It is clear that the each Tcl_LogCommandInfo
call is appending to -errorinfo text related to
the commands that have completed their
evaluation, while it appends to -errorstack
information about the current context which
has not yet been terminated.  Parallel appends
would be less confusing.

If -errorstack construction could be changed to
match -errorinfo, then the [control::eval] command
would already handle -errorstack correctly as written.

With the current patch, the [control::eval] command
needs to have code added to strip a final {uplevel 1}
from the -errorstack to fully disguise the wrapping replacement.
The analogous issue would impact [control::ascaller] too.

dgp added on 2010-03-25 03:24:44:
oops, uploaded that patch before learning
that result-6.4 needs an update to match it.

dgp added on 2010-03-25 03:22:51:
ok, that logic wasn't hard to correct. see new patch.

Not a huge deal, but I notice that the constructed uplevel
entries always reports the relative level difference, even
when it was a command using an absolute level, like
[uplevel #0] that caused the branch in the tree of CallFrames.

dgp added on 2010-03-25 03:21:06:

File Added - 368110: 2868499.patch

dgp added on 2010-03-25 02:35:05:
In the latest patch the uplevel entries don't
appear to happen at the toplevel.  Contrast:

% proc foo {} {uplevel 1 bar}
% proc bar {} {error baz}
% catch foo m o
1
% dict get $o -errorstack
bar foo
%
% proc wrap {} {foo}
% catch wrap m o
1
% dict get $o -errorstack
bar {uplevel 1} foo wrap


The uplevel entry between bar and foo
doesn't get inserted when foo is called
from level #0.

ferrieux added on 2010-03-24 22:58:05:

File Added - 368071: errorstack13.patch

ferrieux added on 2010-03-24 22:55:23:
Attaching patch displaying the uplevel slots as {uplevel $relativelevel}.

ferrieux added on 2010-03-24 04:25:07:
Thanks to Miguel for the hint: varFramePtr != framePtr is the simple check for [uplevel] spurious echoes. Patch updated.

ferrieux added on 2010-03-24 04:24:04:

File Added - 367943: errorstack12.patch

ferrieux added on 2010-03-19 06:08:43:
Patch updated to include the cleaner MergeRetOpts-based check for listness of -errorstack values, and a test case.

ferrieux added on 2010-03-19 06:07:50:

File Added - 367319: errorstack11.patch

ferrieux added on 2010-03-18 05:57:58:
Let's handle that tiny issue in the bug 2383005 itself.

ferrieux added on 2010-03-18 05:39:45:
Attached patch doing as promised, [return -errorstack], and a new test case checking that this affects [info errorstack] on subsequent [catch] as expected.

Note that it also includes the fix for 2383005.

Note however, that in both cases if a non-list like "{}a" is passed to [return -errorstack/errorcode], the resulting error message remains the final arg of [return -code error ...] instead of 'list element in braces followed by "a" instead of space'.

I suspect this is the work of Tcl_TransferResult but I'm not familiar with that part, I'd appreciate help (ipso facto solving 2383005).

ferrieux added on 2010-03-18 05:30:17:

File Added - 367176: errorstack10.patch

ferrieux added on 2010-03-18 03:47:03:
Ah, OK. Just noticed that [return -foo bar something] just populates the dict with key -foo and value bar. I had wrongly assumed that only the meaningful ones were passed, erroring on others.

Anyway I accept the argument of symmetry with errorinfo. Will shortly upload an update including the r/w option.

dgp added on 2010-03-18 00:52:21:
um, that's novel. :)

I might be convinced to make this
one special, but in that case I don't
think the right thing to do with
[return -errorstack] is to just let it
silently fail.  If we are to disallow it,
raise an error.

I was wondering myself about
whether [info errostack] would be
sufficient.  The issue appears to
come down to whether or not
the value of -errorstack ought to
be considered part of the "state"
of the interp -- those values saved
and restored by Tcl_*InterpState()
and cable to be [catch]-ed and
re-raised by [return -options].  Since
you call -errorstack a sister of -errorinfo,
my first assumption is that it is part of
that state, and needs to be part of that
save/restore capability, and for that reason
needs to be writable via the [return] command.

ferrieux added on 2010-03-18 00:46:11:
No, -errorstack is just a readonly optsDict member. Even Joe, who asked for it in the first place, didn't ask for a [return -errorstack]. Personally I believe(d) that restricting ourselves to [info/interp errorstack] is(was) even better, but I yielded.

Question1: do you think that a read-only optsDict member is out of question ?
Question2: where's your preference between:
    (a) a read-write opt
    (b) no opt, just [info/interp errorstack]
?

dgp added on 2010-03-17 21:50:59:
TclProcessReturn() does not appear
to have anything in it to pull a -errorstack
value out of the options dict to be stored
in the iPtr->errorstack field.

dgp added on 2010-03-17 21:03:44:
Need to examine whether
Tcl_SaveInterpState and friends
need to give the -errorstack dict
entry any special treatment like
errorInfo and errorCode receive,
or whether it's fully covered by
the returnOpts dict.

dgp added on 2010-03-17 21:01:29:
-errorstack is like -errorcode in that
it wants a list argument.  This patch
suffers from the same bug as 2383005.
Both bugs should be fixed.

ferrieux added on 2010-03-17 07:59:06:
Patch re-updated to HEAD, and now includes two updates suggested by dgp:
(a) restriction of -errorstack dict option to TCL_ERROR cases
(b) simplification of the internal [lappend] used

ferrieux added on 2010-03-17 07:57:35:

File Added - 367044: errorstack9.patch

ferrieux added on 2010-02-08 17:08:38:
Patch updated to HEAD.

ferrieux added on 2010-02-08 17:08:22:

File Added - 362053: errorstack8.patch

ferrieux added on 2009-11-09 20:26:40:
Added documentatoin in catch.n and info.n.

ferrieux added on 2009-11-09 20:26:18:

File Added - 350135: errorstack7.patch

ferrieux added on 2009-11-04 00:20:26:

File Added - 349392: errorstack6.patch

ferrieux added on 2009-10-31 23:37:42:
Test suite now fixed. Thanks to -match glob :)

ferrieux added on 2009-10-31 23:36:43:

File Added - 348992: errorstack5.patch

ferrieux added on 2009-10-31 23:01:39:
Patch now restricted as per Joe's recommendations on tclcore:
 - always on
 - non-flat list
 - with options dict
Test suite now needs fixes wherever the options dict was checked in extenso...

ferrieux added on 2009-10-31 22:59:31:

File Added - 348989: errorstack4.patch

ferrieux added on 2009-10-23 08:03:15:
Extended patch adds bit value 4 as "flat list", ie errorstack is built as
 2 foo bar 1 baz 2 gnu gnats
instead of
 {foo bar} baz {gnu gnats}
Hence:
   use=0 : pristine
   use=1 : info errorstack
   use=2 : + dict
   use=5 : 1 + flatlist
   use=6 : 2 + flatlist

Timings below:
Case: Ten-fold catch ladder use=0 -> 101.3854 microseconds per iteration
Case: Ten-fold catch ladder use=1 -> 106.2653 microseconds per iteration
Case: Ten-fold catch ladder use=2 -> 106.8439 microseconds per iteration
Case: Ten-fold catch ladder use=5 -> 103.5229 microseconds per iteration
Case: Ten-fold catch ladder use=6 -> 103.7669 microseconds per iteration
Case: Single Catch with opt dict use=0 -> 25.814 microseconds per iteration
Case: Single Catch with opt dict use=1 -> 26.2905 microseconds per iteration
Case: Single Catch with opt dict use=2 -> 27.0002 microseconds per iteration
Case: Single Catch with opt dict use=5 -> 26.0665 microseconds per iteration
Case: Single Catch with opt dict use=6 -> 26.783 microseconds per iteration

Conclusion: while the gain for a tight catch loop is negligible, for a ten-fold  catch ladder building a long errorstack it is substantial: the flatlist roughly divides the overhead by three.

However, the flatlist is a bit less convenient for the programmer.
It could be postprocessed into a nested list in [info errorstack] but not int the options dict...

ferrieux added on 2009-10-23 07:56:29:

File Added - 347809: errorstack3.patch

ferrieux added on 2009-10-20 23:17:22:
Attaching a new patch, errorstack2.patch, fulfilling Joe's request of [info errostack ?interp?] and [dict get $d -errostack].
I left the ::tcl::useErrorStack control variable for perf measurements:
    0 --> pristine HEAD
    1 --> just [info errorstack]
    2 ---> adds options dict entry -errorstack

Note the implementation still uses ckalloc-based Tcl_NewListObj for argslist, pending proof that optimization is needed.
Here are the figures:

Case: Ten-fold catch ladder  use=0 ->    74.7924 microseconds per iteration
Case: Ten-fold catch ladder  use=1 ->    76.7888 microseconds per iteration
Case: Ten-fold catch ladder  use=2 ->    77.1977 microseconds per iteration
Case: Single Catch with opt dict  use=0 ->    19.2613 microseconds per iteration
Case: Single Catch with opt dict  use=1 ->    19.7608 microseconds per iteration
Case: Single Catch with opt dict  use=2 ->    20.2815 microseconds per iteration

with code:

for {set i 1} {$i<10} {incr i} {
proc f$i x "f[expr {$i+1}] \$x"
}
proc f10 x {error F10:$x}

proc boo {} {error BOO}

proc catchloop {} {
for {set i 0} {$i<100000} {incr i} {
if {[catch boo m d]} continue
}
}

foreach {mm ss} {
"Ten-fold catch ladder" {catch {f1 12}}
"Single Catch with opt dict" {catch boo m d}
} {

foreach v {0 1 2} {
set ::tcl::useErrorStack $v
puts "Case: $mm  use=$v ->    [time $ss 10000]"
}
}

ferrieux added on 2009-10-20 23:12:30:

File Added - 347441: errorstack2.patch

ferrieux added on 2009-10-07 14:04:37:

File Deleted - 345369:

ferrieux added on 2009-10-07 14:04:27:

File Added - 345590: errorstack.patch

ferrieux added on 2009-10-05 15:12:36:
Complete with test cases now.

ferrieux added on 2009-10-05 15:11:43:

File Deleted - 345341:

ferrieux added on 2009-10-05 15:11:33:

File Added - 345369: errorstack.patch

ferrieux added on 2009-10-05 07:46:02:
Mostly done.
- now the var is ::tcl::errorStack and the flag is ::tcl::useErrorStack
- zero overhead when ::tcl::useErrorStack is 0 (default)
- uses a read trace to postpone var machinery until real use
- uses an optimized in-place lappend/reset to keep the list intrep as much as possible (just as fast as the linked list approach IMO)
TBD: test cases.

ferrieux added on 2009-10-05 07:41:09:

File Deleted - 344684:

ferrieux added on 2009-10-05 07:40:46:

File Added - 345341: errorstack.patch

ferrieux added on 2009-09-29 06:00:27:
First slight improvement: explicitly skip iPtr->rootFramePtr, like in [info level $n], since the root frame will always have argc==0. This removes the parasitic {} always found at the end of ::errorStack, and also an extra one generated by [tailcall] and [coroutine].

Next improvements planned:

(1) make the on/off switch more efficient by using a linked boolean var rather than the existence of ::errorStack which costs a hash lookup when negative.

(2) make the list building slightly more efficient by using a backbone of Tcl_Obj as Cons'es (linked list), and turning it into a Tcl list only in a read trace attached to ::errorStack (instead of directly lappending as is currently the case. This construction is wasted in caught, expected exception handling where the errorStack is not used).

(3) proper ::tcl namespace + test cases.

ferrieux added on 2009-09-29 05:48:21:

File Deleted - 344544:

ferrieux added on 2009-09-29 05:48:07:

File Added - 344684: errorstack.patch

ferrieux added on 2009-09-28 06:57:24:

File Added - 344544: errorstack.patch

Attachments: