[Tcl-bugs] [ tcl-Patches-2995655 ] Report inner contexts in [info errorstack]

[SourceForge.net <no...@so...>]

Patches item #2995655, was opened at 2010-05-03 01:24
Message generated for change (Comment added) made by ferrieux
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=310894&aid=2995655&group_id=10894

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 9
Private: No
Submitted By: Alexandre Ferrieux (ferrieux)
Assigned to: miguel sofer (msofer)
Summary: Report inner contexts in [info errorstack]

Initial Comment:
In TIP #348, now accepted and committed, the error stack reports a list of argslists for procedure calls, along with uplevel offsets.
However, it has been noted that the deepest argslist, aka the "inner context", ie the bytecode instruction and the arguments it used off the stack before deciding to raise an error, are not reported. This is frustrating, typically when [expr] with a complex expression raises an error like "domain error: argument not in valid range".

The attached patch, designed after advice from Miguel, overcomes this limitation by inserting (when possible) at the beginning of the errorstack:

    INNER {name_of_instruction arg1 arg2 ... argN}

for example:

    % proc f {x y} {expr {log($x)+acos($y)}}
    % f 1 2
    domain error: argument not in valid range
    % info errorstack
     INNER {invokeStk1 tcl::mathfunc::acos 2} CALL {f 1 2}

A few notes on the implementation:
(1) the impact on TEBC is intentionally minimal (gentle arm twisting, Mig ;-): a single line, saving the pc in another local.
(2) only those INSTs where the argc is readily computable, generate a nontrivial inner context report. Others are untouched, just like non-TEBC error sites.
(3) the 'knowledge base' giving the algorithm for counting the argc is a single switch() block mimicking a tiny part of TEBC, but which can evolve much slower, since the default is to report only the instruction name. Hence no need to 'keep in sync' too tightly. This should free the mind of those who want to innovate in TEBC...
(4) at this stage, there's room for optimization, like pre-computing and sharing the instruction-name literals, and also reusing the list storage for the inner context, just like was done for the error stack itself.

High prio because it is an extension that has been asked for during the TIP discussion, the TIP is very young, and a milestone release is approaching...

----------------------------------------------------------------------

>Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-10-10 00:35

Message:
Attaching inner8.patch which has two perf improvements:
  - no unnecessary alloc of a list for the inner context's argslist,
thanks to the reuse of a cached list when unshared
  - no unnecessary alloc of an inst's name, nor table of such Tcl_Objs,
through a dedicated new objtype (InstNames)
Bottom line: on the critical path, NO allocations except Tcl_Objs are done
by the inner context extractor.

----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-09-29 23:49

Message:
OK, you made it all clear in one sentence: with saner NRE, TEBC is no
longer a long-lived while(1)...switch(*pc), but rather a TEBCresume
callback that gets re-pushed forever doing switch(*pc). Neat !
As a result, please find attached as inner6.patch the corrected inner
context patch. Outline:
  - pcBeg is now a field in BP as per your suggestion
  - it is directly initialised to pc at beginning of TEBCresume (and to
codeStart at beginning of TnrEBC)
  - it is directly used in the TclLogCommandInfo call
  - no more extra local :)
Test suite okay.

Now re the coro warning, it seems to cross the coro bottom correctly, up
into the resume's caller stack, just as before (after you fixed the wub
crash). Moreover, we see that [info level $n] doesn't cross the coro
bottom:

% proc c x {yield;stack;error BEUH:$x}
% coroutine C c bla
% proc stack {} {catch {for {set n 0} {1} {incr n -1} {puts "LEVEL $n:
[info level $n]"}}}
% proc f x {g $x}
% proc g x C
% f 2
LEVEL 0: stack
LEVEL -1: c bla
BEUH:bla
% info errorstack
INNER {returnStk BEUH:bla} CALL {c bla} CALL {g 2} CALL {f 2}


----------------------------------------------------------------------

Comment By: miguel sofer (msofer)
Date: 2010-09-29 00:29

Message:
Mmhhh ... is this in coroutines? We stopped insuring that the framePtr
chains can be traversed across the coroutine's bottom. Please see what
[info frame] does to patch the chain when it needs it: in InfoFrameCmd,
tclCmdIL.c lines 1156ff

If the problem is *not* in coroutines, no idea: let's talk.

----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-09-28 23:38

Message:
Yikes ! Saner NRE kicked pcBeg out of sync again :/
Will dive, but insight appreciated: how has the return-from-error path
changed, to reach this effect ?

----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-09-27 23:15

Message:
Patch updated to HEAD (saner NRE). Timings coming soon.

----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-09-07 00:20

Message:
OK, problem solved,  helped by bug 3059758. The core of the issue was not
the tosPtr, but rather the 'pc jitter' (jumping to the last byte of the
INST). Solved by adding a local to TEBC, and keeping it in sync in
NR_DATA_DIG.
Test suite OK.

----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-06-08 23:43

Message:
Some progress: looks like the 'abnormalReturn' target in TEBC is associated
with situations where the stack pointer is rewound out of sync with the pc
(when popping from a nonrecursive call). Attached inner3.patch has a few
additional checks do detect that early. Simplest crashing case is opt-3.2,
summarized below:

package require opt 0.4.6
set n $::tcl::OptDescN
catch {::tcl::OptKeyDelete $n}
catch {::tcl::OptParse {{-foo}} {-blah}}

I suspect that a single NR_DATA_DIG could do the job, but I'd appreciate
guidance.


----------------------------------------------------------------------

Comment By: Alexandre Ferrieux (ferrieux)
Date: 2010-06-08 01:38

Message:
Improved version now handles all sorts of compile-time, deferred errors.

Still crashing in the four following tests, to be investigated:

  namespace.test

  opt.test

  tcltest.test

  trace.test



----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=310894&aid=2995655&group_id=10894