ARC: misc bugfixes (#13156)

* fixes #13102
* closes #13149
* ARC: fixes a move optimizer bug (there are more left regarding array and tuple indexing)
* proper fix; fixes #12957
* fixes yet another case object '=' code generation problem

Author: Araq

compiler/dfa.nim

@@ -597,7 +597,7 @@ proc genUse(c: var Con; orig: PNode) =
   if n.kind == nkSym and n.sym.kind in InterestingSyms:
     c.code.add Instr(n: orig, kind: use, sym: if orig != n: nil else: n.sym)
 
-proc aliases(obj, field: PNode): bool =
+proc aliases*(obj, field: PNode): bool =
   var n = field
   var obj = obj
   while obj.kind in {nkHiddenSubConv, nkHiddenStdConv, nkObjDownConv, nkObjUpConv}:
@@ -646,8 +646,14 @@ proc isAnalysableFieldAccess*(orig: PNode; owner: PSym): bool =
   while true:
     case n.kind
     of nkDotExpr, nkCheckedFieldExpr, nkHiddenSubConv, nkHiddenStdConv,
-       nkObjDownConv, nkObjUpConv, nkHiddenAddr, nkAddr, nkBracketExpr:
+       nkObjDownConv, nkObjUpConv, nkHiddenAddr, nkAddr:
       n = n[0]
+    of nkBracketExpr:
+      # in a[i] the 'i' must be known
+      if n.len > 1 and n[1].kind in {nkCharLit..nkUInt64Lit}:
+        n = n[0]
+      else:
+        return false
     of nkHiddenDeref, nkDerefExpr:
       # We "own" sinkparam[].loc but not ourVar[].location as it is a nasty
       # pointer indirection.

compiler/injectdestructors.nim

@@ -673,7 +673,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
     if isUnpackedTuple(ri[0]):
       # unpacking of tuple: take over elements
       result = newTree(nkFastAsgn, dest, p(ri, c, consumed))
-    elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c):
+    elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
+        not aliases(dest, ri):
       # Rule 3: `=sink`(x, z); wasMoved(z)
       var snk = genSink(c, dest, ri)
       snk.add ri

compiler/liftdestructors.nim

@@ -16,6 +16,8 @@
 import modulegraphs, lineinfos, idents, ast, renderer, semdata,
   sighashes, lowerings, options, types, msgs, magicsys, tables
 
+from trees import isCaseObj
+
 type
   TLiftCtx = object
     g: ModuleGraph
@@ -62,28 +64,45 @@ proc defaultOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
   if c.kind in {attachedAsgn, attachedDeepCopy, attachedSink}:
     body.add newAsgnStmt(x, y)
 
-proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode) =
+proc genAddr(g: ModuleGraph; x: PNode): PNode =
+  if x.kind == nkHiddenDeref:
+    checkSonsLen(x, 1, g.config)
+    result = x[0]
+  else:
+    result = newNodeIT(nkHiddenAddr, x.info, makeVarType(x.typ.owner, x.typ))
+    result.add x
+
+proc destructorCall(g: ModuleGraph; op: PSym; x: PNode): PNode =
+  result = newNodeIT(nkCall, x.info, op.typ[0])
+  result.add(newSymNode(op))
+  result.add genAddr(g, x)
+
+proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode; enforceDefaultOp: bool) =
   case n.kind
   of nkSym:
     let f = n.sym
     let b = if c.kind == attachedTrace: y else: y.dotField(f)
-    if sfCursor in f.flags and f.typ.skipTypes(abstractInst).kind in {tyRef, tyProc} and
-        c.g.config.selectedGC in {gcArc, gcOrc, gcHooks}:
+    if (sfCursor in f.flags and f.typ.skipTypes(abstractInst).kind in {tyRef, tyProc} and
+        c.g.config.selectedGC in {gcArc, gcOrc, gcHooks}) or
+        enforceDefaultOp:
       defaultOp(c, f.typ, body, x.dotField(f), b)
     else:
       fillBody(c, f.typ, body, x.dotField(f), b)
   of nkNilLit: discard
   of nkRecCase:
-    if c.kind in {attachedSink, attachedAsgn, attachedDeepCopy}:
+    # XXX This is only correct for 'attachedSink'!
+    var localEnforceDefaultOp = enforceDefaultOp
+    if c.kind == attachedSink:
       ## the value needs to be destroyed before we assign the selector
       ## or the value is lost
       let prevKind = c.kind
       c.kind = attachedDestructor
-      fillBodyObj(c, n, body, x, y)
+      fillBodyObj(c, n, body, x, y, enforceDefaultOp = false)
       c.kind = prevKind
+      localEnforceDefaultOp = true
 
     # copy the selector:
-    fillBodyObj(c, n[0], body, x, y)
+    fillBodyObj(c, n[0], body, x, y, enforceDefaultOp = false)
     # we need to generate a case statement:
     var caseStmt = newNodeI(nkCaseStmt, c.info)
     # XXX generate 'if' that checks same branches
@@ -96,28 +115,69 @@ proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode) =
       var branch = copyTree(n[i])
       branch[^1] = newNodeI(nkStmtList, c.info)
 
-      fillBodyObj(c, n[i].lastSon, branch[^1], x, y)
+      fillBodyObj(c, n[i].lastSon, branch[^1], x, y,
+                  enforceDefaultOp = localEnforceDefaultOp)
       if branch[^1].len == 0: inc emptyBranches
       caseStmt.add(branch)
     if emptyBranches != n.len-1:
       body.add(caseStmt)
   of nkRecList:
-    for t in items(n): fillBodyObj(c, t, body, x, y)
+    for t in items(n): fillBodyObj(c, t, body, x, y, enforceDefaultOp)
   else:
     illFormedAstLocal(n, c.g.config)
 
-proc fillBodyObjT(c: var TLiftCtx; t: PType, body, x, y: PNode) =
+proc fillBodyObjTImpl(c: var TLiftCtx; t: PType, body, x, y: PNode) =
   if t.len > 0 and t[0] != nil:
-    fillBodyObjT(c, skipTypes(t[0], abstractPtrs), body, x, y)
-  fillBodyObj(c, t.n, body, x, y)
+    fillBodyObjTImpl(c, skipTypes(t[0], abstractPtrs), body, x, y)
+  fillBodyObj(c, t.n, body, x, y, enforceDefaultOp = false)
 
-proc genAddr(g: ModuleGraph; x: PNode): PNode =
-  if x.kind == nkHiddenDeref:
-    checkSonsLen(x, 1, g.config)
-    result = x[0]
+proc fillBodyObjT(c: var TLiftCtx; t: PType, body, x, y: PNode) =
+  var hasCase = isCaseObj(t.n)
+  var obj = t
+  while obj.len > 0 and obj[0] != nil:
+    obj = skipTypes(obj[0], abstractPtrs)
+    hasCase = hasCase or isCaseObj(obj.n)
+
+  if hasCase and c.kind in {attachedAsgn, attachedDeepCopy}:
+    # assignment for case objects is complex, we do:
+    # =destroy(dest)
+    # wasMoved(dest)
+    # for every field:
+    #   `=` dest.field, src.field
+    # ^ this is what we used to do, but for 'result = result.sons[0]' it
+    # destroys 'result' too early.
+    # So this is what we really need to do:
+    # let blob {.cursor.} = dest # remembers the old dest.kind
+    # wasMoved(dest)
+    # dest.kind = src.kind
+    # for every field (dependent on dest.kind):
+    #   `=` dest.field, src.field
+    # =destroy(blob)
+    var temp = newSym(skTemp, getIdent(c.g.cache, lowerings.genPrefix), c.fn, c.info)
+    temp.typ = x.typ
+    incl(temp.flags, sfFromGeneric)
+    var v = newNodeI(nkVarSection, c.info)
+    let blob = newSymNode(temp)
+    v.addVar(blob, x)
+    body.add v
+    #body.add newAsgnStmt(blob, x)
+
+    var wasMovedCall = newNodeI(nkCall, c.info)
+    wasMovedCall.add(newSymNode(createMagic(c.g, "wasMoved", mWasMoved)))
+    wasMovedCall.add x # mWasMoved does not take the address
+    body.add wasMovedCall
+
+    fillBodyObjTImpl(c, t, body, x, y)
+    when false:
+      # does not work yet due to phase-ordering problems:
+      assert t.destructor != nil
+      body.add destructorCall(c.g, t.destructor, blob)
+    let prevKind = c.kind
+    c.kind = attachedDestructor
+    fillBodyObjTImpl(c, t, body, blob, y)
+    c.kind = prevKind
   else:
-    result = newNodeIT(nkHiddenAddr, x.info, makeVarType(x.typ.owner, x.typ))
-    result.add x
+    fillBodyObjTImpl(c, t, body, x, y)
 
 proc newHookCall(g: ModuleGraph; op: PSym; x, y: PNode): PNode =
   #if sfError in op.flags:
@@ -136,11 +196,6 @@ proc newOpCall(op: PSym; x: PNode): PNode =
   result.add(newSymNode(op))
   result.add x
 
-proc destructorCall(g: ModuleGraph; op: PSym; x: PNode): PNode =
-  result = newNodeIT(nkCall, x.info, op.typ[0])
-  result.add(newSymNode(op))
-  result.add genAddr(g, x)
-
 proc newDeepCopyCall(op: PSym; x, y: PNode): PNode =
   result = newAsgnStmt(x, newOpCall(op, y))
 

lib/system/seqs_v2.nim

@@ -48,7 +48,7 @@ proc newSeqPayload(cap, elemSize: int): pointer {.compilerRtl, raises: [].} =
     result = nil
 
 proc prepareSeqAdd(len: int; p: pointer; addlen, elemSize: int): pointer {.
-    compilerRtl, noSideEffect, raises: [].} =
+    noSideEffect, raises: [].} =
   {.noSideEffect.}:
     template `+!`(p: pointer, s: int): pointer =
       cast[pointer](cast[int](p) +% s)

tests/arc/tcaseobj.nim

@@ -0,0 +1,146 @@
+discard """
+  valgrind: true
+  cmd: "nim c --gc:arc -d:useMalloc $file"
+  output: '''myobj destroyed
+myobj destroyed
+myobj destroyed
+A
+B
+begin
+end
+myobj destroyed
+'''
+"""
+
+# bug #13102
+
+type
+  D = ref object
+  R = object
+    case o: bool
+    of false:
+      discard
+    of true:
+      field: D
+
+iterator things(): R =
+  when true:
+    var
+      unit = D()
+    while true:
+      yield R(o: true, field: unit)
+  else:
+    while true:
+      var
+        unit = D()
+      yield R(o: true, field: unit)
+
+proc main =
+  var i = 0
+  for item in things():
+    discard item.field
+    inc i
+    if i == 2: break
+
+main()
+
+# bug #13149
+
+type
+  TMyObj = object
+    p: pointer
+    len: int
+
+proc `=destroy`(o: var TMyObj) =
+  if o.p != nil:
+    dealloc o.p
+    o.p = nil
+    echo "myobj destroyed"
+
+proc `=`(dst: var TMyObj, src: TMyObj) =
+  `=destroy`(dst)
+  dst.p = alloc(src.len)
+  dst.len = src.len
+
+proc `=sink`(dst: var TMyObj, src: TMyObj) =
+  `=destroy`(dst)
+  dst.p = src.p
+  dst.len = src.len
+
+type
+  TObjKind = enum Z, A, B
+  TCaseObj = object
+    case kind: TObjKind
+    of Z: discard
+    of A:
+      x1: int # this int plays important role
+      x2: TMyObj
+    of B:
+      y: TMyObj
+
+proc testSinks: TCaseObj =
+  result = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5)))
+  result = TCaseObj(kind: B, y: TMyObj(len: 3, p: alloc(3)))
+
+proc use(x: TCaseObj) = discard
+
+proc testCopies(i: int) =
+  var a: array[2, TCaseObj]
+  a[i] = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5)))
+  a[i+1] = a[i] # copy, cannot move
+  use(a[i])
+
+let x1 = testSinks()
+testCopies(0)
+
+# bug #12957
+
+type
+  PegKind* = enum
+    pkCharChoice,
+    pkSequence
+  Peg* = object ## type that represents a PEG
+    case kind: PegKind
+    of pkCharChoice: charChoice: ref set[char]
+    else: discard
+    sons: seq[Peg]
+
+proc charSet*(s: set[char]): Peg =
+  ## constructs a PEG from a character set `s`
+  result = Peg(kind: pkCharChoice)
+  new(result.charChoice)
+  result.charChoice[] = s
+
+proc len(a: Peg): int {.inline.} = return a.sons.len
+proc myadd(d: var Peg, s: Peg) {.inline.} = add(d.sons, s)
+
+proc sequence*(a: openArray[Peg]): Peg =
+  result = Peg(kind: pkSequence, sons: @[])
+  when false:
+    #works too:
+    result.myadd(a[0])
+    result.myadd(a[1])
+  for x in items(a):
+    # works:
+    #result.sons.add(x)
+    # fails:
+    result.myadd x
+  if result.len == 1:
+    result = result.sons[0] # this must not move!
+
+when true:
+  # bug #12957
+
+  proc p =
+    echo "A"
+    let x = sequence([charSet({'a'..'z', 'A'..'Z', '_'}),
+              charSet({'a'..'z', 'A'..'Z', '0'..'9', '_'})])
+    echo "B"
+  p()
+
+  proc testSubObjAssignment =
+    echo "begin"
+    # There must be extactly one element in the array constructor!
+    let x = sequence([charSet({'a'..'z', 'A'..'Z', '_'})])
+    echo "end"
+  testSubObjAssignment()