From f5a6b5efd2d11c781fd0834e26937b0167a96fbe Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 17 Dec 2019 20:37:29 +0100 Subject: [PATCH 1/2] fixes #12899 --- compiler/ccgexprs.nim | 6 ++++-- compiler/liftdestructors.nim | 20 ++++++++++------- tests/destructor/tnewruntime_strutils.nim | 26 ++++++++++++++++++++++- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/compiler/ccgexprs.nim b/compiler/ccgexprs.nim index 5fa7ac8d7f803..db8ba79b6b163 100644 --- a/compiler/ccgexprs.nim +++ b/compiler/ccgexprs.nim @@ -2031,13 +2031,15 @@ proc genDestroy(p: BProc; n: PNode) = var a: TLoc initLocExpr(p, arg, a) linefmt(p, cpsStmts, "if ($1.p && $1.p->allocator) {$n" & - " $1.p->allocator->dealloc($1.p->allocator, $1.p, $1.p->cap + 1 + sizeof(NI) + sizeof(void*)); }$n", + " $1.p->allocator->dealloc($1.p->allocator, $1.p, $1.p->cap + 1 + sizeof(NI) + sizeof(void*));$n" & + " $1.p = NIM_NIL; }$n", [rdLoc(a)]) of tySequence: var a: TLoc initLocExpr(p, arg, a) linefmt(p, cpsStmts, "if ($1.p && $1.p->allocator) {$n" & - " $1.p->allocator->dealloc($1.p->allocator, $1.p, ($1.p->cap * sizeof($2)) + sizeof(NI) + sizeof(void*)); }$n", + " $1.p->allocator->dealloc($1.p->allocator, $1.p, ($1.p->cap * sizeof($2)) + sizeof(NI) + sizeof(void*));$n" & + " $1.p = NIM_NIL; }$n", [rdLoc(a), getTypeDesc(p.module, t.lastSon)]) else: discard "nothing to do" else: diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 38f9ee417b3ee..932b24bb097b2 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -429,9 +429,7 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, cond, actions) body.add newAsgnStmt(x, y) of attachedDestructor: - when false: - # XXX investigate if this is necessary: - actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t)) + actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t)) body.add genIf(c, cond, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: @@ -480,9 +478,7 @@ proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, cond, actions) body.add newAsgnStmt(x, y) of attachedDestructor: - when false: - # XXX investigate if this is necessary: - actions.add newAsgnStmt(xenv, newNodeIT(nkNilLit, body.info, xenv.typ)) + actions.add newAsgnStmt(xenv, newNodeIT(nkNilLit, body.info, xenv.typ)) body.add genIf(c, cond, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: @@ -510,7 +506,10 @@ proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = of attachedDestructor: # it's better to prepend the destruction of weak refs in order to # prevent wrong "dangling refs exist" problems: - let des = genIf(c, x, callCodegenProc(c.g, "nimDecWeakRef", c.info, x)) + var actions = newNodeI(nkStmtList, c.info) + actions.add callCodegenProc(c.g, "nimDecWeakRef", c.info, x) + actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t)) + let des = genIf(c, x, actions) if body.len == 0: body.add des else: @@ -537,6 +536,7 @@ proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, x, actions) body.add newAsgnStmt(x, y) of attachedDestructor: + actions.add newAsgnStmt(x, newNodeIT(nkNilLit, body.info, t)) body.add genIf(c, x, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace, attachedDispose: discard @@ -567,7 +567,10 @@ proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx)) body.add newAsgnStmt(x, y) of attachedDestructor: - let des = genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx)) + var actions = newNodeI(nkStmtList, c.info) + actions.add callCodegenProc(c.g, "nimDecWeakRef", c.info, xx) + actions.add newAsgnStmt(xx, newNodeIT(nkNilLit, body.info, xx.typ)) + let des = genIf(c, xx, actions) if body.len == 0: body.add des else: @@ -586,6 +589,7 @@ proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, xx, actions) body.add newAsgnStmt(x, y) of attachedDestructor: + actions.add newAsgnStmt(xx, newNodeIT(nkNilLit, body.info, xx.typ)) body.add genIf(c, xx, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace, attachedDispose: discard diff --git a/tests/destructor/tnewruntime_strutils.nim b/tests/destructor/tnewruntime_strutils.nim index 3e4ee27beb98b..74cd985abceb3 100644 --- a/tests/destructor/tnewruntime_strutils.nim +++ b/tests/destructor/tnewruntime_strutils.nim @@ -1,7 +1,9 @@ discard """ valgrind: true cmd: '''nim c --newruntime -d:useMalloc $file''' - output: '''422 422''' + output: ''' +@[(input: @["KXSC", "BGMC"]), (input: @["PXFX"]), (input: @["WXRQ", "ZSCZD"])] +461 461''' """ import strutils, os, std / wordwrap @@ -13,6 +15,28 @@ import system / ansi_c proc retTuple(): (seq[int], int) = return (@[1], 1) +# bug #12899 + +import sequtils, strmisc + +const input = ["KXSC, BGMC => 7 PTHL", "PXFX => LBZJ", "WXRQ, ZSCZD => HLQM"] + +type + Reaction = object + input: seq[string] + +proc bug12899 = + var reactions: seq[Reaction] = @[] + for l in input: + let x = l.partition(" => ") + reactions.add Reaction(input: @(x[0].split(", "))) + + let x = $reactions + echo x + +bug12899() + + proc nonStaticTests = doAssert formatBiggestFloat(1234.567, ffDecimal, -1) == "1234.567000" doAssert formatBiggestFloat(1234.567, ffDecimal, 0) == "1235" # bugs 8242, 12586 From cb5f71654254cee66fa821aa7ef8267b5d4196e1 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 17 Dec 2019 22:58:01 +0100 Subject: [PATCH 2/2] fixes regression: destroy global variables in reverse declaration order, closureleak test relies on it --- compiler/cgen.nim | 4 ++-- tests/gc/closureleak.nim | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/cgen.nim b/compiler/cgen.nim index 389c5a70cfa80..5b336e78d2763 100644 --- a/compiler/cgen.nim +++ b/compiler/cgen.nim @@ -1981,8 +1981,8 @@ proc myClose(graph: ModuleGraph; b: PPassContext, n: PNode): PNode = if b == nil: return var m = BModule(b) if sfMainModule in m.module.flags: - for destructorCall in graph.globalDestructors: - n.add destructorCall + for i in countdown(high(graph.globalDestructors), 0): + n.add graph.globalDestructors[i] if passes.skipCodegen(m.config, n): return if moduleHasChanged(graph, m.module): # if the module is cached, we don't regenerate the main proc diff --git a/tests/gc/closureleak.nim b/tests/gc/closureleak.nim index 508004a5325a4..0265431d04631 100644 --- a/tests/gc/closureleak.nim +++ b/tests/gc/closureleak.nim @@ -6,7 +6,7 @@ discard """ type TFoo* = object id: int - fn: proc(){.closure.} + fn: proc() {.closure.} var foo_counter = 0 var alive_foos = newseq[int](0) @@ -31,7 +31,7 @@ proc newFoo*(): ref TFoo = inc foo_counter for i in 0 ..< 10: - discard newFoo() + discard newFoo() for i in 0 ..< 10: let f = newFoo()