From 52a3acb0663a7d94c10f128a8ed3e1ff8e3b4be4 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Fri, 31 Oct 2014 07:55:56 +0100 Subject: [PATCH 1/6] Fix method recursion bug. Additional checks for method call transformations. --- compiler/transf.nim | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/transf.nim b/compiler/transf.nim index b9b06675ed..20fd5620f3 100644 --- a/compiler/transf.nim +++ b/compiler/transf.nim @@ -624,7 +624,11 @@ proc transformCall(c: PTransf, n: PNode): PTransNode = # bugfix: check after 'transformSons' if it's still a method call: # use the dispatcher for the call: if s.sons[0].kind == nkSym and s.sons[0].sym.kind == skMethod: - result = methodCall(s).PTransNode + let t = lastSon(s.sons[0].sym.ast) + if t.kind == nkSym and sfDispatcher in t.sym.flags: + result = methodCall(s).PTransNode + else: + result = s.PTransNode else: result = s.PTransNode From 1fc8bab6433982611b8dc3585e65dbd7be946a89 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sat, 1 Nov 2014 04:07:42 +0100 Subject: [PATCH 2/6] Reset location when creating a method dispatcher When creating a method dispatcher, the location of the underlying method was copied. Under some circumstances, the name of the location (loc.r) was already initialized, in which case the method dispatcher shared a name with one of the methods, leading to a C compiler error. By setting loc.r to nil when copying the dispatcher information from the original method, we ensure that the dispatcher C function gets its proper name. --- compiler/cgmeth.nim | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/cgmeth.nim b/compiler/cgmeth.nim index 948541302c..9755bc6745 100644 --- a/compiler/cgmeth.nim +++ b/compiler/cgmeth.nim @@ -100,6 +100,7 @@ proc methodDef*(s: PSym, fromCache: bool) = if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault disp.ast = copyTree(s.ast) disp.ast.sons[bodyPos] = ast.emptyNode + disp.loc.r = nil if s.typ.sons[0] != nil: disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) attachDispatcher(s, newSymNode(disp)) From f3d7158d5db591a88a6da3234a1f72d534c543fb Mon Sep 17 00:00:00 2001 From: Simon Krauter Date: Sun, 2 Nov 2014 03:00:47 +0100 Subject: [PATCH 3/6] Fix terminate() and add kill() --- lib/pure/osproc.nim | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index 3c181bf533..4384cd7d27 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -164,8 +164,11 @@ proc resume*(p: PProcess) {.rtl, extern: "nosp$1", tags: [].} ## Resumes the process `p`. proc terminate*(p: PProcess) {.rtl, extern: "nosp$1", tags: [].} - ## Terminates the process `p`. + ## Stop the process `p`. On Posix OSs the procedure sends SIGTERM to the process. On Windows the Win32 API function TerminateProcess() is called to stop the process. +proc kill*(p: PProcess) {.rtl, extern: "nosp$1", tags: [].} + ## Kill the process `p`. On Posix OSs the procedure sends SIGKILL to the process. On Windows kill() is an alias for terminate(). + proc running*(p: PProcess): bool {.rtl, extern: "nosp$1", tags: [].} ## Returns true iff the process `p` is still running. Returns immediately. @@ -475,6 +478,9 @@ when defined(Windows) and not defined(useNimRtl): if running(p): discard terminateProcess(p.fProcessHandle, 0) + proc kill(p: PProcess) = + terminate(p) + proc waitForExit(p: PProcess, timeout: int = -1): int = discard waitForSingleObject(p.fProcessHandle, timeout.int32) @@ -815,10 +821,10 @@ elif not defined(useNimRtl): discard close(p.errHandle) proc suspend(p: PProcess) = - if kill(-p.id, SIGSTOP) != 0'i32: osError(osLastError()) + if kill(p.id, SIGSTOP) != 0'i32: osError(osLastError()) proc resume(p: PProcess) = - if kill(-p.id, SIGCONT) != 0'i32: osError(osLastError()) + if kill(p.id, SIGCONT) != 0'i32: osError(osLastError()) proc running(p: PProcess): bool = var ret = waitpid(p.id, p.exitCode, WNOHANG) @@ -826,11 +832,13 @@ elif not defined(useNimRtl): result = ret == int(p.id) proc terminate(p: PProcess) = - if kill(-p.id, SIGTERM) == 0'i32: - if p.running(): - if kill(-p.id, SIGKILL) != 0'i32: osError(osLastError()) - else: osError(osLastError()) + if kill(p.id, SIGTERM) != 0'i32: + osError(osLastError()) + proc kill(p: PProcess) = + if kill(p.id, SIGKILL) != 0'i32: + osError(osLastError()) + proc waitForExit(p: PProcess, timeout: int = -1): int = #if waitPid(p.id, p.exitCode, 0) == int(p.id): # ``waitPid`` fails if the process is not running anymore. But then From bce2d57d95b71f3e4a5d4585ea735870251d1caa Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sun, 2 Nov 2014 15:37:16 +0100 Subject: [PATCH 4/6] Added test case for recursive methods. --- tests/method/trecmeth.nim | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/method/trecmeth.nim diff --git a/tests/method/trecmeth.nim b/tests/method/trecmeth.nim new file mode 100644 index 0000000000..32f620f15f --- /dev/null +++ b/tests/method/trecmeth.nim @@ -0,0 +1,22 @@ +# Note: We only compile this to verify that code generation +# for recursive methods works, no code is being executed + +type + Obj = ref object of TObject + +# Mutual recursion + +method alpha(x: Obj) +method beta(x: Obj) + +method alpha(x: Obj) = + beta(x) + +method beta(x: Obj) = + alpha(x) + +# Simple recursion + +method gamma(x: Obj) = + gamma(x) + From ce9a57fcfd21a1a3cb17087107c246f0492bec7f Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sun, 2 Nov 2014 23:35:41 +0100 Subject: [PATCH 5/6] Fix dispatcher creation for method prototypes. When method prototypes were involved (e.g. forward declarations for mutual recursion), calls were sometimes dispatched to the wrong method implementation. One of the reasons was that method dispatchers were then not always attached to method ASTs in the correct place. --- compiler/cgmeth.nim | 88 +++++++++++++++++++++++++++++----------- compiler/transf.nim | 7 ++-- tests/method/tmproto.nim | 25 ++++++++++++ 3 files changed, 92 insertions(+), 28 deletions(-) create mode 100644 tests/method/tmproto.nim diff --git a/compiler/cgmeth.nim b/compiler/cgmeth.nim index 9755bc6745..d881226d4d 100644 --- a/compiler/cgmeth.nim +++ b/compiler/cgmeth.nim @@ -11,7 +11,7 @@ import intsets, options, ast, astalgo, msgs, idents, renderer, types, magicsys, - sempass2 + sempass2, strutils proc genConv(n: PNode, d: PType, downcast: bool): PNode = var dest = skipTypes(d, abstractPtrs) @@ -44,7 +44,8 @@ proc methodCall*(n: PNode): PNode = result.sons[i] = genConv(result.sons[i], disp.typ.sons[i], true) # save for incremental compilation: -var gMethods: seq[TSymSeq] = @[] +var + gMethods: seq[tuple[methods: TSymSeq, dispatcher: PSym]] = @[] proc sameMethodBucket(a, b: PSym): bool = result = false @@ -80,32 +81,70 @@ proc attachDispatcher(s: PSym, dispatcher: PNode) = else: s.ast.add(dispatcher) +proc createDispatcher(s: PSym): PSym = + var disp = copySym(s) + incl(disp.flags, sfDispatcher) + excl(disp.flags, sfExported) + disp.typ = copyType(disp.typ, disp.typ.owner, false) + # we can't inline the dispatcher itself (for now): + if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault + disp.ast = copyTree(s.ast) + disp.ast.sons[bodyPos] = ast.emptyNode + disp.loc.r = nil + if s.typ.sons[0] != nil: + if disp.ast.sonsLen > resultPos: + disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) + else: + # We've encountered a method prototype without a filled-in + # resultPos slot. We put a placeholder in there that will + # be updated in fixupDispatcher(). + disp.ast.addSon(ast.emptyNode) + attachDispatcher(s, newSymNode(disp)) + # attach to itself to prevent bugs: + attachDispatcher(disp, newSymNode(disp)) + return disp + +proc fixupDispatcher(meth, disp: PSym) = + # We may have constructed the dispatcher from a method prototype + # and need to augment the incomplete dispatcher with information + # from later definitions, particularly the resultPos slot. Also, + # the lock level of the dispatcher needs to be updated/checked + # against that of the method. + if disp.ast.sonsLen > resultPos and meth.ast.sonsLen > resultPos and + disp.ast.sons[resultPos] == ast.emptyNode: + disp.ast.sons[resultPos] = copyTree(meth.ast.sons[resultPos]) + + # The following code works only with lock levels, so we disable + # it when they're not available. + when declared(TLockLevel): + proc `<`(a, b: TLockLevel): bool {.borrow.} + proc `==`(a, b: TLockLevel): bool {.borrow.} + if disp.typ.lockLevel == UnspecifiedLockLevel: + disp.typ.lockLevel = meth.typ.lockLevel + elif meth.typ.lockLevel != UnspecifiedLockLevel and + meth.typ.lockLevel != disp.typ.lockLevel: + message(meth.info, warnLockLevel, + "method has lock level $1, but another method has $2" % + [$meth.typ.lockLevel, $disp.typ.lockLevel]) + # XXX The following code silences a duplicate warning in + # checkMethodeffects() in sempass2.nim for now. + if disp.typ.lockLevel < meth.typ.lockLevel: + disp.typ.lockLevel = meth.typ.lockLevel + proc methodDef*(s: PSym, fromCache: bool) = var L = len(gMethods) for i in countup(0, L - 1): - let disp = gMethods[i][0] + var disp = gMethods[i].dispatcher if sameMethodBucket(disp, s): - add(gMethods[i], s) + add(gMethods[i].methods, s) attachDispatcher(s, lastSon(disp.ast)) + fixupDispatcher(s, disp) when useEffectSystem: checkMethodEffects(disp, s) return - add(gMethods, @[s]) # create a new dispatcher: - if not fromCache: - var disp = copySym(s) - incl(disp.flags, sfDispatcher) - excl(disp.flags, sfExported) - disp.typ = copyType(disp.typ, disp.typ.owner, false) - # we can't inline the dispatcher itself (for now): - if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault - disp.ast = copyTree(s.ast) - disp.ast.sons[bodyPos] = ast.emptyNode - disp.loc.r = nil - if s.typ.sons[0] != nil: - disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) - attachDispatcher(s, newSymNode(disp)) - # attach to itself to prevent bugs: - attachDispatcher(disp, newSymNode(disp)) + add(gMethods, (methods: @[s], dispatcher: createDispatcher(s))) + if fromCache: + internalError(s.info, "no method dispatcher found") proc relevantCol(methods: TSymSeq, col: int): bool = # returns true iff the position is relevant @@ -195,8 +234,9 @@ proc generateMethodDispatchers*(): PNode = result = newNode(nkStmtList) for bucket in countup(0, len(gMethods) - 1): var relevantCols = initIntSet() - for col in countup(1, sonsLen(gMethods[bucket][0].typ) - 1): - if relevantCol(gMethods[bucket], col): incl(relevantCols, col) - sortBucket(gMethods[bucket], relevantCols) - addSon(result, newSymNode(genDispatcher(gMethods[bucket], relevantCols))) + for col in countup(1, sonsLen(gMethods[bucket].methods[0].typ) - 1): + if relevantCol(gMethods[bucket].methods, col): incl(relevantCols, col) + sortBucket(gMethods[bucket].methods, relevantCols) + addSon(result, + newSymNode(genDispatcher(gMethods[bucket].methods, relevantCols))) diff --git a/compiler/transf.nim b/compiler/transf.nim index 20fd5620f3..1ec31d605c 100644 --- a/compiler/transf.nim +++ b/compiler/transf.nim @@ -625,10 +625,9 @@ proc transformCall(c: PTransf, n: PNode): PTransNode = # use the dispatcher for the call: if s.sons[0].kind == nkSym and s.sons[0].sym.kind == skMethod: let t = lastSon(s.sons[0].sym.ast) - if t.kind == nkSym and sfDispatcher in t.sym.flags: - result = methodCall(s).PTransNode - else: - result = s.PTransNode + if t.kind != nkSym or sfDispatcher notin t.sym.flags: + methodDef(s.sons[0].sym, false) + result = methodCall(s).PTransNode else: result = s.PTransNode diff --git a/tests/method/tmproto.nim b/tests/method/tmproto.nim new file mode 100644 index 0000000000..5d75cff1a0 --- /dev/null +++ b/tests/method/tmproto.nim @@ -0,0 +1,25 @@ +type + Obj1 = ref object {.inheritable.} + Obj2 = ref object of Obj1 + +method beta(x: Obj1): int + +proc delta(x: Obj2): int = + beta(x) + +method beta(x: Obj2): int + +proc alpha(x: Obj1): int = + beta(x) + +method beta(x: Obj1): int = 1 +method beta(x: Obj2): int = 2 + +proc gamma(x: Obj1): int = + beta(x) + +doAssert alpha(Obj1()) == 1 +doAssert gamma(Obj1()) == 1 +doAssert alpha(Obj2()) == 2 +doAssert gamma(Obj2()) == 2 +doAssert delta(Obj2()) == 2 From a7f5d550c1567f9b150fc73891e1afeb4623cfba Mon Sep 17 00:00:00 2001 From: Simon Krauter Date: Mon, 3 Nov 2014 12:18:45 +0100 Subject: [PATCH 6/6] Added test case --- tests/stdlib/tosprocterminate.nim | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 tests/stdlib/tosprocterminate.nim diff --git a/tests/stdlib/tosprocterminate.nim b/tests/stdlib/tosprocterminate.nim new file mode 100644 index 0000000000..fd044414c5 --- /dev/null +++ b/tests/stdlib/tosprocterminate.nim @@ -0,0 +1,21 @@ +import os, osproc + +when defined(Windows): + const ProgramWhichDoesNotEnd = "notepad" +else: + const ProgramWhichDoesNotEnd = "/bin/sh" + +echo("starting " & ProgramWhichDoesNotEnd) +var process = startProcess(ProgramWhichDoesNotEnd) +sleep(500) +echo("stopping process") +process.terminate() +var TimeToWait = 5000 +while process.running() and TimeToWait > 0: + sleep(100) + TimeToWait = TimeToWait - 100 + +if process.running(): + echo("FAILED") +else: + echo("SUCCESS")