Merge nim-works#645

645: make `goto` exceptions the default r=saem a=zerbina

## Summary

This is a preparation for the removal of the `setjmp` and `quirky` exceptions modes. `goto` exceptions were already the default when using `--gc:arc|orc`.

* fix the programs crashing when a thread was created and both thread- local storage emulation and `--exceptions:goto` were enabled
* fix execution continuing after a call to an RVO-using closure when using `--exceptions:goto`
* fix an issue where assignments were observable if the right-hand expression is a call that raised an exception
* remove the `segfaults` standard library module; its current implementation is incompatible with anything besides setjmp-exceptions
* `--exceptions:native` now means `--exceptions:goto` when using the C target

There's currently an issue with the ordering of routines in the `system` module, which prevents `nimrtl` from compiling when using `--exceptions:goto`.

## Details

The infinite recursion error that occurred when using emulated thread- local storage is caused by the procedure for accessing emulated threadvars (`GetThreadLocalVars`) getting an error flag query (read, access of a threadvar) injected, now that `system` procedures are no longer treated as never raising a defect.

The intermediate solution: disable error flag injection for all `.compilerproc`s. As a consequence, exceptional control-flow inside their  immediate bodies is no longer possible. In order to still support raise-dispatcher-like compilerprocs (e.g. `raiseFloatOverflow`), an unconditional return is inserted after the call to a `noreturn` procedure when inside a procedure that has the error flag disabled.

In addition, special-case the `threadProcWrapper` system procedure to also have the the error flag (and thus exceptional control-flow) disabled, as it is responsible for setting up the emulated thread-local storage.

### Standard Library

The `segfaults` module implicitly relied on `setjmp`-exceptions, as only `longjmp` is able to leave the signal handler and enter the NimSkull exception handler without additional support from the code generator.

Since it would become non-functional with the planned removal of `setjmp`-exceptions, it is removed already.



Co-authored-by: zerbina <[email protected]>

compiler/backend/ccgcalls.nim

@@ -71,6 +71,21 @@ proc isHarmlessStore(p: BProc; canRaise: bool; d: TLoc): bool =
   else:
     result = false
 
+proc exitCall(p: BProc, callee: PNode, canRaise: bool) =
+  ## Emits the exceptional control-flow related post-call logic.
+  if p.config.exc == excGoto:
+    if nimErrorFlagDisabled in p.flags:
+      if callee.kind == nkSym and sfNoReturn in callee.sym.flags and
+         canRaiseConservative(callee):
+        # when using goto-exceptions, noreturn doesn't map to "doesn't return"
+        # at the C-level. In order to still support dispatching to wrapper
+        # procedures around ``raise`` from inside ``.compilerprocs``, we emit
+        # an exit after the call
+        p.flags.incl beforeRetNeeded
+        lineF(p, cpsStmts, "goto BeforeRet_;$n", [])
+    elif canRaise:
+      raiseExit(p)
+
 proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
                callee, params: Rope) =
   let canRaise = p.config.exc == excGoto and canRaiseDisp(p, ri[0])
@@ -91,14 +106,15 @@ proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
         pl.add(addrLoc(p.config, d))
         pl.add(~");$n")
         line(p, cpsStmts, pl)
+        exitCall(p, ri[0], canRaise)
       else:
         var tmp: TLoc
         getTemp(p, typ[0], tmp, needsInit=true)
         pl.add(addrLoc(p.config, tmp))
         pl.add(~");$n")
         line(p, cpsStmts, pl)
+        exitCall(p, ri[0], canRaise)
         genAssignment(p, d, tmp, {}) # no need for deep copying
-      if canRaise: raiseExit(p)
     else:
       pl.add(~")")
       if isHarmlessStore(p, canRaise, d):
@@ -108,20 +124,20 @@ proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
         initLoc(list, locCall, d.lode, OnUnknown)
         list.r = pl
         genAssignment(p, d, list, {}) # no need for deep copying
-        if canRaise: raiseExit(p)
+        exitCall(p, ri[0], canRaise)
       else:
         var tmp: TLoc
         getTemp(p, typ[0], tmp, needsInit=true)
         var list: TLoc
         initLoc(list, locCall, d.lode, OnUnknown)
         list.r = pl
         genAssignment(p, tmp, list, {}) # no need for deep copying
-        if canRaise: raiseExit(p)
+        exitCall(p, ri[0], canRaise)
         genAssignment(p, d, tmp, {})
   else:
     pl.add(~");$n")
     line(p, cpsStmts, pl)
-    if canRaise: raiseExit(p)
+    exitCall(p, ri[0], canRaise)
 
 proc genBoundsCheck(p: BProc; arr, a, b: TLoc)
 
@@ -418,12 +434,13 @@ proc genClosureCall(p: BProc, le, ri: PNode, d: var TLoc) =
           discard "resetLoc(p, d)"
         pl.add(addrLoc(p.config, d))
         genCallPattern()
+        exitCall(p, ri[0], canRaise)
       else:
         var tmp: TLoc
         getTemp(p, typ[0], tmp, needsInit=true)
         pl.add(addrLoc(p.config, tmp))
         genCallPattern()
-        if canRaise: raiseExit(p)
+        exitCall(p, ri[0], canRaise)
         genAssignment(p, d, tmp, {}) # no need for deep copying
     elif isHarmlessStore(p, canRaise, d):
       if d.k == locNone: getTemp(p, typ[0], d)
@@ -435,7 +452,7 @@ proc genClosureCall(p: BProc, le, ri: PNode, d: var TLoc) =
       else:
         list.r = PatProc % [rdLoc(op), pl, pl.addComma, rawProc]
       genAssignment(p, d, list, {}) # no need for deep copying
-      if canRaise: raiseExit(p)
+      exitCall(p, ri[0], canRaise)
     else:
       var tmp: TLoc
       getTemp(p, typ[0], tmp)
@@ -447,11 +464,11 @@ proc genClosureCall(p: BProc, le, ri: PNode, d: var TLoc) =
       else:
         list.r = PatProc % [rdLoc(op), pl, pl.addComma, rawProc]
       genAssignment(p, tmp, list, {})
-      if canRaise: raiseExit(p)
+      exitCall(p, ri[0], canRaise)
       genAssignment(p, d, tmp, {})
   else:
     genCallPattern()
-    if canRaise: raiseExit(p)
+    exitCall(p, ri[0], canRaise)
 
 proc notYetAlive(n: PNode): bool {.inline.} =
   let r = getRoot(n)

compiler/backend/cgen.nim

@@ -1063,6 +1063,18 @@ proc genProcAux(m: BModule, prc: PSym) =
         #incl(res.loc.flags, lfIndirect)
         res.loc.storage = OnUnknown
 
+  # for now, we treat all compilerprocs as being able to run in a boot
+  # environment where the error flag is not yet accessible. This is not quite
+  # correct, and a dedicated facility for designating runtime procedures as
+  # usable in a boot environment is eventually required
+  # The ``threadProcWrapper`` is special-cased to have the flag disabled too,
+  # as thread-local storage might not have been set up when the flag is first
+  # queried. Making it a compilerproc is not possible, due to it being a
+  # generic routine
+  if sfCompilerProc in prc.flags or (prc.name.s == "threadProcWrapper" and
+     sfSystemModule in getModule(prc).flags):
+    p.flags.incl nimErrorFlagDisabled
+
   for i in 1..<prc.typ.n.len:
     let param = prc.typ.n[i].sym
     if param.typ.isCompileTimeOnly: continue

compiler/front/main.nim

@@ -317,7 +317,9 @@ proc mainCommand*(graph: ModuleGraph) =
     defineSymbol(graph.config, $conf.backend)
     case conf.backend
     of backendC:
-      if conf.exc == excNone: conf.exc = excSetjmp
+      case conf.exc
+      of excNone, excNative:            conf.exc = excGoto
+      of excQuirky, excSetjmp, excGoto: discard
     of backendJs, backendNimVm:
       discard
     of backendInvalid: doAssert false

doc/lib.rst

@@ -462,9 +462,6 @@ Miscellaneous
 * `logging <logging.html>`_
   This module implements a simple logger.
 
-* `segfaults <segfaults.html>`_
-  Turns access violations or segfaults into a `NilAccessDefect` exception.
-
 * `sugar <sugar.html>`_
   This module implements nice syntactic sugar based on Nim's macro system.
 

lib/system/chcks.nim

@@ -32,7 +32,7 @@ proc raiseFieldError(f: string) {.compilerproc, noinline.} =
 when defined(nimV2):
   proc raiseFieldError2(f: string, discVal: int) {.compilerproc, noinline.} =
     ## raised when field is inaccessible given runtime value of discriminant
-    sysFatal(FieldError, f & $discVal & "'")
+    sysFatal(FieldError, formatFieldDefect(f, $discVal))
 else:
   proc raiseFieldError2(f: string, discVal: string) {.compilerproc, noinline.} =
     ## raised when field is inaccessible given runtime value of discriminant

lib/system/exceptions.nim

@@ -141,8 +141,6 @@ type
     ## Raised if it is attempted to send a message to a dead thread.
   NilAccessDefect* = object of Defect ## \
     ## Raised on dereferences of `nil` pointers.
-    ##
-    ## This is only raised if the `segfaults module <segfaults.html>`_ was imported!
 
   ArithmeticError* {.deprecated: "See corresponding Defect".} = ArithmeticDefect
   DivByZeroError* {.deprecated: "See corresponding Defect".} = DivByZeroDefect

lib/system/indexerrors.nim

@@ -1,15 +1,23 @@
 # imported by other modules, unlike helpers.nim which is included
 # xxx this is now included instead of imported, we should import instead
 
+# NOTE: these routines are used by compilerprocs raising exceptions, which
+# currently cannot rely on finalizers (both explicit and implicit). In other
+# words, the routines here need to make sure that they don't inject any local
+# or temporary that requires destruction.
+
+proc formatErrorIndexBoundStr(i, a, b: sink string): string =
+  "index " & $i & " not in " & $a & " .. " & $b
+
 template formatErrorIndexBound*[T](i, a, b: T): string =
   when defined(standalone):
     "indexOutOfBounds"
   else:
     if b < a: "index out of bounds, the container is empty"
-    else: "index " & $i & " not in " & $a & " .. " & $b
+    else: formatErrorIndexBoundStr($i, $a, $b)
 
 template formatErrorIndexBound*[T](i, n: T): string =
   formatErrorIndexBound(i, 0, n)
 
-template formatFieldDefect*(f, discVal): string =
+proc formatFieldDefect*(f, discVal: sink string): string =
   f & discVal & "'"

lib/system/threads.nim

@@ -181,6 +181,8 @@ template threadProcWrapperBody(closure: untyped): untyped =
   deallocShared(cast[pointer](core))
 
 {.push stack_trace:off.}
+# NOTE: the `threadProcWrapper` is currently special-cased by the compiler to
+# not access the error flag
 when defined(windows):
   proc threadProcWrapper[TArg](closure: pointer): int32 {.stdcall.} =
     threadProcWrapperBody(closure)

testament/categories.nim

@@ -46,9 +46,12 @@ proc runBasicDLLTest(c, r: var TResults, cat: Category, options: string) =
     else:
       ""
 
-  var test1 = makeTest("lib/nimrtl.nim", options & " --outdir:tests/dll", cat)
-  test1.spec.action = actionCompile
-  testSpec c, test1
+  # XXX: nimrtl is currently defunct and the tests making use of it are
+  #      disabled
+  when false:
+    var test1 = makeTest("lib/nimrtl.nim", options & " --outdir:tests/dll", cat)
+    test1.spec.action = actionCompile
+    testSpec c, test1
   var test2 = makeTest("tests/dll/server.nim", options & " --threads:on" & rpath, cat)
   test2.spec.action = actionCompile
   testSpec c, test2

tests/ccgbugs/tgotoexceptions.nim

@@ -0,0 +1,27 @@
+discard """
+  targets: c
+  description: '''
+    Regression tests related for C code-generator issues related to
+    goto-exceptions
+  '''
+"""
+
+block missing_exception_control_flow:
+  # no if-error-goto-exit was injected after the call to a closure procedure
+  # using RVO. At the time of writing, arrays always use RVO when using the C
+  # target.
+  proc outer(p: ptr int): array[4, int] =
+    proc inner(): array[4, int] =
+      discard p # capture something
+      raise CatchableError.newException("")
+
+    result = inner()
+    p[] = 1 # `p` is nil; the assignment must never be reached
+
+  var caught = false
+  try:
+    discard outer(nil)
+  except CatchableError:
+    caught = true
+
+  doAssert caught

tests/ccgbugs/tmissingvolatile.nim

@@ -1,6 +1,6 @@
 discard """
   output: "1"
-  cmd: r"nim c --hints:on $options -d:release $file"
+  matrix: "--exceptions:setjmp -d:release"
   ccodecheck: "'NI volatile state;'"
   targets: "c"
 """

tests/dll/client.nim

@@ -1,5 +1,6 @@
 discard """
   cmd: "nim $target --debuginfo --hints:on --define:useNimRtl $options $file"
+  knownIssue: "nimrtl doesn't build with --exceptions:goto and/or --gc:arc|orc"
 """
 
 type

tests/exception/tassignment.nim

@@ -0,0 +1,26 @@
+discard """
+  targets: "c js vm"
+  description: '''
+    Tests for assignments where an exception is raised during evaluation of
+    the right-hand side
+  '''
+"""
+
+block unobservable_rvo_assignment:
+  # assigning the result of an RVO-using call to a variable that is also used
+  # as one of the arguments must not be oversable when the call raises an
+  # exception
+  proc raiseEx(x: array[4, int]): array[4, int] =
+    result[0] = 1
+    raise CatchableError.newException("")
+
+  proc test() =
+    var x: array[4, int]
+    try:
+      x = raiseEx(x)
+    except CatchableError:
+      doAssert x[0] == 0, "handler observed changed value"
+
+    doAssert x[0] == 0, "following statement observed changed value"
+
+  test()

tests/js/tstdlib_imports.nim

@@ -65,7 +65,6 @@ import std/[
   # Miscellaneous:
   colors, logging, sugar, unittest, varints, with,
   # fails due to FFI: browsers
-  # works but uses FFI: segfaults
 
   # Modules for JS backend:
   asyncjs, dom, jsconsole, jscore, jsffi, jsbigints,

tests/pragmas/tnoreturn.nim

@@ -1,5 +1,4 @@
 discard """
-ccodeCheck: "\\i @'__attribute__((noreturn))' .*"
 action: compile
 """