Improve internal reporting of errors in vm/vmgen
#268
Conversation
zerbina
force-pushed
the
vm-error-handling
branch
3 times, most recently
from
8f68d9d
to
6d66bfd
Compare
There was a problem hiding this comment.
Code is looking much improved, the more uniform handling is very welcome.
I suggested a possible way to handle putting the exception trace in the nkError node, on the phone so it's a bit light on details. I can dig in more on the weekend (slim chance earlier) if it comes up short.
The minor style remarks aren't do or die, shortly evolving the code style, eventually as it cements it can go into the linter.
| proc rawExecute(c: var TCtx, start: int, sframe: StackFrameIndex): TFullReg = | ||
| assert sframe == c.sframes.high | ||
| proc rawExecute(c: var TCtx, pc: var int, tos: var StackFrameIndex): TFullReg = | ||
| ## Runs the execution loop, starting in frame `tos` at program counter `pc`. |
| @@ -0,0 +1,44 @@ | |||
| ## This module contains the definitions for error reporting inside | |||
| @@ -152,25 +152,6 @@ when defined(nimHasInvariant): | |||
| of MultipleValueSetting.cincludes: copySeq(conf.cIncludes) | |||
| of MultipleValueSetting.clibs: copySeq(conf.cLibs) | |||
|
|
|||
| proc stackTrace2(c: PCtx, report: SemReport, n: PNode) = | |||
| # TODO: put the stack-trace into the error somehow, nothing happens | ||
| # with it right now. Maybe use report's `context`? | ||
|
|
||
| result = newError(config, node, errKind, rId, instLoc()) |
There was a problem hiding this comment.
IIRC, error node now has specialized storage so it might be possible to put a report object in there, in which case creates report object to store the trace. Otherwise, serialize to a string, shove it in there like a son.
(on my phone, so hard to look things up)
There was a problem hiding this comment.
So, should I add the stack-trace's report ID as a nkIntLit to the error node then? Serializing it to a string, would require going through cli_reporter
There was a problem hiding this comment.
Yes, that makes sense. Adding it in as an additional argument is likely the way to go.
zerbina
force-pushed
the
vm-error-handling
branch
from
6d66bfd
to
8850e11
Compare
|
Requested style adjustments are applied. In addition, the stack-trace's report id is now added to the error node as a In the future, VM execution related errors should use their own report type (e.g. |
Reporting of guest code related errors was done in an inconsistent manner via either `stackTrace` or `globalReport`, where `stackTrace` used `localReport` and an injected `return` statement to terminate the execution loop. `stackTrace` was sometimes used in nested functions, thus not terminating execution at all. ## Changes Both the execution engine and `vmgen` now use exceptions for error propagation internally, translating them into a result value at their respective interface edges. In case of an error, the result value stores the error report. Otherwise, it stores the actual result. In addition, some usages of `globalReport` are also replaced with the new error propagation mechanism. Depending on whether or not a VM invocation is expected to return something, errors reported from VM execution and/or `vmgen` are either turned into a `nkError` node or handled directly via `handleReport`
|
Build succeeded: |
Reporting of guest code related errors was done in an inconsistent
manner via either
stackTraceorglobalReport, wherestackTraceusedlocalReportand an injectedreturnstatement to terminate theexecution loop.
stackTracewas sometimes used in nested functions,thus not terminating execution at all.
Changes
Both the execution engine and
vmgennow use exceptions for errorpropagation internally, translating them into a result value at their
respective interface edges. In case of an error, the result value stores the
error report. Otherwise, it stores the actual result.
In addition, some usages of
globalReportare also replaced with thenew error propagation mechanism.
Depending on whether or not a VM invocation is expected to return
something, errors reported from VM execution and/or
vmgenare eitherturned into a
nkErrornode or handled directly viahandleReportFixes #160
This implements an adjusted/simplified version of the proposal from the VM discussion thread. Error reporting should be a bit more consistent and streamlined now, making future changes to it a bit easier.
Notes for Reviewers
newErrorusage rightnkErrorwas, so the stack-traces are simply dropped right now