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 <rumpf_a@web.de>

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..<ri.len:
+    if i < typ.len:
+      assert(typ.n[i].kind == nkSym)
+      let paramType = typ.n[i]
+      if not paramType.typ.isCompileTimeOnly:
+        if result != nil: result.add(~", ")
+        result.add(genArg(p, ri[i], paramType.sym, ri, needTmp[i-1]))
+    else:
+      if result != nil: result.add(~", ")
+      result.add(genArgNoParam(p, ri[i], needTmp[i-1]))
 
 proc addActualSuffixForHCR(res: var Rope, module: PSym, sym: PSym) =
   if sym.flags * {sfImportc, sfNonReloadable} == {} and sym.loc.k == locProc and
@@ -276,13 +344,13 @@ proc genPrefixCall(p: BProc, le, ri: PNode, d: var TLoc) =
   var op: TLoc
   # this is a hotspot in the compiler
   initLocExpr(p, ri[0], op)
-  var params: Rope
   # getUniqueType() is too expensive here:
   var typ = skipTypes(ri[0].typ, abstractInstOwned)
   assert(typ.kind == tyProc)
   assert(typ.len == typ.n.len)
-  for i in 1..<ri.len:
-    genParamLoop(params)
+
+  var params = genParams(p, ri, typ)
+
   var callee = rdLoc(op)
   if p.hcrOn and ri[0].kind == nkSym:
     callee.addActualSuffixForHCR(p.module.module, ri[0].sym)
@@ -290,23 +358,21 @@ proc genPrefixCall(p: BProc, le, ri: PNode, d: var TLoc) =
 
 proc genClosureCall(p: BProc, le, ri: PNode, d: var TLoc) =
 
-  proc getRawProcType(p: BProc, t: PType): Rope =
-    result = getClosureType(p.module, t, clHalf)
-
   proc addComma(r: Rope): Rope =
-    result = if r == nil: r else: r & ~", "
+    if r == nil: r else: r & ~", "
 
   const PatProc = "$1.ClE_0? $1.ClP_0($3$1.ClE_0):(($4)($1.ClP_0))($2)"
   const PatIter = "$1.ClP_0($3$1.ClE_0)" # we know the env exists
+
   var op: TLoc
   initLocExpr(p, ri[0], op)
-  var pl: Rope
 
-  var typ = skipTypes(ri[0].typ, abstractInst)
+  # getUniqueType() is too expensive here:
+  var typ = skipTypes(ri[0].typ, abstractInstOwned)
   assert(typ.kind == tyProc)
-  for i in 1..<ri.len:
-    assert(typ.len == typ.n.len)
-    genParamLoop(pl)
+  assert(typ.len == typ.n.len)
+
+  var pl = genParams(p, ri, typ)
 
   template genCallPattern {.dirty.} =
     if tfIterator in typ.flags:
@@ -314,7 +380,7 @@ proc genClosureCall(p: BProc, le, ri: PNode, d: var TLoc) =
     else:
       lineF(p, cpsStmts, PatProc & ";$n", [rdLoc(op), pl, pl.addComma, rawProc])
 
-  let rawProc = getRawProcType(p, typ)
+  let rawProc = getClosureType(p.module, typ, clHalf)
   let canRaise = p.config.exc == excGoto and canRaiseDisp(p, ri[0])
   if typ[0] != nil:
     if isInvalidReturnType(p.config, typ[0]):
@@ -668,20 +734,9 @@ proc isInactiveDestructorCall(p: BProc, e: PNode): bool =
   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:
-    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)

compiler/closureiters.nim

@@ -129,7 +129,7 @@
 #   break :stateLoop
 
 import
-  ast, astalgo, msgs, idents,
+  ast, msgs, idents,
   renderer, magicsys, lowerings, lambdalifting, modulegraphs, lineinfos
 
 type

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)

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