Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM callbacks can now raise (and be caught, or show their stacktrace) #13714

Closed
1 change: 1 addition & 0 deletions compiler/condsyms.nim
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,4 @@ proc initDefines*(symbols: StringTableRef) =
defineSymbol("nimHasSinkInference")
defineSymbol("nimNewIntegerOps")
defineSymbol("nimHasStacktraceMsgs")
defineSymbol("nimHasInternalProc")
1 change: 1 addition & 0 deletions compiler/modulegraphs.nim
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type
exposed*: TStrTable
intTypeCache*: array[-5..64, PType]
opContains*, opNot*: PSym
nimInternalNewException*: PSym
emptyNode*: PNode
incr*: IncrementalCtx
canonTypes*: Table[SigHash, PType]
Expand Down
8 changes: 7 additions & 1 deletion compiler/pragmas.nim
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const
## common pragmas for declarations, to a good approximation
procPragmas* = declPragmas + {FirstCallConv..LastCallConv,
wMagic, wNoSideEffect, wSideEffect, wNoreturn, wNosinks, wDynlib, wHeader,
wCompilerProc, wNonReloadable, wCore, wProcVar, wVarargs, wCompileTime, wMerge,
wCompilerProc, wNonReloadable, wCore, wInternalProc, wProcVar, wVarargs, wCompileTime, wMerge,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important to avoid introducing new pragmas

wBorrow, wImportCompilerProc, wThread,
wAsmNoStackFrame, wDiscardable, wNoInit, wCodegenDecl,
wGensym, wInject, wRaises, wTags, wLocks, wDelegator, wGcSafe,
Expand Down Expand Up @@ -897,6 +897,12 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
incl(sym.flags, sfWasForwarded)
of wDynlib:
processDynLib(c, it, sym)
of wInternalProc:
noVal(c, it)
if sym.name.id == getIdent(c.cache, "nimInternalNewException").id:
c.graph.nimInternalNewException = sym
else:
doAssert false, sym.name.s
of wCompilerProc, wCore:
noVal(c, it) # compilerproc may not get a string!
cppDefine(c.graph.config, sym.name.s)
Expand Down
21 changes: 6 additions & 15 deletions compiler/scriptconfig.nim
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ from strutils import cmpIgnoreStyle, contains

proc listDirs(a: VmArgs, filter: set[PathComponent]) =
let dir = getString(a, 0)
let checkDir = getBool(a, 1)
var result: seq[string] = @[]
for kind, path in walkDir(dir):
for kind, path in walkDir(dir, checkDir = checkDir):
if kind in filter: result.add path
setResult(a, result)

Expand All @@ -35,7 +36,6 @@ proc setupVM*(module: PSym; cache: IdentCache; scriptName: string;
let conf = graph.config

# captured vars:
var errorMsg: string
var vthisDir = scriptName.splitFile.dir

template cbconf(name, body) {.dirty.} =
Expand All @@ -44,22 +44,17 @@ proc setupVM*(module: PSym; cache: IdentCache; scriptName: string;
body

template cbexc(name, exc, body) {.dirty.} =
result.registerCallback "stdlib.system." & astToStr(name),
proc (a: VmArgs) =
errorMsg = ""
try:
body
except exc:
errorMsg = getCurrentExceptionMsg()
result.registerCallback "stdlib.system." & astToStr(name), mayRaise = true, callback =
proc (a: VmArgs) = body

template cbos(name, body) {.dirty.} =
cbexc(name, OSError, body)

# Idea: Treat link to file as a file, but ignore link to directory to prevent
# endless recursions out of the box.
cbos listFilesImpl:
cbos listFiles:
listDirs(a, {pcFile, pcLinkToFile})
cbos listDirsImpl:
cbos listDirs:
listDirs(a, {pcDir})
cbos removeDir:
if defined(nimsuggest) or graph.config.cmd == cmdCheck:
Expand All @@ -73,10 +68,6 @@ proc setupVM*(module: PSym; cache: IdentCache; scriptName: string;
os.removeFile getString(a, 0)
cbos createDir:
os.createDir getString(a, 0)

result.registerCallback "stdlib.system.getError",
proc (a: VmArgs) = setResult(a, errorMsg)

cbos setCurrentDir:
os.setCurrentDir getString(a, 0)
cbos getCurrentDir:
Expand Down
66 changes: 48 additions & 18 deletions compiler/vm.nim
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,27 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
"ra", regDescr("ra", ra), "rb", regDescr("rb", instr.regB),
"rc", regDescr("rc", instr.regC)]

proc onProcCall(prc: PSym, isClosure: bool, rb = 0, rc = 0, regs2: var seq[TFullReg]) =
Copy link
Member Author

@timotheecour timotheecour Mar 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to reviewer: this code blocks is mostly moved without change; the only change involves instances of regs2 vs regs (1 regs left intentionally)

let newPc = compile(c, prc)
# tricky: a recursion is also a jump back, so we use the same
# logic as for loops:
if newPc < pc: handleJmpBack()
#echo "new pc ", newPc, " calling: ", prc.name.s
var newFrame = PStackFrame(prc: prc, comesFrom: pc, next: tos)
newSeq(newFrame.slots, prc.offset+ord(isClosure))
# echo0b (prc.name.s, prc.offset+ord(isClosure),isClosure, rb, rc)
if not isEmptyType(prc.typ[0]):
putIntoReg(newFrame.slots[0], getNullValue(prc.typ[0], prc.info, c.config))
for i in 1..rc-1:
newFrame.slots[i] = regs2[rb+i]
# echo0b (i, newFrame.slots[i].kind, rc)
if isClosure:
newFrame.slots[rc] = TFullReg(kind: rkNode, node: regs2[rb].node[1])
tos = newFrame
move(regs, newFrame.slots)
# -1 for the following 'inc pc'
pc = newPc-1

case instr.opcode
of opcEof: return regs[ra]
of opcRet:
Expand Down Expand Up @@ -1193,10 +1214,34 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
let prc = if not isClosure: bb.sym else: bb[0].sym
if prc.offset < -1:
# it's a callback:
c.callbacks[-prc.offset-2].value(
let cb = c.callbacks[-prc.offset-2].addr
template call() = cb[].value(
VmArgs(ra: ra, rb: rb, rc: rc, slots: cast[ptr UncheckedArray[TFullReg]](addr regs[0]),
currentException: c.currentExceptionA,
currentLineInfo: c.debug[pc]))
if cb[].mayRaise:
let info = c.debug[pc]
try:
call()
except Exception as e:
var traceMsg: string
var trace = e.getStackTraceEntries
if trace.len > 0:
# we truncate the stacktrace to only keep the useful portion
var first = 0
for i in countdown(trace.len-1, 0):
if trace[i].procname == "rawExecute":
first = i
break
trace = trace[first..^1]
traceMsg = toStrDefault(trace)
let vars = [$e.name, e.msg, traceMsg]
var regs2 = newSeq[TFullReg](vars.len)
for i, ai in [$e.name, e.msg, traceMsg]:
putIntoReg(regs2[i], newStrNode(ai, info))
onProcCall(c.graph.nimInternalNewException, isClosure = false, rb = -1, rc = 1 + regs2.len, regs2)
else:
call()
elif importcCond(prc):
if compiletimeFFI notin c.config.features:
globalError(c.config, c.debug[pc], "VM not allowed to do FFI, see `compiletimeFFI`")
Expand All @@ -1221,23 +1266,7 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
else:
globalError(c.config, c.debug[pc], "VM not built with FFI support")
elif prc.kind != skTemplate:
let newPc = compile(c, prc)
# tricky: a recursion is also a jump back, so we use the same
# logic as for loops:
if newPc < pc: handleJmpBack()
#echo "new pc ", newPc, " calling: ", prc.name.s
var newFrame = PStackFrame(prc: prc, comesFrom: pc, next: tos)
newSeq(newFrame.slots, prc.offset+ord(isClosure))
if not isEmptyType(prc.typ[0]):
putIntoReg(newFrame.slots[0], getNullValue(prc.typ[0], prc.info, c.config))
for i in 1..rc-1:
newFrame.slots[i] = regs[rb+i]
if isClosure:
newFrame.slots[rc] = TFullReg(kind: rkNode, node: regs[rb].node[1])
tos = newFrame
move(regs, newFrame.slots)
# -1 for the following 'inc pc'
pc = newPc-1
onProcCall(prc, isClosure, rb = rb, rc = rc, regs)
else:
# for 'getAst' support we need to support template expansion here:
let genSymOwner = if tos.next != nil and tos.next.prc != nil:
Expand Down Expand Up @@ -1513,6 +1542,7 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
rc = instr.regC
idx = int(regs[rb+rc-1].intVal)
callback = c.callbacks[idx].value
# xxx: consider using same treatment as for other VmArgs callback
args = VmArgs(ra: ra, rb: rb, rc: rc, slots: cast[ptr UncheckedArray[TFullReg]](addr regs[0]),
currentException: c.currentExceptionA,
currentLineInfo: c.debug[pc])
Expand Down
13 changes: 9 additions & 4 deletions compiler/vmdef.nim
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,10 @@ type
currentException*: PNode
currentLineInfo*: TLineInfo
VmCallback* = proc (args: VmArgs) {.closure.}

CallbackElement = object
key*: string
value*: VmCallback
mayRaise*: bool
PCtx* = ref TCtx
TCtx* = object of TPassContext # code gen context
code*: seq[TInstr]
Expand All @@ -255,7 +258,7 @@ type
traceActive*: bool
loopIterations*: int
comesFromHeuristic*: TLineInfo # Heuristic for better macro stack traces
callbacks*: seq[tuple[key: string, value: VmCallback]]
callbacks*: seq[CallbackElement]
errorFlag*: string
cache*: IdentCache
config*: ConfigRef
Expand All @@ -278,9 +281,11 @@ proc refresh*(c: PCtx, module: PSym) =
c.prc = PProc(blocks: @[])
c.loopIterations = c.config.maxLoopIterationsVM

proc registerCallback*(c: PCtx; name: string; callback: VmCallback): int {.discardable.} =
proc registerCallback*(c: PCtx; name: string; callback: VmCallback, mayRaise = false): int {.discardable.} =
# Alternatively, we could also pass (as a string) the exception(s) we expect
# to catch, eg `EOFError`
result = c.callbacks.len
c.callbacks.add((name, callback))
c.callbacks.add(CallbackElement(key: name, value: callback, mayRaise: mayRaise))

const
firstABxInstr* = opcTJmp
Expand Down
4 changes: 2 additions & 2 deletions compiler/vmgen.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1975,8 +1975,8 @@ proc matches(s: PSym; y: varargs[string]): bool =
proc procIsCallback(c: PCtx; s: PSym): bool =
if s.offset < -1: return true
var i = -2
for key, value in items(c.callbacks):
if s.matches(key):
for ai in items(c.callbacks):
if s.matches(ai.key):
doAssert s.offset == -1
s.offset = i
return true
Expand Down
10 changes: 5 additions & 5 deletions compiler/vmops.nim
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ template systemop(op) {.dirty.} =
registerCallback(c, "stdlib.system." & astToStr(op), `op Wrapper`)

template ioop(op) {.dirty.} =
registerCallback(c, "stdlib.io." & astToStr(op), `op Wrapper`)
registerCallback(c, "stdlib.io." & astToStr(op), `op Wrapper`, mayRaise = true)

template macrosop(op) {.dirty.} =
registerCallback(c, "stdlib.macros." & astToStr(op), `op Wrapper`)
Expand Down Expand Up @@ -96,9 +96,9 @@ proc getCurrentExceptionMsgWrapper(a: VmArgs) {.nimcall.} =
proc getCurrentExceptionWrapper(a: VmArgs) {.nimcall.} =
setResult(a, a.currentException)

proc staticWalkDirImpl(path: string, relative: bool): PNode =
proc staticWalkDirImpl(path: string, relative: bool, checkDir: bool): PNode =
result = newNode(nkBracket)
for k, f in walkDir(path, relative):
for k, f in walkDir(path, relative, checkDir):
result.add newTree(nkTupleConstr, newIntNode(nkIntLit, k.ord),
newStrNode(nkStrLit, f))

Expand Down Expand Up @@ -179,8 +179,8 @@ proc registerAdditionalOps*(c: PCtx) =
wrap2si(readLines, ioop)
systemop getCurrentExceptionMsg
systemop getCurrentException
registerCallback c, "stdlib.*.staticWalkDir", proc (a: VmArgs) {.nimcall.} =
setResult(a, staticWalkDirImpl(getString(a, 0), getBool(a, 1)))
registerCallback c, "stdlib.*.staticWalkDir", mayRaise = true, callback = proc (a: VmArgs) {.nimcall.} =
setResult(a, staticWalkDirImpl(getString(a, 0), getBool(a, 1), getBool(a, 2)))
registerCallback c, "stdlib.compilesettings.querySetting", proc (a: VmArgs) {.nimcall.} =
setResult(a, querySettingImpl(a, c.config, getInt(a, 0)))
registerCallback c, "stdlib.compilesettings.querySettingSeq", proc (a: VmArgs) {.nimcall.} =
Expand Down
4 changes: 2 additions & 2 deletions compiler/wordrecg.nim
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type
wImportc, wImportJs, wExportc, wExportCpp, wExportNims, wIncompleteStruct, wRequiresInit,
wAlign, wNodecl, wPure, wSideEffect, wHeader,
wNoSideEffect, wGcSafe, wNoreturn, wNosinks, wMerge, wLib, wDynlib,
wCompilerProc, wCore, wProcVar, wBase, wUsed,
wCompilerProc, wCore, wInternalProc, wProcVar, wBase, wUsed,
wFatal, wError, wWarning, wHint, wLine, wPush, wPop, wDefine, wUndef,
wLineDir, wStackTrace, wLineTrace, wLink, wCompile,
wLinksys, wDeprecated, wVarargs, wCallconv, wDebugger,
Expand Down Expand Up @@ -129,7 +129,7 @@ const
"incompletestruct",
"requiresinit", "align", "nodecl", "pure", "sideeffect",
"header", "nosideeffect", "gcsafe", "noreturn", "nosinks", "merge", "lib", "dynlib",
"compilerproc", "core", "procvar", "base", "used",
"compilerproc", "core", "internalproc", "procvar", "base", "used",
"fatal", "error", "warning", "hint", "line",
"push", "pop", "define", "undef", "linedir", "stacktrace", "linetrace",
"link", "compile", "linksys", "deprecated", "varargs",
Expand Down
6 changes: 3 additions & 3 deletions lib/pure/os.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2024,7 +2024,7 @@ when defined(posix) and not weirdTarget:
else:
result = pcLinkToFile

proc staticWalkDir(dir: string; relative: bool): seq[
proc staticWalkDir(dir: string; relative: bool, checkDir: bool): seq[
tuple[kind: PathComponent, path: string]] =
discard

Expand Down Expand Up @@ -2060,11 +2060,11 @@ iterator walkDir*(dir: string; relative = false, checkDir = false):
## * `walkDirRec iterator <#walkDirRec.i,string>`_

when nimvm:
for k, v in items(staticWalkDir(dir, relative)):
for k, v in items(staticWalkDir(dir, relative, checkDir)):
yield (k, v)
else:
when weirdTarget:
for k, v in items(staticWalkDir(dir, relative)):
for k, v in items(staticWalkDir(dir, relative, checkDir)):
yield (k, v)
elif defined(windows):
var f: WIN32_FIND_DATA
Expand Down
23 changes: 22 additions & 1 deletion lib/system.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2224,7 +2224,6 @@ when notJSnotNims:
include "system/arithm"
{.pop.}


when not defined(js):
# this is a hack: without this when statement, you would get:
# Error: system module needs: nimGCvisit
Expand Down Expand Up @@ -3011,3 +3010,25 @@ export io

when not defined(createNimHcr):
include nimhcr

when defined(nimHasInternalProc):
## {.internalproc.} is analog to {.compilerproc.} but is used for semantic
## phase instead of codegen. It simplifies implementation by using nim code
## instead of building the AST by hand, while also avoiding making these
## public.
proc nimInternalNewException(name: string, msg: string, traceMsg: string) {.internalproc.} =
## internal proc for raising from vmpops
template fun(T) =
if name == astToStr(T):
var msg = msg
if traceMsg.len > 0:
# we could query for whether nim was build with --stacktrace to
# provide more accurate msg, but not sure if it's exposed.
msg.add "\nVM callback stacktrace (compile nim with --stacktrace if needed)\n" & traceMsg & "\n"
raise newException(T, msg)
# can add more hardcoded standard types here
fun(OSError)
fun(IOError)
fun(AssertionError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does thins mean that only OSError, IOError and AssertionError can be raised? Isn't there space to have a generic solution here that works on all error kinds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this bit needs improvement. Can't you just pass already created object and raise it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not possible to use compilerProc here then it needs to be moved into compiler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is horrible.

# if hits here, should be easy to handle
doAssert false, "`nimInternalNewException` does not yet handle " & name
4 changes: 4 additions & 0 deletions lib/system/excpt.nim
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,10 @@ proc `$`(s: seq[StackTraceEntry]): string =
elif s[i].line == reraisedFromEnd: result.add "]]\n"
else: addFrameEntry(result, s[i])

template toStrDefault*(s: seq[StackTraceEntry]): string =
## see also asyncfutures.`$`
$s

when hasSomeStackTrace:

proc auxWriteStackTrace(f: PFrame, s: var string) =
Expand Down
Loading