fixes #1796

lib/system/gc.nim

@@ -49,7 +49,7 @@ type
     waMarkGlobal,    # part of the backup/debug mark&sweep
     waMarkPrecise,   # part of the backup/debug mark&sweep
     waZctDecRef, waPush, waCycleDecRef, waMarkGray, waScan, waScanBlack, 
-    waCollectWhite,
+    waCollectWhite #, waDebug
 
   TFinalizer {.compilerproc.} = proc (self: pointer) {.nimcall, benign.}
     # A ref type can have a finalizer that is called before the object's
@@ -595,9 +595,15 @@ proc scan(s: PCell) =
     else:
       s.setColor(rcWhite)
       forAllChildren(s, waScan)
-  
+
 proc collectWhite(s: PCell) =
-  if s.color == rcWhite and s notin gch.cycleRoots:
+  # This is a hacky way to deal with the following problem (bug #1796)
+  # Consider this content in cycleRoots:
+  #   x -> a; y -> a  where 'a' is an acyclic object so not included in
+  # cycleRoots itself. Then 'collectWhite' used to free 'a' twice. The
+  # 'isAllocatedPtr' check prevents this. This also means we do not need
+  # to query 's notin gch.cycleRoots' at all.
+  if isAllocatedPtr(gch.region, s) and s.color == rcWhite:
     s.setColor(rcBlack)
     forAllChildren(s, waCollectWhite)
     freeCyclicCell(gch, s)
@@ -648,6 +654,28 @@ when useMarkForDebug or useBackupGc:
       if objStart != nil:
         markS(gch, objStart)
 
+when logGC:
+  var
+    cycleCheckA: array[100, PCell]
+    cycleCheckALen = 0
+
+  proc alreadySeen(c: PCell): bool =
+    for i in 0 .. <cycleCheckALen:
+      if cycleCheckA[i] == c: return true
+    if cycleCheckALen == len(cycleCheckA):
+      gcAssert(false, "cycle detection overflow")
+      quit 1
+    cycleCheckA[cycleCheckALen] = c
+    inc cycleCheckALen
+
+  proc debugGraph(s: PCell) =
+    if alreadySeen(s):
+      writeCell("child cell (already seen) ", s)
+    else:
+      writeCell("cell {", s)
+      forAllChildren(s, waDebug)
+      c_fprintf(c_stdout, "}\n")
+
 proc doOperation(p: pointer, op: TWalkOp) =
   if p == nil: return
   var c: PCell = usrToCell(p)
@@ -690,6 +718,7 @@ proc doOperation(p: pointer, op: TWalkOp) =
   of waMarkPrecise:
     when useMarkForDebug or useBackupGc:
       add(gch.tempStack, c)
+  #of waDebug: debugGraph(c)
 
 proc nimGCvisit(d: pointer, op: int) {.compilerRtl.} =
   doOperation(d, TWalkOp(op))
@@ -702,7 +731,6 @@ when useMarkForDebug or useBackupGc:
 
 proc collectRoots(gch: var TGcHeap) =
   for s in elements(gch.cycleRoots):
-    excl(gch.cycleRoots, s)
     collectWhite(s)
 
 proc collectCycles(gch: var TGcHeap) =

tests/gc/closureleak.nim

@@ -7,7 +7,7 @@ from strutils import join
 type
   TFoo * = object
     id: int
-    func: proc(){.closure.}
+    fn: proc(){.closure.}
 var foo_counter = 0
 var alive_foos = newseq[int](0)
 
@@ -26,7 +26,7 @@ for i in 0 .. <10:
 
 for i in 0 .. <10:
   let f = newFoo()
-  f.func = proc = 
+  f.fn = proc = 
     echo f.id
 
 GC_fullcollect()

tests/gc/cyclecollector.nim

@@ -0,0 +1,21 @@
+
+# Program to detect bug #1796 reliably
+
+type
+  Node = ref object
+    a, b: Node
+    leaf: string
+
+proc createCycle(leaf: string): Node =
+  new result
+  result.a = result
+  shallowCopy result.leaf, leaf
+
+proc main =
+  for i in 0 .. 100_000:
+    var leaf = "this is the leaf. it allocates"
+    let x = createCycle(leaf)
+    let y = createCycle(leaf)
+  echo "done ", getOccupiedMem()
+
+main()

tests/gc/gctest.nim

@@ -196,7 +196,8 @@ write(stdout, "starting main...\n")
 main()
 
 GC_fullCollect()
+# the M&S GC fails with this call and it's unclear why. Definitely something
+# we need to fix!
 GC_fullCollect()
 writeln(stdout, GC_getStatistics())
 write(stdout, "finished\n")
-

tests/testament/categories.nim

@@ -107,12 +107,15 @@ proc dllTests(r: var TResults, cat: Category, options: string) =
 # ------------------------------ GC tests -------------------------------------
 
 proc gcTests(r: var TResults, cat: Category, options: string) =
-  template test(filename: expr): stmt =
+  template testWithoutMs(filename: expr): stmt =
     testSpec r, makeTest("tests/gc" / filename, options, cat, actionRun)
     testSpec r, makeTest("tests/gc" / filename, options &
                   " -d:release", cat, actionRun)
     testSpec r, makeTest("tests/gc" / filename, options &
                   " -d:release -d:useRealtimeGC", cat, actionRun)
+
+  template test(filename: expr): stmt =
+    testWithoutMs filename
     testSpec r, makeTest("tests/gc" / filename, options &
                   " --gc:markAndSweep", cat, actionRun)
     testSpec r, makeTest("tests/gc" / filename, options &
@@ -124,13 +127,15 @@ proc gcTests(r: var TResults, cat: Category, options: string) =
   test "gctest"
   test "gcleak3"
   test "gcleak4"
-  test "gcleak5"
+  # Disabled because it works and takes too long to run:
+  #test "gcleak5"
   test "weakrefs"
   test "cycleleak"
   test "closureleak"
-  test "refarrayleak"
+  testWithoutMs "refarrayleak"
-  test "stackrefleak"
   
+  test "stackrefleak"
+  test "cyclecollector"
 
 # ------------------------- threading tests -----------------------------------