From 5278cf80eb4adce3110fd991d772c18e7b4cb0be Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Mon, 18 Nov 2019 12:33:44 +0100 Subject: [PATCH] ARC: closure bugfixes (#12677) * ARC: closure bugfixes * progress * ARC closures: create =hooks for captured parameters * ARC: always destroy constructions like tuples, arrays properly, even in edge cases * fixes a regression --- compiler/injectdestructors.nim | 51 ++++++++++++++++++++++++++-------- compiler/lambdalifting.nim | 11 ++++++-- compiler/sempass2.nim | 7 +++-- tests/destructor/tlists.nim | 19 +++++++++++++ 4 files changed, 72 insertions(+), 16 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 0bb192036e..705d8c3e82 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -271,7 +271,7 @@ proc sinkParamIsLastReadCheck(c: var Con, s: PNode) = localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s & "` is already consumed at " & toFileLineCol(c. graph.config, s.info)) -proc p(n: PNode; c: var Con): PNode +proc p(n: PNode; c: var Con; consumed = false): PNode proc pArg(arg: PNode; c: var Con; isSink: bool): PNode proc moveOrCopy(dest, ri: PNode; c: var Con): PNode @@ -366,6 +366,22 @@ template handleNested(n: untyped, processCall: untyped) = result.add branch else: assert(false) +proc ensureDestruction(arg: PNode; c: var Con): PNode = + # it can happen that we need to destroy expression contructors + # like [], (), closures explicitly in order to not leak them. + if arg.typ != nil and hasDestructor(arg.typ): + # produce temp creation for (fn, env). But we need to move 'env'? + # This was already done in the sink parameter handling logic. + result = newNodeIT(nkStmtListExpr, arg.info, arg.typ) + let tmp = getTemp(c, arg.typ, arg.info) + var sinkExpr = genSink(c, tmp, arg) + sinkExpr.add arg + result.add sinkExpr + result.add tmp + c.destroys.add genDestroy(c, tmp) + else: + result = arg + proc pArg(arg: PNode; c: var Con; isSink: bool): PNode = if isSink: if arg.kind in nkCallKinds: @@ -374,7 +390,7 @@ proc pArg(arg: PNode; c: var Con; isSink: bool): PNode = result = copyNode(arg) let parameters = arg[0].typ let L = if parameters != nil: parameters.len else: 0 - result.add arg[0] + result.add pArg(arg[0], c, isSink = false) for i in 1.. 0 and isDangerousSeq(ri.typ): result = genCopy(c, dest, ri) else: result = genSink(c, dest, ri) - result.add p(ri, c) + result.add p(ri, c, consumed = true) of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit: result = genSink(c, dest, ri) - result.add p(ri, c) + result.add p(ri, c, consumed = true) of nkSym: if isSinkParam(ri.sym): # Rule 3: `=sink`(x, z); wasMoved(z) @@ -610,7 +637,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: result = genCopy(c, dest, ri) - result.add p(ri, c) + result.add p(ri, c, consumed = true) of nkHiddenSubConv, nkHiddenStdConv, nkConv: result = moveOrCopy(dest, ri[1], c) if not sameType(ri.typ, ri[1].typ): @@ -633,7 +660,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: result = genCopy(c, dest, ri) - result.add p(ri, c) + result.add p(ri, c, consumed = true) proc computeUninit(c: var Con) = if not c.uninitComputed: diff --git a/compiler/lambdalifting.nim b/compiler/lambdalifting.nim index 9a12fb1927..f5fc4bb460 100644 --- a/compiler/lambdalifting.nim +++ b/compiler/lambdalifting.nim @@ -218,6 +218,10 @@ proc makeClosure*(g: ModuleGraph; prc: PSym; env: PNode; info: TLineInfo): PNode if env.skipConv.kind == nkClosure: localError(g.config, info, "internal error: taking closure of closure") result.add(env) + #if isClosureIterator(result.typ): + createTypeBoundOps(g, nil, result.typ, info) + if tfHasAsgn in result.typ.flags or optSeqDestructors in g.config.globalOptions: + prc.flags.incl sfInjectDestructors proc interestingIterVar(s: PSym): bool {.inline.} = # XXX optimization: Only lift the variable if it lives across @@ -236,8 +240,7 @@ proc liftingHarmful(conf: ConfigRef; owner: PSym): bool {.inline.} = proc createTypeBoundOpsLL(g: ModuleGraph; refType: PType; info: TLineInfo; owner: PSym) = createTypeBoundOps(g, nil, refType.lastSon, info) createTypeBoundOps(g, nil, refType, info) - if (tfHasAsgn in refType.flags) or - optSeqDestructors in g.config.globalOptions: + if tfHasAsgn in refType.flags or optSeqDestructors in g.config.globalOptions: owner.flags.incl sfInjectDestructors proc liftIterSym*(g: ModuleGraph; n: PNode; owner: PSym): PNode = @@ -604,6 +607,9 @@ proc rawClosureCreation(owner: PSym; let fieldAccess = indirectAccess(env, local, env.info) # add ``env.param = param`` result.add(newAsgnStmt(fieldAccess, newSymNode(local), env.info)) + createTypeBoundOps(d.graph, nil, fieldAccess.typ, env.info) + if tfHasAsgn in fieldAccess.typ.flags or optSeqDestructors in d.graph.config.globalOptions: + owner.flags.incl sfInjectDestructors let upField = lookupInRecord(env.typ.skipTypes({tyOwned, tyRef, tyPtr}).n, getIdent(d.graph.cache, upName)) if upField != nil: @@ -627,6 +633,7 @@ proc finishClosureCreation(owner: PSym; d: DetectionPass; c: LiftingPass; assert unowned != nil let nilLit = newNodeIT(nkNilLit, info, unowned.typ) res.add newAsgnStmt(unowned, nilLit, info) + createTypeBoundOpsLL(d.graph, unowned.typ, info, owner) proc closureCreationForIter(iter: PNode; d: DetectionPass; c: var LiftingPass): PNode = diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index a05ea68297..ef0dcec00d 100644 --- a/compiler/sempass2.nim +++ b/compiler/sempass2.nim @@ -709,11 +709,11 @@ proc track(tracked: PEffects, n: PNode) = # p's effects are ours too: var a = n.sons[0] let op = a.typ - if getConstExpr(tracked.ownerModule, n, tracked.graph) != nil: - return if n.typ != nil: if tracked.owner.kind != skMacro and n.typ.skipTypes(abstractVar).kind != tyOpenArray: createTypeBoundOps(tracked, n.typ, n.info) + if getConstExpr(tracked.ownerModule, n, tracked.graph) != nil: + return if a.kind == nkCast and a[1].typ.kind == tyProc: a = a[1] # XXX: in rare situations, templates and macros will reach here after @@ -883,6 +883,9 @@ proc track(tracked: PEffects, n: PNode) = if n.len == 2: track(tracked, n.sons[1]) of nkObjUpConv, nkObjDownConv, nkChckRange, nkChckRangeF, nkChckRange64: if n.len == 1: track(tracked, n.sons[0]) + of nkBracket: + for i in 0 ..< safeLen(n): track(tracked, n.sons[i]) + createTypeBoundOps(tracked, n.typ, n.info) else: for i in 0 ..< safeLen(n): track(tracked, n.sons[i]) diff --git a/tests/destructor/tlists.nim b/tests/destructor/tlists.nim index 78c09b1058..f5786d9361 100644 --- a/tests/destructor/tlists.nim +++ b/tests/destructor/tlists.nim @@ -51,7 +51,26 @@ proc tleakingNewStmt = for i in 0..10: new(x) +iterator infinite(): int {.closure.} = + var i = 0 + while true: + yield i + inc i + +iterator take(it: iterator (): int, numToTake: int): int {.closure.} = + var i = 0 + for x in it(): + if i >= numToTake: + break + yield x + inc i + +proc take3 = + for x in infinite.take(3): + discard + let startMem = getOccupiedMem() +take3() tlazyList() mkManyLeaks()