fixes #13881

* fixes #13881
* documented changed requirements for system.onThreadDestruction
* destructors.rst: update the documentation

9 files changed, 84 insertions, 23 deletions

diff --git a/changelog.md b/changelog.md
index 9bf372dc4..af6217324 100644
--- a/changelog.md
+++ b/changelog.md
@@ -89,6 +89,9 @@
 - Fix a bug where calling `close` on io streams in osproc.startProcess was a noop and led to
   hangs if a process had both reads from stdin and writes (eg to stdout).
 
+- The callback that is passed to `system.onThreadDestruction` must now be `.raises: []`.
+
+
 ## Language changes
 - In the newruntime it is now allowed to assign discriminator field without restrictions as long as case object doesn't have custom destructor. Discriminator value doesn't have to be a constant either. If you have custom destructor for case object and you do want to freely assign discriminator fields, it is recommended to refactor object into 2 objects like this:
 
diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
index 5333e639c..7b05faa87 100644
--- a/compiler/condsyms.nim
+++ b/compiler/condsyms.nim
@@ -117,3 +117,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimNewIntegerOps")
   defineSymbol("nimHasInvariant")
   defineSymbol("nimHasStacktraceMsgs")
+  defineSymbol("nimDoesntTrackDefects")
diff --git a/doc/destructors.rst b/doc/destructors.rst
index 2b1222778..e7ebd93ea 100644
--- a/doc/destructors.rst
+++ b/doc/destructors.rst
@@ -469,10 +469,51 @@ for expressions of type ``lent T`` or of type ``var T``.
     echo t[0] # accessor does not copy the element!
 
 
+The .cursor annotation
+======================
+
+Under the ``--gc:arc|orc`` modes Nim's `ref` type is implemented via the same runtime
+"hooks" and thus via reference counting. This means that cyclic structures cannot be freed
+immediately (``--gc:orc`` ships with a cycle collector). With the ``.cursor`` annotation
+one can break up cycles declaratively:
+
+.. code-block:: nim
+
+  type
+    Node = ref object
+      left: Node # owning ref
+      right {.cursor.}: Node # non-owning ref
+
+But please notice that this is not C++'s weak_ptr, it means the right field is not
+involved in the reference counting, it is a raw pointer without runtime checks.
+
+Automatic reference counting also has the disadvantage that it introduces overhead
+when iterating over linked structures. The ``.cursor`` annotation can also be used
+to avoid this overhead:
+
+.. code-block:: nim
+
+  var it {.cursor.} = listRoot
+  while it != nil:
+    use(it)
+    it = it.next
+
+
+In fact, ``.cursor`` more generally prevents object construction/destruction pairs
+and so can also be useful in other contexts. The alternative solution would be to
+use raw pointers (``ptr``) instead which is more cumbersome and also more dangerous
+for Nim's evolution: Later on the compiler can try to prove ``.cursor`` annotations
+to be safe, but for ``ptr`` the compiler has to remain silent about possible
+problems.
+
 
 Owned refs
 ==========
 
+**Note**: The ``owned`` type constructor is only available with
+the ``--newruntime`` compiler switch and is experimental.
+
+
 Let ``W`` be an ``owned ref`` type. Conceptually its hooks look like:
 
 .. code-block:: nim
@@ -568,14 +609,14 @@ used to specialize the object traversal in order to avoid deep recursions:
 
   type Node = ref object
     x, y: int32
-    left, right: owned Node
+    left, right: Node
 
   type Tree = object
-    root: owned Node
+    root: Node
 
   proc `=destroy`(t: var Tree) {.nodestroy.} =
     # use an explicit stack so that we do not get stack overflows:
-    var s: seq[owned Node] = @[t.root]
+    var s: seq[Node] = @[t.root]
     while s.len > 0:
       let x = s.pop
       if x.left != nil: s.add(x.left)
diff --git a/lib/system.nim b/lib/system.nim
index 8878277b7..8ce79aa2f 100644
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -692,7 +692,7 @@ proc len*(x: string): int {.magic: "LengthStr", noSideEffect.}
 proc len*(x: cstring): int {.magic: "LengthStr", noSideEffect.}
   ## Returns the length of a compatible string. This is sometimes
   ## an O(n) operation.
-  ## 
+  ##
   ## **Note:** On the JS backend this currently counts UTF-16 code points
   ## instead of bytes at runtime (not at compile time). For now, if you
   ## need the byte length of the UTF-8 encoding, convert to string with
@@ -2093,7 +2093,7 @@ when not defined(js):
 
   when hasAlloc:
     when not defined(gcRegions) and not usesDestructors:
-      proc initGC() {.gcsafe.}
+      proc initGC() {.gcsafe, raises: [].}
 
     proc initStackBottom() {.inline, compilerproc.} =
       # WARNING: This is very fragile! An array size of 8 does not work on my
diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim
index bbb890cd4..fd1a73b2d 100644
--- a/lib/system/excpt.nim
+++ b/lib/system/excpt.nim
@@ -493,6 +493,14 @@ proc reraiseException() {.compilerRtl.} =
     else:
       raiseExceptionAux(currException)
 
+proc threadTrouble() =
+  # also forward declared, it is 'raises: []' hence the try-except.
+  try:
+    if currException != nil: reportUnhandledError(currException)
+  except:
+    discard
+  quit 1
+
 proc writeStackTrace() =
   when hasSomeStackTrace:
     var s = ""
diff --git a/lib/system/gc.nim b/lib/system/gc.nim
index 2be16ef90..304eda19a 100644
--- a/lib/system/gc.nim
+++ b/lib/system/gc.nim
@@ -843,9 +843,10 @@ when not defined(useNimRtl):
   proc GC_disable() =
     inc(gch.recGcLock)
   proc GC_enable() =
-    if gch.recGcLock <= 0:
-      raise newException(AssertionDefect,
-          "API usage error: GC_enable called but GC is already enabled")
+    when defined(nimDoesntTrackDefects):
+      if gch.recGcLock <= 0:
+        raise newException(AssertionDefect,
+            "API usage error: GC_enable called but GC is already enabled")
     dec(gch.recGcLock)
 
   proc GC_setStrategy(strategy: GC_Strategy) =
diff --git a/lib/system/gc_interface.nim b/lib/system/gc_interface.nim
index 9183e6d48..84145f33a 100644
--- a/lib/system/gc_interface.nim
+++ b/lib/system/gc_interface.nim
@@ -14,7 +14,7 @@ when hasAlloc:
       gcOptimizeSpace    ## optimize for memory footprint
 
 when hasAlloc and not defined(js) and not usesDestructors:
-  proc GC_disable*() {.rtl, inl, benign.}
+  proc GC_disable*() {.rtl, inl, benign, raises: [].}
     ## Disables the GC. If called `n` times, `n` calls to `GC_enable`
     ## are needed to reactivate the GC.
     ##
@@ -22,7 +22,7 @@ when hasAlloc and not defined(js) and not usesDestructors:
     ## the mark and sweep phase with
     ## `GC_disableMarkAndSweep <#GC_disableMarkAndSweep>`_.
 
-  proc GC_enable*() {.rtl, inl, benign.}
+  proc GC_enable*() {.rtl, inl, benign, raises: [].}
     ## Enables the GC again.
 
   proc GC_fullCollect*() {.rtl, benign.}
@@ -54,7 +54,7 @@ when hasAlloc and not defined(js) and not usesDestructors:
   proc GC_unref*(x: string) {.magic: "GCunref", benign.}
     ## See the documentation of `GC_ref <#GC_ref,string>`_.
 
-  proc nimGC_setStackBottom*(theStackBottom: pointer) {.compilerRtl, noinline, benign.}
+  proc nimGC_setStackBottom*(theStackBottom: pointer) {.compilerRtl, noinline, benign, raises: [].}
     ## Expands operating GC stack range to `theStackBottom`. Does nothing
       ## if current stack bottom is already lower than `theStackBottom`.
 
diff --git a/lib/system/gc_ms.nim b/lib/system/gc_ms.nim
index eda220901..0b9745f1e 100644
--- a/lib/system/gc_ms.nim
+++ b/lib/system/gc_ms.nim
@@ -495,9 +495,10 @@ when not defined(useNimRtl):
   proc GC_disable() =
     inc(gch.recGcLock)
   proc GC_enable() =
-    if gch.recGcLock <= 0:
-      raise newException(AssertionDefect,
-          "API usage error: GC_enable called but GC is already enabled")
+    when defined(nimDoesntTrackDefects):
+      if gch.recGcLock <= 0:
+        raise newException(AssertionDefect,
+            "API usage error: GC_enable called but GC is already enabled")
     dec(gch.recGcLock)
 
   proc GC_setStrategy(strategy: GC_Strategy) = discard
diff --git a/lib/system/threads.nim b/lib/system/threads.nim
index b3a4e32e4..34a2d2ccf 100644
--- a/lib/system/threads.nim
+++ b/lib/system/threads.nim
@@ -89,9 +89,9 @@ type
       data: TArg
 
 var
-  threadDestructionHandlers {.rtlThreadVar.}: seq[proc () {.closure, gcsafe.}]
+  threadDestructionHandlers {.rtlThreadVar.}: seq[proc () {.closure, gcsafe, raises: [].}]
 
-proc onThreadDestruction*(handler: proc () {.closure, gcsafe.}) =
+proc onThreadDestruction*(handler: proc () {.closure, gcsafe, raises: [].}) =
   ## Registers a *thread local* handler that is called at the thread's
   ## destruction.
   ##
@@ -105,7 +105,10 @@ template afterThreadRuns() =
     threadDestructionHandlers[i]()
 
 when not defined(boehmgc) and not hasSharedHeap and not defined(gogc) and not defined(gcRegions):
-  proc deallocOsPages() {.rtl.}
+  proc deallocOsPages() {.rtl, raises: [].}
+
+proc threadTrouble() {.raises: [].}
+  ## defined in system/excpt.nim
 
 when defined(boehmgc):
   type GCStackBaseProc = proc(sb: pointer, t: pointer) {.noconv.}
@@ -116,7 +119,7 @@ when defined(boehmgc):
   proc boehmGC_unregister_my_thread()
     {.importc: "GC_unregister_my_thread", boehmGC.}
 
-  proc threadProcWrapDispatch[TArg](sb: pointer, thrd: pointer) {.noconv.} =
+  proc threadProcWrapDispatch[TArg](sb: pointer, thrd: pointer) {.noconv, raises: [].} =
     boehmGC_register_my_thread(sb)
     try:
       let thrd = cast[ptr Thread[TArg]](thrd)
@@ -124,11 +127,13 @@ when defined(boehmgc):
         thrd.dataFn()
       else:
         thrd.dataFn(thrd.data)
+    except:
+      threadTrouble()
     finally:
       afterThreadRuns()
     boehmGC_unregister_my_thread()
 else:
-  proc threadProcWrapDispatch[TArg](thrd: ptr Thread[TArg]) =
+  proc threadProcWrapDispatch[TArg](thrd: ptr Thread[TArg]) {.raises: [].} =
     try:
       when TArg is void:
         thrd.dataFn()
@@ -139,21 +144,22 @@ else:
           var x: TArg
           deepCopy(x, thrd.data)
           thrd.dataFn(x)
+    except:
+      threadTrouble()
     finally:
       afterThreadRuns()
 
-proc threadProcWrapStackFrame[TArg](thrd: ptr Thread[TArg]) =
+proc threadProcWrapStackFrame[TArg](thrd: ptr Thread[TArg]) {.raises: [].} =
   when defined(boehmgc):
     boehmGC_call_with_stack_base(threadProcWrapDispatch[TArg], thrd)
   elif not defined(nogc) and not defined(gogc) and not defined(gcRegions) and not usesDestructors:
-    var p {.volatile.}: proc(a: ptr Thread[TArg]) {.nimcall, gcsafe.} =
-      threadProcWrapDispatch[TArg]
+    var p {.volatile.}: pointer
     # init the GC for refc/markandsweep
     nimGC_setStackBottom(addr(p))
     initGC()
     when declared(threadType):
       threadType = ThreadType.NimThread
-    p(thrd)
+    threadProcWrapDispatch[TArg](thrd)
     when declared(deallocOsPages): deallocOsPages()
   else:
     threadProcWrapDispatch(thrd)