scoped memory management (#14790)

* fixes the regressions
* closes #13936
* scope based memory management implemented
* enabled tcontrolflow.nim test case
* final cleanups

compiler/ccgcalls.nim

@@ -644,7 +644,33 @@ proc genNamedParamCall(p: BProc, ri: PNode, d: var TLoc) =
     pl.add(~"];$n")
     line(p, cpsStmts, pl)
 
+proc notYetAlive(n: PNode): bool {.inline.} =
+  let r = getRoot(n)
+  result = r != nil and r.loc.lode == nil
+
+proc isInactiveDestructorCall(p: BProc, e: PNode): bool =
+  #[ Consider this example.
+
+    var :tmpD_3281815
+    try:
+      if true:
+        return
+      let args_3280013 =
+        wasMoved_3281816(:tmpD_3281815)
+        `=_3280036`(:tmpD_3281815, [1])
+        :tmpD_3281815
+    finally:
+      `=destroy_3280027`(args_3280013)
+
+  We want to return early but the 'finally' section is traversed before
+  the 'let args = ...' statement. We exploit this to generate better
+  code for 'return'. ]#
+  result = e.len == 2 and e[0].kind == nkSym and
+    e[0].sym.name.s == "=destroy" and notYetAlive(e[1].skipAddr)
+
 proc genCall(p: BProc, e: PNode, d: var TLoc) =
+  if p.withinBlockLeaveActions > 0 and isInactiveDestructorCall(p, e):
+    return
   if e[0].typ.skipTypes({tyGenericInst, tyAlias, tySink, tyOwned}).callConv == ccClosure:
     genClosureCall(p, nil, e, d)
   elif e[0].kind == nkSym and sfInfixCall in e[0].sym.flags:

compiler/ccgexprs.nim

@@ -2066,10 +2066,14 @@ proc skipAddr(n: PNode): PNode =
 
 proc genWasMoved(p: BProc; n: PNode) =
   var a: TLoc
-  initLocExpr(p, n[1].skipAddr, a)
-  resetLoc(p, a)
-  #linefmt(p, cpsStmts, "#nimZeroMem((void*)$1, sizeof($2));$n",
-  #  [addrLoc(p.config, a), getTypeDesc(p.module, a.t)])
+  let n1 = n[1].skipAddr
+  if p.withinBlockLeaveActions > 0 and notYetAlive(n1):
+    discard
+  else:
+    initLocExpr(p, n1, a)
+    resetLoc(p, a)
+    #linefmt(p, cpsStmts, "#nimZeroMem((void*)$1, sizeof($2));$n",
+    #  [addrLoc(p.config, a), getTypeDesc(p.module, a.t)])
 
 proc genMove(p: BProc; n: PNode; d: var TLoc) =
   var a: TLoc
@@ -2593,10 +2597,12 @@ proc expr(p: BProc, n: PNode, d: var TLoc) =
       else:
         putLocIntoDest(p, d, sym.loc)
     of skTemp:
-      if sym.loc.r == nil:
-        # we now support undeclared 'skTemp' variables for easier
-        # transformations in other parts of the compiler:
-        assignLocalVar(p, n)
+      when false:
+        # this is more harmful than helpful.
+        if sym.loc.r == nil:
+          # we now support undeclared 'skTemp' variables for easier
+          # transformations in other parts of the compiler:
+          assignLocalVar(p, n)
       if sym.loc.r == nil or sym.loc.t == nil:
         #echo "FAILED FOR PRCO ", p.prc.name.s
         #echo renderTree(p.prc.ast, {renderIds})

compiler/ccgstmts.nim

@@ -200,6 +200,7 @@ proc blockLeaveActions(p: BProc, howManyTrys, howManyExcepts: int) =
 
   var stack = newSeq[tuple[fin: PNode, inExcept: bool, label: Natural]](0)
 
+  inc p.withinBlockLeaveActions
   for i in 1..howManyTrys:
     let tryStmt = p.nestedTryStmts.pop
     if p.config.exc == excSetjmp:
@@ -217,6 +218,8 @@ proc blockLeaveActions(p: BProc, howManyTrys, howManyExcepts: int) =
     if finallyStmt != nil:
       genStmts(p, finallyStmt[0])
 
+  dec p.withinBlockLeaveActions
+
   # push old elements again:
   for i in countdown(howManyTrys-1, 0):
     p.nestedTryStmts.add(stack[i])
@@ -861,10 +864,10 @@ proc genStringCase(p: BProc, t: PNode, d: var TLoc) =
     genCaseGeneric(p, t, d, "", "if (#eqStrings($1, $2)) goto $3;$n")
 
 proc branchHasTooBigRange(b: PNode): bool =
-  for i in 0..<b.len-1:
+  for it in b:
     # last son is block
-    if (b[i].kind == nkRange) and
-        b[i][1].intVal - b[i][0].intVal > RangeExpandLimit:
+    if (it.kind == nkRange) and
+        it[1].intVal - it[0].intVal > RangeExpandLimit:
       return true
 
 proc ifSwitchSplitPoint(p: BProc, n: PNode): int =
@@ -988,9 +991,14 @@ proc genTryCpp(p: BProc, t: PNode, d: var TLoc) =
   let fin = if t[^1].kind == nkFinally: t[^1] else: nil
   p.nestedTryStmts.add((fin, false, 0.Natural))
 
-  startBlock(p, "try {$n")
-  expr(p, t[0], d)
-  endBlock(p)
+  if t.kind == nkHiddenTryStmt:
+    lineCg(p, cpsStmts, "try {$n", [])
+    expr(p, t[0], d)
+    lineCg(p, cpsStmts, "}$n", [])
+  else:
+    startBlock(p, "try {$n")
+    expr(p, t[0], d)
+    endBlock(p)
 
   # First pass: handle Nim based exceptions:
   lineCg(p, cpsStmts, "catch (#Exception* T$1_) {$n", [etmp+1])
@@ -1335,13 +1343,13 @@ proc genTrySetjmp(p: BProc, t: PNode, d: var TLoc) =
       linefmt(p, cpsStmts, "$1.status = _setjmp($1.context);$n", [safePoint])
     else:
       linefmt(p, cpsStmts, "$1.status = setjmp($1.context);$n", [safePoint])
-    startBlock(p, "if ($1.status == 0) {$n", [safePoint])
+    lineCg(p, cpsStmts, "if ($1.status == 0) {$n", [safePoint])
   let fin = if t[^1].kind == nkFinally: t[^1] else: nil
   p.nestedTryStmts.add((fin, quirkyExceptions, 0.Natural))
   expr(p, t[0], d)
   if not quirkyExceptions:
     linefmt(p, cpsStmts, "#popSafePoint();$n", [])
-    endBlock(p)
+    lineCg(p, cpsStmts, "}$n", [])
     startBlock(p, "else {$n")
     linefmt(p, cpsStmts, "#popSafePoint();$n", [])
     genRestoreFrameAfterException(p)

compiler/cgen.nim

@@ -1561,7 +1561,7 @@ proc registerModuleToMain(g: BModuleList; m: BModule) =
   if sfSystemModule in m.module.flags:
     if emulatedThreadVars(m.config) and m.config.target.targetOS != osStandalone:
       g.mainDatInit.add(ropecg(m, "\t#initThreadVarsEmulation();$N", []))
-    if m.config.target.targetOS != osStandalone and m.config.selectedGC != gcNone:
+    if m.config.target.targetOS != osStandalone and m.config.selectedGC notin {gcNone, gcArc, gcOrc}:
       g.mainDatInit.add(ropecg(m, "\t#initStackBottomWith((void *)&inner);$N", []))
 
   if m.s[cfsInitProc].len > 0:
@@ -1666,6 +1666,10 @@ proc genInitCode(m: BModule) =
     writeSection(preInitProc, cpsInit, m.hcrOn)
     writeSection(preInitProc, cpsStmts)
     prc.addf("}$N", [])
+    when false:
+      m.initProc.blocks[0].sections[cpsLocals].add m.preInitProc.s(cpsLocals)
+      m.initProc.blocks[0].sections[cpsInit].prepend m.preInitProc.s(cpsInit)
+      m.initProc.blocks[0].sections[cpsStmts].prepend m.preInitProc.s(cpsStmts)
 
   # add new scope for following code, because old vcc compiler need variable
   # be defined at the top of the block

compiler/cgendata.nim

@@ -97,6 +97,7 @@ type
                               # requires 'T x = T()' to become 'T x; x = T()'
                               # (yes, C++ is weird like that)
     withinTryWithExcept*: int # required for goto based exception handling
+    withinBlockLeaveActions*: int # complex to explain
     sigConflicts*: CountTable[string]
 
   TTypeSeq* = seq[PType]

compiler/semstmts.nim

@@ -229,8 +229,8 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags): PNode =
         a[0][2] = newSymNode(symbol, a[0][2].info)
 
       elif a.len == 1:
-          # count number of ``except: body`` blocks
-          inc catchAllExcepts
+        # count number of ``except: body`` blocks
+        inc catchAllExcepts
 
       else:
         # support ``except KeyError, ValueError, ... : body``

compiler/spawn.nim

@@ -10,7 +10,7 @@
 ## This module implements threadpool's ``spawn``.
 
 import ast, types, idents, magicsys, msgs, options, modulegraphs,
-  lowerings, liftdestructors
+  lowerings, liftdestructors, renderer
 from trees import getMagic, getRoot
 
 proc callProc(a: PNode): PNode =
@@ -321,7 +321,7 @@ proc wrapProcForSpawn*(g: ModuleGraph; owner: PSym; spawnExpr: PNode; retType: P
     result = newNodeI(nkStmtList, n.info)
 
   if n.kind notin nkCallKinds:
-    localError(g.config, n.info, "'spawn' takes a call expression")
+    localError(g.config, n.info, "'spawn' takes a call expression; got " & $n)
     return
   if optThreadAnalysis in g.config.globalOptions:
     if {tfThread, tfNoSideEffect} * n[0].typ.flags == {}:

doc/destructors.rst

@@ -283,8 +283,8 @@ Rewrite rules
    around the complete routine body.
 2. The produced ``finally`` section is wrapped around the enclosing scope.
 
-The current implementation follows strategy (1). This means that resources are
-not destroyed at the scope exit, but at the proc exit.
+The current implementation follows strategy (2). This means that resources are
+destroyed at the scope exit.
 
 ::
 

tests/arc/tarcmisc.nim

@@ -173,3 +173,39 @@ proc bug14495 =
     echo o[]
 
 bug14495()
+
+when false:
+  # bug #14396
+  type
+    Spinny = ref object
+      t: ref int
+      text: string
+
+  proc newSpinny*(): Spinny =
+    Spinny(t: new(int), text: "hello")
+
+  proc spinnyLoop(x: ref int, spinny: sink Spinny) =
+    echo x[]
+
+  proc start*(spinny: sink Spinny) =
+    spinnyLoop(spinny.t, spinny)
+
+  var spinner1 = newSpinny()
+  spinner1.start()
+
+  # bug #14345
+
+  type
+    SimpleLoopB = ref object
+      children: seq[SimpleLoopB]
+      parent: SimpleLoopB
+
+  proc addChildLoop(self: SimpleLoopB, loop: SimpleLoopB) =
+    self.children.add loop
+
+  proc setParent(self: SimpleLoopB, parent: SimpleLoopB) =
+    self.parent = parent
+    self.parent.addChildLoop(self)
+
+  var l = SimpleLoopB()
+  l.setParent(l)

tests/arc/tcontrolflow.nim

@@ -11,15 +11,13 @@ begin true
 if
 end true
 7
+##index 2 not in 0 .. 1##
 '''
   cmd: "nim c --gc:arc -d:danger $file"
-  disabled: "true"
 """
 # we use the -d:danger switch to detect uninitialized stack
 # slots more reliably (there shouldn't be any, of course).
 
-# XXX Enable once scope based destruction works!
-
 type
   Foo = object
     id: int
@@ -68,12 +66,25 @@ proc run(data: Control) =
   evt.control = data
   if evt.button == 1:
     discard
-  else: 
+  else:
     return
-  
+
   echo data.x
 
 var c = Control(x: 7)
 
 run(c)
 
+proc sysFatal(exceptn: typedesc, message: string) {.inline, noreturn.} =
+  var buf = newStringOfCap(200)
+  add(buf, "##")
+  add(buf, message)
+  add(buf, "##")
+  echo buf
+
+proc ifexpr(i, a, b: int) {.compilerproc, noinline.} =
+  sysFatal(IndexDefect,
+    if b < a: "index out of bounds, the container is empty"
+    else: "index " & $i & " not in " & $a & " .. " & $b)
+
+ifexpr(2, 0, 1)

tests/arc/tmovebug.nim

@@ -247,7 +247,7 @@ iterator combinations[T](s: openarray[T], k: int): seq[T] =
       break
 
 type
-  UndefEx = object of Exception
+  UndefEx = object of ValueError
 
 proc main2 =
   var delayedSyms = @[1, 2, 3]
@@ -318,10 +318,10 @@ proc `=sink`(dest: var O2, src: O2) =
 
 var testSeq: O2
 
-proc Update(): void =
+proc update() =
   # testSeq.add(0) # uncommenting this line fixes the leak
   testSeq = O2(s: @[])
   testSeq.s.add(0)
 
 for i in 1..3:
-  Update()
+  update()

tests/arc/tthread.nim

@@ -14,7 +14,7 @@ type
     p: int
   MyObjRef = ref MyObj
 
-proc `=destroy`(x: var MyObj) = 
+proc `=destroy`(x: var MyObj) =
   if x.p != 0:
     echo "destroyed"
 
@@ -48,16 +48,16 @@ proc thread5(x: sink MyObjRef): MyObjRef =
   os.sleep(1000)
   result = x
 
-proc ref_forwarding_test = 
+proc ref_forwarding_test =
   var x = new(MyObj)
   x[].p = 2
   var y = spawn thread4(x)
- 
-proc ref_sink_forwarding_test = 
+
+proc ref_sink_forwarding_test =
   var x = new(MyObj)
   x[].p = 2
   var y = spawn thread5(x)
 
 ref_forwarding_test()
-ref_sink_forwarding_test()  
+ref_sink_forwarding_test()
 sync()

tests/arc/tweave.nim

@@ -0,0 +1,154 @@
+discard """
+  outputsub: '''Success'''
+  cmd: '''nim c --gc:arc --threads:on $file'''
+  disabled: "bsd"
+"""
+
+# bug #13936
+
+import std/atomics
+
+const MemBlockSize = 256
+
+type
+  ChannelSPSCSingle* = object
+    full{.align: 128.}: Atomic[bool]
+    itemSize*: uint8
+    buffer*{.align: 8.}: UncheckedArray[byte]
+
+proc `=`(
+    dest: var ChannelSPSCSingle,
+    source: ChannelSPSCSingle
+  ) {.error: "A channel cannot be copied".}
+
+proc initialize*(chan: var ChannelSPSCSingle, itemsize: SomeInteger) {.inline.} =
+  ## If ChannelSPSCSingle is used intrusive another data structure
+  ## be aware that it should be the last part due to ending by UncheckedArray
+  ## Also due to 128 bytes padding, it automatically takes half
+  ## of the default MemBlockSize
+  assert itemsize.int in 0 .. int high(uint8)
+  assert itemSize.int +
+          sizeof(chan.itemsize) +
+          sizeof(chan.full) < MemBlockSize
+
+  chan.itemSize = uint8 itemsize
+  chan.full.store(false, moRelaxed)
+
+func isEmpty*(chan: var ChannelSPSCSingle): bool {.inline.} =
+  not chan.full.load(moAcquire)
+
+func tryRecv*[T](chan: var ChannelSPSCSingle, dst: var T): bool {.inline.} =
+  ## Try receiving the item buffered in the channel
+  ## Returns true if successful (channel was not empty)
+  ##
+  ## ⚠ Use only in the consumer thread that reads from the channel.
+  assert (sizeof(T) == chan.itemsize.int) or
+          # Support dummy object
+          (sizeof(T) == 0 and chan.itemsize == 1)
+
+  let full = chan.full.load(moAcquire)
+  if not full:
+    return false
+  dst = cast[ptr T](chan.buffer.addr)[]
+  chan.full.store(false, moRelease)
+  return true
+
+func trySend*[T](chan: var ChannelSPSCSingle, src: sink T): bool {.inline.} =
+  ## Try sending an item into the channel
+  ## Reurns true if successful (channel was empty)
+  ##
+  ## ⚠ Use only in the producer thread that writes from the channel.
+  assert (sizeof(T) == chan.itemsize.int) or
+          # Support dummy object
+          (sizeof(T) == 0 and chan.itemsize == 1)
+
+  let full = chan.full.load(moAcquire)
+  if full:
+    return false
+  cast[ptr T](chan.buffer.addr)[] = src
+  chan.full.store(true, moRelease)
+  return true
+
+# Sanity checks
+# ------------------------------------------------------------------------------
+when isMainModule:
+
+  when not compileOption("threads"):
+    {.error: "This requires --threads:on compilation flag".}
+
+  template sendLoop[T](chan: var ChannelSPSCSingle,
+                       data: sink T,
+                       body: untyped): untyped =
+    while not chan.trySend(data):
+      body
+
+  template recvLoop[T](chan: var ChannelSPSCSingle,
+                       data: var T,
+                       body: untyped): untyped =
+    while not chan.tryRecv(data):
+      body
+
+  type
+    ThreadArgs = object
+      ID: WorkerKind
+      chan: ptr ChannelSPSCSingle
+
+    WorkerKind = enum
+      Sender
+      Receiver
+
+  template Worker(id: WorkerKind, body: untyped): untyped {.dirty.} =
+    if args.ID == id:
+      body
+
+  proc thread_func(args: ThreadArgs) =
+
+    # Worker RECEIVER:
+    # ---------
+    # <- chan
+    # <- chan
+    # <- chan
+    #
+    # Worker SENDER:
+    # ---------
+    # chan <- 42
+    # chan <- 53
+    # chan <- 64
+    Worker(Receiver):
+      var val: int
+      for j in 0 ..< 10:
+        args.chan[].recvLoop(val):
+          # Busy loop, in prod we might want to yield the core/thread timeslice
+          discard
+        echo "                  Receiver got: ", val
+        doAssert val == 42 + j*11
+
+    Worker(Sender):
+      doAssert args.chan.full.load(moRelaxed) == false
+      for j in 0 ..< 10:
+        let val = 42 + j*11
+        args.chan[].sendLoop(val):
+          # Busy loop, in prod we might want to yield the core/thread timeslice
+          discard
+        echo "Sender sent: ", val
+
+  proc main() =
+    echo "Testing if 2 threads can send data"
+    echo "-----------------------------------"
+    var threads: array[2, Thread[ThreadArgs]]
+
+    var chan = cast[ptr ChannelSPSCSingle](allocShared(MemBlockSize))
+    chan[].initialize(itemSize = sizeof(int))
+
+    createThread(threads[0], thread_func, ThreadArgs(ID: Receiver, chan: chan))
+    createThread(threads[1], thread_func, ThreadArgs(ID: Sender, chan: chan))
+
+    joinThread(threads[0])
+    joinThread(threads[1])
+
+    freeShared(chan)
+
+    echo "-----------------------------------"
+    echo "Success"
+
+  main()

tests/async/testmanyasyncevents.nim

@@ -1,8 +1,9 @@
 discard """
 output: '''
 hasPendingOperations: false
-triggerCount: 100 
+triggerCount: 100
 '''
+disabled: "windows"
 """
 
 import asyncDispatch

tests/casestmt/t12785.nim

@@ -3,6 +3,8 @@ discard """
   output: '''copied
 copied
 2
+destroyed
+destroyed
 copied
 copied
 2
@@ -37,9 +39,9 @@ proc test(a: range[0..1], arg: ObjWithDestructor) =
     inc iteration
 
     case a
-    of 0:  
-        assert false
-    of 1:      
+    of 0:
+      assert false
+    of 1:
       echo b
       if iteration == 2:
         break

tests/destructor/tdestructor.nim

@@ -1,27 +1,27 @@
 discard """
-  output: '''----
+  output: '''----1
 myobj constructed
 myobj destroyed
-----
+----2
 mygeneric1 constructed
 mygeneric1 destroyed
-----
+----3
 mygeneric2 constructed
 mygeneric2 destroyed
 myobj destroyed
-----
+----4
 mygeneric3 constructed
 mygeneric1 destroyed
-----
+----5
 mydistinctObj constructed
 myobj destroyed
 mygeneric2 destroyed
-------------------
-----
-----
-myobj destroyed
+------------------8
 mygeneric1 destroyed
----
+----6
+myobj destroyed
+----7
+---9
 myobj destroyed
 myobj destroyed
 '''
@@ -114,19 +114,19 @@ proc mydistinctObj =
 
   echo "mydistinctObj constructed"
 
-echo "----"
+echo "----1"
 myobj()
 
-echo "----"
+echo "----2"
 mygeneric1()
 
-echo "----"
+echo "----3"
 mygeneric2[int](10)
 
-echo "----"
+echo "----4"
 mygeneric3()
 
-echo "----"
+echo "----5"
 mydistinctObj()
 
 proc caseobj =
@@ -134,16 +134,16 @@ proc caseobj =
     var o1 = TCaseObj(kind: A, x: TMyGeneric1[int](x: 10))
 
   block:
-    echo "----"
+    echo "----6"
     var o2 = TCaseObj(kind: B, y: open())
 
   block:
-    echo "----"
+    echo "----7"
     var o3 = TCaseObj(kind: D, innerKind: B, r: "test",
                       p: TMyGeneric3[int, float, string](x: 10, y: 1.0, z: "test"))
 
 
-echo "------------------"
+echo "------------------8"
 caseobj()
 
 proc caseobj_test_sink: TCaseObj =
@@ -153,7 +153,7 @@ proc caseobj_test_sink: TCaseObj =
   result = TCaseObj(kind: B, y: open())
 
 
-echo "---"
+echo "---9"
 discard caseobj_test_sink()
 
 # issue #14315