From af27e6bdea63bbf66718193ec44bc61e745ded38 Mon Sep 17 00:00:00 2001 From: Clyybber Date: Sat, 4 Jul 2020 17:45:07 +0200 Subject: [PATCH] Fix #14396 (#14793) * Correct Left-To-Right evaluation of proc args * Fix CPP backend * Add testcase * closes #14396 * closes #14345 * Improve test and optimize * Improve testcase and optimize literals * Fix bug * Expand testcase and use DFA to optimize * Turn genParams into proc * Turn withTmpIfNeeded into a proc * Cleanup * Fix crash * Better analysis * Cleanup * Trailing newline.. * Fix build * Tiny cleanup Co-authored-by: Andreas Rumpf --- compiler/ccgcalls.nim | 145 ++++++++++++++++++++--------- compiler/closureiters.nim | 2 +- tests/arc/tarcmisc.nim | 52 +++++------ tests/ccgbugs/targ_lefttoright.nim | 70 ++++++++++++++ 4 files changed, 198 insertions(+), 71 deletions(-) create mode 100644 tests/ccgbugs/targ_lefttoright.nim diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim index 2c5d306a3f..5f99f357dd 100644 --- a/compiler/ccgcalls.nim +++ b/compiler/ccgcalls.nim @@ -217,21 +217,30 @@ proc openArrayLoc(p: BProc, formalType: PType, n: PNode): Rope = internalError(p.config, "openArrayLoc: " & typeToString(a.t)) else: internalError(p.config, "openArrayLoc: " & typeToString(a.t)) -proc genArgStringToCString(p: BProc, n: PNode): Rope {.inline.} = +proc withTmpIfNeeded(p: BProc, a: TLoc, needsTmp: bool): TLoc = + if needsTmp: + var tmp: TLoc + getTemp(p, a.lode.typ, tmp, needsInit=false) + genAssignment(p, tmp, a, {}) + tmp + else: + a + +proc genArgStringToCString(p: BProc, n: PNode, needsTmp: bool): Rope {.inline.} = var a: TLoc initLocExpr(p, n[0], a) - result = ropecg(p.module, "#nimToCStringConv($1)", [a.rdLoc]) + ropecg(p.module, "#nimToCStringConv($1)", [withTmpIfNeeded(p, a, needsTmp).rdLoc]) -proc genArg(p: BProc, n: PNode, param: PSym; call: PNode): Rope = +proc genArg(p: BProc, n: PNode, param: PSym; call: PNode, needsTmp = false): Rope = var a: TLoc if n.kind == nkStringToCString: - result = genArgStringToCString(p, n) + result = genArgStringToCString(p, n, needsTmp) elif skipTypes(param.typ, abstractVar).kind in {tyOpenArray, tyVarargs}: var n = if n.kind != nkHiddenAddr: n else: n[0] result = openArrayLoc(p, param.typ, n) elif ccgIntroducedPtr(p.config, param, call[0].typ[0]): initLocExpr(p, n, a) - result = addrLoc(p.config, a) + result = addrLoc(p.config, withTmpIfNeeded(p, a, needsTmp)) elif p.module.compileToCpp and param.typ.kind in {tyVar} and n.kind == nkHiddenAddr: initLocExprSingleUse(p, n[0], a) @@ -246,26 +255,85 @@ proc genArg(p: BProc, n: PNode, param: PSym; call: PNode): Rope = result = rdLoc(a) else: initLocExprSingleUse(p, n, a) - result = rdLoc(a) + result = rdLoc(withTmpIfNeeded(p, a, needsTmp)) -proc genArgNoParam(p: BProc, n: PNode): Rope = +proc genArgNoParam(p: BProc, n: PNode, needsTmp = false): Rope = var a: TLoc if n.kind == nkStringToCString: - result = genArgStringToCString(p, n) + result = genArgStringToCString(p, n, needsTmp) else: initLocExprSingleUse(p, n, a) - result = rdLoc(a) + result = rdLoc(withTmpIfNeeded(p, a, needsTmp)) -template genParamLoop(params) {.dirty.} = - if i < typ.len: - assert(typ.n[i].kind == nkSym) - let paramType = typ.n[i] - if not paramType.typ.isCompileTimeOnly: - if params != nil: params.add(~", ") - params.add(genArg(p, ri[i], paramType.sym, ri)) +from dfa import instrTargets, InstrTargetKind + +proc potentialAlias(n: PNode, potentialWrites: seq[PNode]): bool = + for p in potentialWrites: + if instrTargets(p, n) != None: + return true + +proc skipTrivialIndirections(n: PNode): PNode = + result = n + while true: + case result.kind + of {nkDerefExpr, nkHiddenDeref, nkAddr, nkHiddenAddr, nkObjDownConv, nkObjUpConv}: + result = result[0] + of {nkHiddenStdConv, nkHiddenSubConv}: + result = result[1] + else: break + +proc getPotentialWrites(n: PNode, mutate = false): seq[PNode] = + case n.kind: + of nkLiterals, nkIdent: discard + of nkSym: + if mutate: result.add n + of nkAsgn, nkFastAsgn: + result.add getPotentialWrites(n[0], true) + result.add getPotentialWrites(n[1], mutate) + of nkAddr, nkHiddenAddr: + result.add getPotentialWrites(n[0], true) + of nkCallKinds: #TODO: Find out why in f += 1, f is a nkSym and not a nkHiddenAddr + for s in n.sons: + result.add getPotentialWrites(s, true) else: - if params != nil: params.add(~", ") - params.add(genArgNoParam(p, ri[i])) + for s in n.sons: + result.add getPotentialWrites(s, mutate) + +proc getPotentialReads(n: PNode): seq[PNode] = + case n.kind: + of nkLiterals, nkIdent: discard + of nkSym: result.add n + else: + for s in n.sons: + result.add getPotentialReads(s) + +proc genParams(p: BProc, ri: PNode, typ: PType): Rope = + # We must generate temporaries in cases like #14396 + # to keep the strict Left-To-Right evaluation + var needTmp = newSeq[bool](ri.len - 1) + var potentialWrites: seq[PNode] + for i in countdown(ri.len - 1, 1): + if ri[i].skipTrivialIndirections.kind == nkSym: + needTmp[i - 1] = potentialAlias(ri[i], potentialWrites) + else: + for n in getPotentialReads(ri[i]): + if not needTmp[i - 1]: + needTmp[i - 1] = potentialAlias(n, potentialWrites) + potentialWrites.add getPotentialWrites(ri[i]) + if ri[i].kind == nkHiddenAddr: + # Optimization: don't use a temp, if we would only take the adress anyway + needTmp[i - 1] = false + + for i in 1.. 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: - genInfixCall(p, nil, e, d) - elif e[0].kind == nkSym and sfNamedParamCall in e[0].sym.flags: - genNamedParamCall(p, e, d) - else: - genPrefixCall(p, nil, e, d) - postStmtActions(p) - proc genAsgnCall(p: BProc, le, ri: PNode, d: var TLoc) = + if p.withinBlockLeaveActions > 0 and isInactiveDestructorCall(p, ri): + return if ri[0].typ.skipTypes({tyGenericInst, tyAlias, tySink, tyOwned}).callConv == ccClosure: genClosureCall(p, le, ri, d) elif ri[0].kind == nkSym and sfInfixCall in ri[0].sym.flags: @@ -691,3 +746,5 @@ proc genAsgnCall(p: BProc, le, ri: PNode, d: var TLoc) = else: genPrefixCall(p, le, ri, d) postStmtActions(p) + +proc genCall(p: BProc, e: PNode, d: var TLoc) = genAsgnCall(p, nil, e, d) diff --git a/compiler/closureiters.nim b/compiler/closureiters.nim index 675f1ae413..2248f956e2 100644 --- a/compiler/closureiters.nim +++ b/compiler/closureiters.nim @@ -129,7 +129,7 @@ # break :stateLoop import - ast, astalgo, msgs, idents, + ast, msgs, idents, renderer, magicsys, lowerings, lambdalifting, modulegraphs, lineinfos type diff --git a/tests/arc/tarcmisc.nim b/tests/arc/tarcmisc.nim index e66d9a75f0..b6d9d781d8 100644 --- a/tests/arc/tarcmisc.nim +++ b/tests/arc/tarcmisc.nim @@ -15,6 +15,7 @@ destroyed: false (x: "8") (x: "9") (x: "10") +0 closed destroying variable ''' @@ -174,38 +175,37 @@ proc bug14495 = bug14495() -when false: - # bug #14396 - type - Spinny = ref object - t: ref int - text: string +# bug #14396 +type + Spinny = ref object + t: ref int + text: string - proc newSpinny*(): Spinny = - Spinny(t: new(int), text: "hello") +proc newSpinny*(): Spinny = + Spinny(t: new(int), text: "hello") - proc spinnyLoop(x: ref int, spinny: sink Spinny) = - echo x[] +proc spinnyLoop(x: ref int, spinny: sink Spinny) = + echo x[] - proc start*(spinny: sink Spinny) = - spinnyLoop(spinny.t, spinny) +proc start*(spinny: sink Spinny) = + spinnyLoop(spinny.t, spinny) - var spinner1 = newSpinny() - spinner1.start() +var spinner1 = newSpinny() +spinner1.start() - # bug #14345 +# bug #14345 - type - SimpleLoopB = ref object - children: seq[SimpleLoopB] - parent: SimpleLoopB +type + SimpleLoopB = ref object + children: seq[SimpleLoopB] + parent: SimpleLoopB - proc addChildLoop(self: SimpleLoopB, loop: SimpleLoopB) = - self.children.add loop +proc addChildLoop(self: SimpleLoopB, loop: SimpleLoopB) = + self.children.add loop - proc setParent(self: SimpleLoopB, parent: SimpleLoopB) = - self.parent = parent - self.parent.addChildLoop(self) +proc setParent(self: SimpleLoopB, parent: SimpleLoopB) = + self.parent = parent + self.parent.addChildLoop(self) - var l = SimpleLoopB() - l.setParent(l) +var l = SimpleLoopB() +l.setParent(l) diff --git a/tests/ccgbugs/targ_lefttoright.nim b/tests/ccgbugs/targ_lefttoright.nim new file mode 100644 index 0000000000..b107b23279 --- /dev/null +++ b/tests/ccgbugs/targ_lefttoright.nim @@ -0,0 +1,70 @@ +discard """ + nimout: '''1,2 +2,3 +2,2 +1,2 +1,2 +2,2 +1,2 +''' + output: '''1,2 +2,3 +1,2 +2,2 +1,2 +1,2 +2,2 +1,2 +''' +""" + +template test = + proc say(a, b: int) = + echo a,",",b + + var a = 1 + say a, (a += 1; a) #1,2 + + var b = 1 + say (b += 1; b), (b += 1; b) #2,3 + + type C = object {.byRef.} + i: int + + proc say(a, b: C) = + echo a.i,",",b.i + + proc `+=`(x: var C, y: C) = x.i += y.i + + var c = C(i: 1) + when nimvm: #XXX: This would output 2,2 in the VM, which is wrong + discard + else: + say c, (c += C(i: 1); c) #1,2 + + proc sayVar(a: var int, b: int) = + echo a,",",b + + var d = 1 + sayVar d, (d += 1; d) #2,2 + + var e = 1 + say (addr e)[], (e += 1; e) #1,2 + + var f = 1 + say f, if false: f + else: f += 1; f #1,2 + + var g = 1 + say g + 1, if false: g + else: g += 1; g #2,2 + + proc `+=+`(x: var int, y: int): int = (inc(x, y); x) + + var h = 1 + say h, h +=+ 1 # 1,2 + +test + +static: + test