diff --git a/compiler/varpartitions.nim b/compiler/varpartitions.nim index f983f9416b..9297c19e16 100644 --- a/compiler/varpartitions.nim +++ b/compiler/varpartitions.nim @@ -13,6 +13,17 @@ ## algorithm. ## The used data structure is "union find" with path compression. +## We perform two passes over the AST: +## - Pass one (``computeLiveRanges``): collect livetimes of local +## variables and whether they are potentially re-assigned. +## - Pass two (``traverse``): combine local variables to abstract "graphs". +## Strict func checking: Ensure that graphs that are connected to +## const parameters are not mutated. +## Cursor inference: Ensure that potential cursors are not +## borrowed from locations that are connected to a graph +## that is mutated during the liveness of the cursor. +## (We track all possible mutations of a graph.) + import ast, types, lineinfos, options, msgs, renderer from trees import getMagic, whichPragma from wordrecg import wNoSideEffect @@ -26,7 +37,8 @@ type VarFlag = enum ownsData, - preventCursor + preventCursor, + isReassigned VarIndexKind = enum isEmptyRoot, @@ -266,36 +278,38 @@ proc allRoots(n: PNode; result: var seq[PSym]; followDotExpr = true) = else: discard "nothing to do" -proc analyseAsgn(c: var Partitions; dest: var VarIndex; n: PNode) = +proc destMightOwn(c: var Partitions; dest: var VarIndex; n: PNode) = + ## Analyse if 'n' is an expression that owns the data, if so mark 'dest' + ## with 'ownsData'. case n.kind of nkEmpty, nkCharLit..nkNilLit: # primitive literals including the empty are harmless: discard of nkExprEqExpr, nkExprColonExpr, nkHiddenStdConv, nkHiddenSubConv, nkCast, nkConv: - analyseAsgn(c, dest, n[1]) + destMightOwn(c, dest, n[1]) of nkIfStmt, nkIfExpr: for i in 0.. 0: - analyseAsgn(c, dest, n[^1]) + destMightOwn(c, dest, n[^1]) of nkClosure: for i in 1..= 0: - analyseAsgn(c, c.s[vid], src) - # do not borrow from a different local variable, this is easier - # than tracking reassignments, consider 'var cursor = local; local = newNode()' + destMightOwn(c, c.s[vid], src) if src.kind == nkSym: - if (src.sym.kind in {skVar, skResult, skTemp} or - (src.sym.kind in {skLet, skParam, skForVar} and hasDisabledAsgn(src.sym.typ))): + let s = src.sym + if {sfGlobal, sfThread} * s.flags != {} or hasDisabledAsgn(s.typ): + # do not borrow from a global variable or from something with a + # disabled assignment operator. c.s[vid].flags.incl preventCursor - elif src.sym.kind in {skVar, skResult, skTemp, skLet, skForVar}: - # XXX: we need to compute variable alive ranges before doing anything else: - let srcid = variableId(c, src.sym) - if srcid >= 0 and preventCursor in c.s[srcid].flags: - # you cannot borrow from a local that lives shorter than 'vid': - if c.s[srcid].aliveStart > c.s[vid].aliveStart or - c.s[srcid].aliveEnd < c.s[vid].aliveEnd: + when false: echo "A not a cursor: ", dest.sym, " ", s + else: + let srcid = variableId(c, s) + if srcid >= 0: + if s.kind notin {skResult, skParam} and ( + c.s[srcid].aliveEnd < c.s[vid].aliveEnd): + # you cannot borrow from a local that lives shorter than 'vid': + when false: echo "B not a cursor ", dest.sym, " ", c.s[srcid].aliveEnd, " ", c.s[vid].aliveEnd + c.s[vid].flags.incl preventCursor + elif {isReassigned, preventCursor} * c.s[srcid].flags != {}: + # you cannot borrow from something that is re-assigned: + when false: echo "C not a cursor ", dest.sym, " ", c.s[srcid].flags c.s[vid].flags.incl preventCursor - if src.kind == nkSym and hasDestructor(src.typ): - rhsIsSink(c, src) + #if src.kind == nkSym and hasDestructor(src.typ): + # rhsIsSink(c, src) + +const + nodesToIgnoreSet = {nkNone..pred(nkSym), succ(nkSym)..nkNilLit, + nkTypeSection, nkProcDef, nkConverterDef, + nkMethodDef, nkIteratorDef, nkMacroDef, nkTemplateDef, nkLambda, nkDo, + nkFuncDef, nkConstSection, nkConstDef, nkIncludeStmt, nkImportStmt, + nkExportStmt, nkPragma, nkCommentStmt, nkBreakState, nkTypeOfExpr} proc traverse(c: var Partitions; n: PNode) = inc c.abstractTime @@ -418,11 +444,11 @@ proc traverse(c: var Partitions; n: PNode) = if child.kind == nkVarTuple and last.kind in {nkPar, nkTupleConstr}: if child.len-2 != last.len: return for i in 0..= 0: - c.s[id].aliveEnd = max(c.s[id].aliveEnd, c.abstractTime) - of nkNone..pred(nkSym), succ(nkSym)..nkNilLit, nkTypeSection, nkProcDef, nkConverterDef, - nkMethodDef, nkIteratorDef, nkMacroDef, nkTemplateDef, nkLambda, nkDo, - nkFuncDef, nkConstSection, nkConstDef, nkIncludeStmt, nkImportStmt, - nkExportStmt, nkPragma, nkCommentStmt, nkBreakState, nkTypeOfExpr: + of nodesToIgnoreSet: dec c.abstractTime discard "do not follow the construct" of nkCallKinds: @@ -514,6 +533,79 @@ proc traverse(c: var Partitions; n: PNode) = else: for child in n: traverse(c, child) +proc computeLiveRanges(c: var Partitions; n: PNode) = + # first pass: Compute live ranges for locals. + # **Watch out!** We must traverse the tree like 'traverse' does + # so that the 'c.abstractTime' is consistent. + inc c.abstractTime + case n.kind + of nkLetSection, nkVarSection: + for child in n: + let last = lastSon(child) + computeLiveRanges(c, last) + if child.kind == nkVarTuple and last.kind in {nkPar, nkTupleConstr}: + if child.len-2 != last.len: return + for i in 0..= 0: + c.s[vid].flags.incl isReassigned + + of nkSym: + dec c.abstractTime + if n.sym.kind in {skVar, skResult, skTemp, skLet, skForVar, skParam}: + let id = variableId(c, n.sym) + if id >= 0: + c.s[id].aliveEnd = max(c.s[id].aliveEnd, c.abstractTime) + + of nodesToIgnoreSet: + dec c.abstractTime + discard "do not follow the construct" + of nkCallKinds: + for child in n: computeLiveRanges(c, child) + + let parameters = n[0].typ + let L = if parameters != nil: parameters.len else: 0 + + for i in 1..= 0: + c.s[vid].flags.incl isReassigned + + of nkAddr, nkHiddenAddr: + computeLiveRanges(c, n[0]) + if n[0].kind == nkSym: + let vid = variableId(c, n[0].sym) + if vid >= 0: + c.s[vid].flags.incl preventCursor + + of nkPragmaBlock: + computeLiveRanges(c, n.lastSon) + of nkWhileStmt, nkForStmt, nkParForStmt: + for child in n: computeLiveRanges(c, child) + # analyse loops twice so that 'abstractTime' suffices to detect cases + # like: + # while cond: + # mutate(graph) + # connect(graph, cursorVar) + for child in n: computeLiveRanges(c, child) + else: + for child in n: computeLiveRanges(c, child) + proc computeGraphPartitions*(s: PSym; n: PNode; cursorInference = false): Partitions = result = Partitions(performCursorInference: cursorInference) if s.kind notin {skModule, skMacro}: @@ -523,6 +615,9 @@ proc computeGraphPartitions*(s: PSym; n: PNode; cursorInference = false): Partit if resultPos < s.ast.safeLen: registerVariable(result, s.ast[resultPos]) + computeLiveRanges(result, n) + # resart the timer for the second pass: + result.abstractTime = 0 traverse(result, n) proc dangerousMutation(g: MutationInfo; v: VarIndex): bool = @@ -560,7 +655,7 @@ proc computeCursors*(s: PSym; n: PNode; config: ConfigRef) = var par = computeGraphPartitions(s, n, true) for i in 0 ..< par.s.len: let v = addr(par.s[i]) - if v.flags == {} and v.sym.kind notin {skParam, skResult} and + if v.flags * {ownsData, preventCursor} == {} and v.sym.kind notin {skParam, skResult} and v.sym.flags * {sfThread, sfGlobal} == {} and hasDestructor(v.sym.typ) and v.sym.typ.skipTypes({tyGenericInst, tyAlias}).kind != tyOwned: let rid = root(par, i) diff --git a/tests/arc/top_no_cursor2.nim b/tests/arc/top_no_cursor2.nim new file mode 100644 index 0000000000..5dfde57aa7 --- /dev/null +++ b/tests/arc/top_no_cursor2.nim @@ -0,0 +1,53 @@ +discard """ + output: '''true +true +true +true +true''' + cmd: "nim c --gc:arc $file" +""" +# bug #15361 + +type + ErrorNodeKind = enum Branch, Leaf + Error = ref object + case kind: ErrorNodeKind + of Branch: + left: Error + right: Error + of Leaf: + leafError: string + input: string + +proc ret(input: string, lefterr, righterr: Error): Error = + result = Error(kind: Branch, left: lefterr, right: righterr, input: input) + +proc parser() = + var rerrors: Error + let lerrors = Error( + kind: Leaf, + leafError: "first error", + input: "123 ;" + ) + # If you remove "block" - everything works + block: + let rresult = Error( + kind: Leaf, + leafError: "second error", + input: ";" + ) + # this assignment is needed too + rerrors = rresult + + # Returns Error(kind: Branch, left: lerrors, right: rerrors, input: "some val") + # needs to be a proc call for some reason, can't inline the result + var data = ret(input = "some val", lefterr = lerrors, righterr = rerrors) + + echo data.left.leafError == "first error" + echo data.left.input == "123 ;" + # stacktrace shows this line + echo data.right.leafError == "second error" + echo data.right.input == ";" + echo data.input == "some val" + +parser() diff --git a/tests/arc/topt_no_cursor.nim b/tests/arc/topt_no_cursor.nim index 8a33cb2ee6..1e5cf7142b 100644 --- a/tests/arc/topt_no_cursor.nim +++ b/tests/arc/topt_no_cursor.nim @@ -52,7 +52,7 @@ _ = ( blitTmp, ";") lvalue = _[0] lnext_cursor = _[1] -`=sink`(result.value, lvalue) +`=sink`(result.value, move lvalue) -- end of expandArc ------------------------ --expandArc: tt @@ -148,7 +148,7 @@ proc p1(): Maybe = var lnext: string (lvalue, lnext) = (lresult, ";") - result.value = lvalue + result.value = move lvalue proc tissue15130 = doAssert p1().value == @[123] diff --git a/tests/destructor/tdestructor3.nim b/tests/destructor/tdestructor3.nim index f967bbf950..ca9a891e10 100644 --- a/tests/destructor/tdestructor3.nim +++ b/tests/destructor/tdestructor3.nim @@ -23,17 +23,18 @@ joinable: false type T = object proc `=`(lhs: var T, rhs: T) = - echo "assign" + echo "assign" proc `=destroy`(v: var T) = - echo "destroy" + echo "destroy" proc use(x: T) = discard proc usedToBeBlock = - var v1 : T - var v2 : T = v1 - use v1 + var v1 = T() + var v2: T = v1 + discard addr(v2) # prevent cursorfication + use v1 usedToBeBlock() diff --git a/tests/destructor/tmatrix.nim b/tests/destructor/tmatrix.nim index a3bd59df37..98ca95c949 100644 --- a/tests/destructor/tmatrix.nim +++ b/tests/destructor/tmatrix.nim @@ -96,14 +96,16 @@ proc info = allocCount = 0 deallocCount = 0 +proc copy(a: Matrix): Matrix = a + proc test1 = var a = matrix(5, 5, 1.0) - var b = a + var b = copy a var c = a + b proc test2 = var a = matrix(5, 5, 1.0) - var b = a + var b = copy a var c = -a proc test3 =