fixes #12899 (#12921)

* fixes #12899

* fixes regression: destroy global variables in reverse declaration order, closureleak test relies on it

Author: Araq

compiler/ccgexprs.nim

@@ -2031,13 +2031,15 @@ proc genDestroy(p: BProc; n: PNode) =
       var a: TLoc
       initLocExpr(p, arg, a)
       linefmt(p, cpsStmts, "if ($1.p && $1.p->allocator) {$n" &
-        " $1.p->allocator->dealloc($1.p->allocator, $1.p, $1.p->cap + 1 + sizeof(NI) + sizeof(void*)); }$n",
+        " $1.p->allocator->dealloc($1.p->allocator, $1.p, $1.p->cap + 1 + sizeof(NI) + sizeof(void*));$n" &
+        " $1.p = NIM_NIL; }$n",
         [rdLoc(a)])
     of tySequence:
       var a: TLoc
       initLocExpr(p, arg, a)
       linefmt(p, cpsStmts, "if ($1.p && $1.p->allocator) {$n" &
-        " $1.p->allocator->dealloc($1.p->allocator, $1.p, ($1.p->cap * sizeof($2)) + sizeof(NI) + sizeof(void*)); }$n",
+        " $1.p->allocator->dealloc($1.p->allocator, $1.p, ($1.p->cap * sizeof($2)) + sizeof(NI) + sizeof(void*));$n" &
+        " $1.p = NIM_NIL; }$n",
         [rdLoc(a), getTypeDesc(p.module, t.lastSon)])
     else: discard "nothing to do"
   else:

compiler/cgen.nim

@@ -1981,8 +1981,8 @@ proc myClose(graph: ModuleGraph; b: PPassContext, n: PNode): PNode =
   if b == nil: return
   var m = BModule(b)
   if sfMainModule in m.module.flags:
-    for destructorCall in graph.globalDestructors:
-      n.add destructorCall
+    for i in countdown(high(graph.globalDestructors), 0):
+      n.add graph.globalDestructors[i]
   if passes.skipCodegen(m.config, n): return
   if moduleHasChanged(graph, m.module):
     # if the module is cached, we don't regenerate the main proc

compiler/liftdestructors.nim

@@ -429,9 +429,7 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
     body.add genIf(c, cond, actions)
     body.add newAsgnStmt(x, y)
   of attachedDestructor:
-    when false:
-      # XXX investigate if this is necessary:
-      actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t))
+    actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t))
     body.add genIf(c, cond, actions)
   of attachedDeepCopy: assert(false, "cannot happen")
   of attachedTrace:
@@ -480,9 +478,7 @@ proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
     body.add genIf(c, cond, actions)
     body.add newAsgnStmt(x, y)
   of attachedDestructor:
-    when false:
-      # XXX investigate if this is necessary:
-      actions.add newAsgnStmt(xenv, newNodeIT(nkNilLit, body.info, xenv.typ))
+    actions.add newAsgnStmt(xenv, newNodeIT(nkNilLit, body.info, xenv.typ))
     body.add genIf(c, cond, actions)
   of attachedDeepCopy: assert(false, "cannot happen")
   of attachedTrace:
@@ -510,7 +506,10 @@ proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
   of attachedDestructor:
     # it's better to prepend the destruction of weak refs in order to
     # prevent wrong "dangling refs exist" problems:
-    let des = genIf(c, x, callCodegenProc(c.g, "nimDecWeakRef", c.info, x))
+    var actions = newNodeI(nkStmtList, c.info)
+    actions.add callCodegenProc(c.g, "nimDecWeakRef", c.info, x)
+    actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t))
+    let des = genIf(c, x, actions)
     if body.len == 0:
       body.add des
     else:
@@ -537,6 +536,7 @@ proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
     body.add genIf(c, x, actions)
     body.add newAsgnStmt(x, y)
   of attachedDestructor:
+    actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t))
     body.add genIf(c, x, actions)
   of attachedDeepCopy: assert(false, "cannot happen")
   of attachedTrace, attachedDispose: discard
@@ -567,7 +567,10 @@ proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
       body.add genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx))
       body.add newAsgnStmt(x, y)
     of attachedDestructor:
-      let des = genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx))
+      var actions = newNodeI(nkStmtList, c.info)
+      actions.add callCodegenProc(c.g, "nimDecWeakRef", c.info, xx)
+      actions.add newAsgnStmt(xx, newNodeIT(nkNilLit, body.info, xx.typ))
+      let des = genIf(c, xx, actions)
       if body.len == 0:
         body.add des
       else:
@@ -586,6 +589,7 @@ proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
     body.add genIf(c, xx, actions)
     body.add newAsgnStmt(x, y)
   of attachedDestructor:
+    actions.add newAsgnStmt(xx, newNodeIT(nkNilLit, body.info, xx.typ))
     body.add genIf(c, xx, actions)
   of attachedDeepCopy: assert(false, "cannot happen")
   of attachedTrace, attachedDispose: discard

tests/destructor/tnewruntime_strutils.nim

@@ -1,7 +1,9 @@
 discard """
   valgrind: true
   cmd: '''nim c --newruntime -d:useMalloc $file'''
-  output: '''422 422'''
+  output: '''
+@[(input: @["KXSC", "BGMC"]), (input: @["PXFX"]), (input: @["WXRQ", "ZSCZD"])]
+461 461'''
 """
 
 import strutils, os, std / wordwrap
@@ -13,6 +15,28 @@ import system / ansi_c
 proc retTuple(): (seq[int], int) =
   return (@[1], 1)
 
+# bug #12899
+
+import sequtils, strmisc
+
+const input = ["KXSC, BGMC => 7 PTHL", "PXFX => LBZJ", "WXRQ, ZSCZD => HLQM"]
+
+type
+  Reaction = object
+    input: seq[string]
+
+proc bug12899 =
+  var reactions: seq[Reaction] = @[]
+  for l in input:
+    let x = l.partition(" => ")
+    reactions.add Reaction(input: @(x[0].split(", ")))
+
+  let x = $reactions
+  echo x
+
+bug12899()
+
+
 proc nonStaticTests =
   doAssert formatBiggestFloat(1234.567, ffDecimal, -1) == "1234.567000"
   doAssert formatBiggestFloat(1234.567, ffDecimal, 0) == "1235" # bugs 8242, 12586

tests/gc/closureleak.nim

@@ -6,7 +6,7 @@ discard """
 type
   TFoo* = object
     id: int
-    fn: proc(){.closure.}
+    fn: proc() {.closure.}
 var foo_counter = 0
 var alive_foos = newseq[int](0)
 
@@ -31,7 +31,7 @@ proc newFoo*(): ref TFoo =
   inc foo_counter
 
 for i in 0 ..< 10:
- discard newFoo()
+  discard newFoo()
 
 for i in 0 ..< 10:
   let f = newFoo()