From a5e67071d27a33c7f19d739a6918e9e16b44e4ab Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Wed, 15 Jan 2020 22:13:31 +0100 Subject: [PATCH] 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 --- compiler/dfa.nim | 10 ++- compiler/injectdestructors.nim | 3 +- compiler/liftdestructors.nim | 99 +++++++++++++++++----- lib/system/seqs_v2.nim | 2 +- tests/arc/tcaseobj.nim | 146 +++++++++++++++++++++++++++++++++ tests/arc/tmovebug.nim | 64 +++++++++++++++ 6 files changed, 298 insertions(+), 26 deletions(-) create mode 100644 tests/arc/tcaseobj.nim create mode 100644 tests/arc/tmovebug.nim diff --git a/compiler/dfa.nim b/compiler/dfa.nim index cec45eaecab52..94c114fef4a85 100644 --- a/compiler/dfa.nim +++ b/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. diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index f5e525e0083f2..080da17152832 100644 --- a/compiler/injectdestructors.nim +++ b/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 diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index b9b1210703613..def557402c293 100644 --- a/compiler/liftdestructors.nim +++ b/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)) diff --git a/lib/system/seqs_v2.nim b/lib/system/seqs_v2.nim index b7f9fb1533189..ed49d3abc7ac2 100644 --- a/lib/system/seqs_v2.nim +++ b/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) diff --git a/tests/arc/tcaseobj.nim b/tests/arc/tcaseobj.nim new file mode 100644 index 0000000000000..44daa5979ab60 --- /dev/null +++ b/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() diff --git a/tests/arc/tmovebug.nim b/tests/arc/tmovebug.nim new file mode 100644 index 0000000000000..98f181b81eff1 --- /dev/null +++ b/tests/arc/tmovebug.nim @@ -0,0 +1,64 @@ +discard """ + cmd: "nim c --gc:arc $file" + output: '''5''' +""" + +# move bug +type + TMyObj = object + p: pointer + len: int + +var destroyCounter = 0 + +proc `=destroy`(o: var TMyObj) = + if o.p != nil: + dealloc o.p + o.p = nil + inc destroyCounter + +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 use(a: TCaseObj) = discard + +proc moveBug(i: var int) = + var a: array[2, TCaseObj] + a[i] = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5))) # 1 + a[i+1] = a[i] # 2 + inc i + use(a[i-1]) + +var x = 0 +moveBug(x) + +proc moveBug2(): (TCaseObj, TCaseObj) = + var a: array[2, TCaseObj] + a[0] = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5))) + a[1] = a[0] # can move 3 + result[0] = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5))) # 4 + result[1] = result[0] # 5 + +proc main = + discard moveBug2() + +main() +echo destroyCounter