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

timotheecour · 2020-03-21T08:51:53Z

before PR

VM callbacks can't be caught, so code behaves differently at RT vs CT if VM code calls a callback that raises. eg:

catch doesn't work
on crash, no useful context or stacktrace is shown so we have no idea how we got here

after PR

VM callbacks can be caught, all that's needed is an extra arg registerCallback(.., mayRaise=true)
the exception (whether caught in regular code or if there's no handler, when the program crashes with stacktrace), contains the relevant context
stacktrace inside VM callback is also shown when nim itself is compiled with --stacktrace:on
nimscript implementation of callbacks is now simplified thanks to this and shows more informative stacktraces, eg when nim is built with --stacktrace:on
so things like walkDir(..,checkDir=true), writeFile, removeDir etc now work correctly in VM
fixes Nimscript listFiles should throw exception when path is not found #12676 (listFiles(..,checkDir=true) + friends now work; I did not change the default semantic to be consistent with walkDir)
a new way to define internal procs is defined ({.internalproc.}), analog to {.compilerproc.}, but for semantic phase instead of codegen. This enables writing handlers in pure nim code instead of having to manually write AST; this not only simplifies writing compiler code, but it also allows change those handlers without having to recompile nim. A key aspect is that these are private (such symbols don't need to be exported).

example

proc fun1() = writeFile("/tmp/nonexistant/bar.txt", "foo")
proc fun2()=fun1()
static: fun2()
# note: try/catch would also work inside VM after this PR (unlike before PR)

before PR: no context shown, hard to debug in complex programs

/Users/timothee/git_clone/nim/Nim_devel/lib/system/io.nim(706) writeFile
Error: unhandled exception: cannot open: /tmp/nonexistant/bar.txt [IOError]

after PR:

stack trace: (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375c.nim(93, 15) t10375c
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375c.nim(92, 19) fun2
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375c.nim(91, 26) fun1
/Users/timothee/git_clone/nim/Nim_prs/lib/system.nim(3024, 9) nimInternalNewException
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375c.nim(1, 1) template/generic instantiation from here
/Users/timothee/git_clone/nim/Nim_prs/lib/system.nim(3024, 9) Error: unhandled exception: cannot open: /tmp/nonexistant/bar.txt
VM callback stacktrace (compile nim with --stacktrace if needed)
/Users/timothee/git_clone/nim/Nim_prs/lib/system/io.nim(706) writeFile

 [IOError]
          raise newException(T, msg)
          ^

stacktrace inside VM callback

furthermore, when nim itself is compiled with --stacktace, the msg will also show the VM stacktrace (truncated to the useful part) eg:

stack trace: (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375b.nims(18, 7)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375b.nims(17, 20) fun2
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10375b.nims(16, 7) fun
/Users/timothee/git_clone/nim/Nim_prs/lib/system/nimscript.nim(325, 16) cd
/Users/timothee/git_clone/nim/Nim_prs/lib/system.nim(3023, 9) nimInternalNewException
/Users/timothee/git_clone/nim/Nim_prs/lib/system.nim(3023, 9) Error: unhandled exception: No such file or directory
Additional info: "nonexistant"
VM callback stacktrace
/Users/timothee/git_clone/nim/Nim_prs/compiler/vm.nim(1231) rawExecute
/Users/timothee/git_clone/nim/Nim_prs/compiler/scriptconfig.nim(88) :anonymous
/Users/timothee/git_clone/nim/Nim_prs/lib/pure/os.nim(1367) setCurrentDir
/Users/timothee/git_clone/nim/Nim_prs/lib/pure/includes/oserr.nim(94) raiseOSError

 [OSError]
          raise newException(T, msg)
          ^

TODO for future PR's

after this PR, it should be easy to catch C++ exceptions from CT FFI
handle similarly opcNDynBindSym

links

using readFile at compile time break exceptions - Nim forum

timotheecour · 2020-03-21T09:41:24Z

compiler/vm.nim

@@ -540,6 +540,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]) =


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

krux02 · 2020-03-23T02:02:33Z

I can't find the test case.

timotheecour · 2020-03-23T04:52:48Z

done PTAL

Clyybber · 2020-03-23T10:29:39Z

lib/system.nim

+    # can add more hardcoded standard types here
+    fun(OSError)
+    fun(IOError)
+    fun(AssertionError)


This is horrible.

krux02

You are adding a new compiler feature: internalproc. Please make a separate PR for that feature alone. You can reference this PR there. I think that feature should be reviewed and tested independently.

This is not a full review yet. I just skimmed the diff.

compiler/pragmas.nim

krux02 · 2020-03-23T10:27:18Z

lib/system.nim

+    # can add more hardcoded standard types here
+    fun(OSError)
+    fun(IOError)
+    fun(AssertionError)


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?

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

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

cooldome · 2020-04-09T13:45:57Z

I have reviewed this PR and really like it.
Only two comment I have:

I don't think you need to introduce wInternalProc pragma, you can use compilerProc pragma and
proc getCompilerProc*(g: ModuleGraph; name: string): PSym from magicsys instead.
You have introduced argument mayRaise that use needs to be populated by the user. I would consider checkings {.raises[].} effect of symbol proc type instead and automatically infer if wrapped proc can raise exception or not.

cooldome · 2020-04-09T13:49:57Z

I have checked again and i think it safe to assume that wrapped proc can always raise exception, little overhead is added for potential exception handling.

cooldome · 2020-04-09T13:59:23Z

lib/system.nim

+    # can add more hardcoded standard types here
+    fun(OSError)
+    fun(IOError)
+    fun(AssertionError)


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

cooldome · 2020-04-09T14:01:44Z

lib/system.nim

+    # can add more hardcoded standard types here
+    fun(OSError)
+    fun(IOError)
+    fun(AssertionError)


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

cooldome · 2020-04-09T14:02:42Z

compiler/pragmas.nim

@@ -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,


Important to avoid introducing new pragmas

Araq · 2020-04-09T21:57:57Z

The addition to system.nim is terrible and the feature itself is of questionable value. Yes, I know you will not stop until everything from os.nim is available in the VM, but I don't like it and the existing mechanism might suffice. But as others remarked the worst part is the new pragma and the system.nim additions neither of which are required afaict.

stale · 2021-04-10T04:12:24Z

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

stale · 2022-09-08T23:52:04Z

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

timotheecour changed the title ~~[WIP] VM callbacks can now raise (and caught, or show stacktrace, in VM code)~~ [WIP] VM callbacks can now raise (and be caught, or show their stacktrace) Mar 21, 2020

timotheecour mentioned this pull request Mar 21, 2020

Nimscript listFiles should throw exception when path is not found #12676

Open

timotheecour commented Mar 21, 2020

View reviewed changes

timotheecour changed the title ~~[WIP] VM callbacks can now raise (and be caught, or show their stacktrace)~~ VM callbacks can now raise (and be caught, or show their stacktrace) Mar 22, 2020

timotheecour force-pushed the pr_fix_12676_nimscript branch from 72948e3 to 6ea4da8 Compare March 23, 2020 05:33

Clyybber reviewed Mar 23, 2020

View reviewed changes

lib/system.nim

# can add more hardcoded standard types here

fun(OSError)

fun(IOError)

fun(AssertionError)

Copy link

Contributor

Clyybber Mar 23, 2020

Choose a reason for hiding this comment

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

This is horrible.

krux02 suggested changes Mar 23, 2020

View reviewed changes

timotheecour added 11 commits March 31, 2020 02:46

vmcallbacks can now raise (and propagate correctly)

7ba7afa

fixup

9f14c71

fix nimInternalNewException

b0fba13

scriptconfig part 1

caa5ed5

_

ff76bd1

simplify nimscript code; make writeFile catchable

7a7d877

closes nim-lang#12676: allwo: listFiles(.., checkDir=true)

317de6e

fix test

d38481a

fixup

16b78f4

add tests

65368f8

use Ident id for style insensitive matches, not the pointers

6117e08

timotheecour force-pushed the pr_fix_12676_nimscript branch from 6ea4da8 to 6117e08 Compare March 31, 2020 09:57

cooldome requested changes Apr 9, 2020

View reviewed changes

stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 10, 2021

timotheecour removed the stale Staled PR/issues; remove the label after fixing them label Apr 10, 2021

timotheecour marked this pull request as draft April 10, 2021 06:35

timotheecour mentioned this pull request Jul 23, 2021

delEnv now works at CT #18568

Merged

stale bot added the stale Staled PR/issues; remove the label after fixing them label Sep 8, 2022

stale bot closed this Oct 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timotheecour commented Mar 21, 2020 •

edited

Loading

timotheecour Mar 21, 2020 •

edited

Loading

krux02 commented Mar 23, 2020

timotheecour commented Mar 23, 2020

Clyybber Mar 23, 2020

krux02 left a comment

krux02 Mar 23, 2020

cooldome Apr 9, 2020

cooldome Apr 9, 2020

cooldome commented Apr 9, 2020

cooldome commented Apr 9, 2020 •

edited

Loading

cooldome Apr 9, 2020

cooldome Apr 9, 2020

cooldome Apr 9, 2020

Araq commented Apr 9, 2020

stale bot commented Apr 10, 2021

stale bot commented Sep 8, 2022

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

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

Conversation

timotheecour commented Mar 21, 2020 • edited Loading

before PR

after PR

example

stacktrace inside VM callback

TODO for future PR's

links

timotheecour Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

krux02 commented Mar 23, 2020

timotheecour commented Mar 23, 2020

Clyybber Mar 23, 2020

Choose a reason for hiding this comment

krux02 left a comment

Choose a reason for hiding this comment

krux02 Mar 23, 2020

Choose a reason for hiding this comment

cooldome Apr 9, 2020

Choose a reason for hiding this comment

cooldome Apr 9, 2020

Choose a reason for hiding this comment

cooldome commented Apr 9, 2020

cooldome commented Apr 9, 2020 • edited Loading

cooldome Apr 9, 2020

Choose a reason for hiding this comment

cooldome Apr 9, 2020

Choose a reason for hiding this comment

cooldome Apr 9, 2020

Choose a reason for hiding this comment

Araq commented Apr 9, 2020

stale bot commented Apr 10, 2021

stale bot commented Sep 8, 2022

timotheecour commented Mar 21, 2020 •

edited

Loading

timotheecour Mar 21, 2020 •

edited

Loading

cooldome commented Apr 9, 2020 •

edited

Loading