bugfix: finally sections are executed before return/break

7 files changed, 71 insertions, 49 deletions

diff --git a/lib/system.nim b/lib/system.nim
index abb3dd6e0..dad8d2d79 100755
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -1232,6 +1232,9 @@ template accumulateResult*(iter: expr) =
   result = @[]
   for x in iter: add(result, x)
 
+# we have to compute this here before turning it off in except.nim anyway ...
+const nimrodStackTrace = compileOption("stacktrace")
+
 {.push checks: off, line_dir: off, debugger: off.}  
 # obviously we cannot generate checking operations here :-)
 # because it would yield into an endless recursion
diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim
index 65e233476..938583bf7 100755
--- a/lib/system/excpt.nim
+++ b/lib/system/excpt.nim
@@ -61,7 +61,6 @@ proc popCurrentException {.compilerRtl, inl.} =
 
 # some platforms have native support for stack traces:
 const
-  nimrodStackTrace = compileOption("stacktrace")
   nativeStackTrace = (defined(macosx) or defined(linux)) and 
                      not nimrodStackTrace and false
 
diff --git a/rod/ccgstmts.nim b/rod/ccgstmts.nim
index e60648184..b888b44c6 100755
--- a/rod/ccgstmts.nim
+++ b/rod/ccgstmts.nim
@@ -27,17 +27,6 @@ proc genLineDir(p: BProc, t: PNode) =
     appf(p.s[cpsStmts], "F.line = $1;F.filename = $2;$n", 
         [toRope(line), makeCString(toFilename(t.info).extractFilename)])
 
-proc popSafePoints(p: BProc, howMany: int) = 
-  for i in countup(1, howMany): 
-    appcg(p, cpsStmts, "#popSafePoint();$n", [])
-
-proc genReturnStmt(p: BProc, t: PNode) = 
-  p.beforeRetNeeded = true
-  genLineDir(p, t)
-  if (t.sons[0] != nil): genStmts(p, t.sons[0])
-  popSafePoints(p, p.nestedTryStmts)
-  appff(p.s[cpsStmts], "goto BeforeRet;$n", "br label %BeforeRet$n", [])
-
 proc genVarTuple(p: BProc, n: PNode) = 
   var 
     L: int
@@ -143,6 +132,31 @@ proc genIfStmt(p: BProc, n: PNode) =
     else: internalError(n.info, "genIfStmt()")
   if sonsLen(n) > 1: fixLabel(p, Lend)
   
+proc popSafePoints(p: BProc, howMany: int) = 
+  var L = p.nestedTryStmts.len
+  # danger of endless recursion! we workaround this here, by a temp stack
+  var stack: seq[PNode]
+  newSeq(stack, howMany)
+  for i in countup(1, howMany): 
+    stack[i-1] = p.nestedTryStmts[L-i]
+  setLen(p.nestedTryStmts, L-howMany)
+  
+  for tryStmt in items(stack):
+    appcg(p, cpsStmts, "#popSafePoint();$n", [])
+    var finallyStmt = lastSon(tryStmt)
+    if finallyStmt.kind == nkFinally: 
+      genStmts(p, finallyStmt.sons[0])
+  # push old elements again:
+  for i in countdown(howMany-1, 0): 
+    p.nestedTryStmts.add(stack[i])
+
+proc genReturnStmt(p: BProc, t: PNode) = 
+  p.beforeRetNeeded = true
+  popSafePoints(p, min(1, p.nestedTryStmts.len))
+  genLineDir(p, t)
+  if (t.sons[0] != nil): genStmts(p, t.sons[0])
+  appff(p.s[cpsStmts], "goto BeforeRet;$n", "br label %BeforeRet$n", [])
+  
 proc genWhileStmt(p: BProc, t: PNode) = 
   # we don't generate labels here as for example GCC would produce
   # significantly worse code
@@ -157,7 +171,7 @@ proc genWhileStmt(p: BProc, t: PNode) =
   length = len(p.blocks)
   setlen(p.blocks, length + 1)
   p.blocks[length].id = - p.labels # negative because it isn't used yet
-  p.blocks[length].nestedTryStmts = p.nestedTryStmts
+  p.blocks[length].nestedTryStmts = p.nestedTryStmts.len
   app(p.s[cpsStmts], "while (1) {" & tnl)
   initLocExpr(p, t.sons[0], a)
   if (t.sons[0].kind != nkIntLit) or (t.sons[0].intVal == 0): 
@@ -178,8 +192,8 @@ proc genBlock(p: BProc, t: PNode, d: var TLoc) =
     sym.loc.k = locOther
     sym.loc.a = idx
   setlen(p.blocks, idx + 1)
-  p.blocks[idx].id = - p.labels # negative because it isn't used yet
-  p.blocks[idx].nestedTryStmts = p.nestedTryStmts
+  p.blocks[idx].id = -p.labels # negative because it isn't used yet
+  p.blocks[idx].nestedTryStmts = p.nestedTryStmts.len
   if t.kind == nkBlockExpr: genStmtListExpr(p, t.sons[1], d)
   else: genStmts(p, t.sons[1])
   if p.blocks[idx].id > 0: 
@@ -187,7 +201,6 @@ proc genBlock(p: BProc, t: PNode, d: var TLoc) =
   setlen(p.blocks, idx)
 
 proc genBreakStmt(p: BProc, t: PNode) = 
-  genLineDir(p, t)
   var idx = len(p.blocks) - 1
   if t.sons[0] != nil: 
     # named break?
@@ -196,7 +209,8 @@ proc genBreakStmt(p: BProc, t: PNode) =
     assert(sym.loc.k == locOther)
     idx = sym.loc.a
   p.blocks[idx].id = abs(p.blocks[idx].id) # label is used
-  popSafePoints(p, p.nestedTryStmts - p.blocks[idx].nestedTryStmts)
+  popSafePoints(p, p.nestedTryStmts.len - p.blocks[idx].nestedTryStmts)
+  genLineDir(p, t)
   appf(p.s[cpsStmts], "goto LA$1;$n", [toRope(p.blocks[idx].id)])
 
 proc genAsmStmt(p: BProc, t: PNode) = 
@@ -286,7 +300,7 @@ proc genCaseSecondPass(p: BProc, t: PNode, labId: int) =
 proc genCaseGeneric(p: BProc, t: PNode, rangeFormat, eqFormat: TFormatStr) = 
   # generate a C-if statement for a Nimrod case statement
   var a: TLoc
-  initLocExpr(p, t.sons[0], a) # fist pass: gnerate ifs+goto:
+  initLocExpr(p, t.sons[0], a) # fist pass: generate ifs+goto:
   var labId = p.labels
   for i in countup(1, sonsLen(t) - 1): 
     inc(p.labels)
@@ -448,11 +462,10 @@ proc genCaseStmt(p: BProc, t: PNode) =
     genOrdinalCase(p, t)
   
 proc hasGeneralExceptSection(t: PNode): bool = 
-  var length, i, blen: int
-  length = sonsLen(t)
-  i = 1
+  var length = sonsLen(t)
+  var i = 1
   while (i < length) and (t.sons[i].kind == nkExceptBranch): 
-    blen = sonsLen(t.sons[i])
+    var blen = sonsLen(t.sons[i])
     if blen == 1: 
       return true
     inc(i)
@@ -494,7 +507,7 @@ proc genTryStmtCpp(p: BProc, t: PNode) =
   if optStackTrace in p.Options: 
     app(p.s[cpsStmts], "framePtr = (TFrame*)&F;" & tnl)
   app(p.s[cpsStmts], "try {" & tnl)
-  inc(p.nestedTryStmts)
+  add(p.nestedTryStmts, t)
   genStmts(p, t.sons[0])
   length = sonsLen(t)
   if t.sons[1].kind == nkExceptBranch: 
@@ -520,8 +533,8 @@ proc genTryStmtCpp(p: BProc, t: PNode) =
     inc(i)
   if t.sons[1].kind == nkExceptBranch: 
     app(p.s[cpsStmts], "}}" & tnl) # end of catch-switch statement
-  popSafePoints(p, p.nestedTryStmts)
-  dec(p.nestedTryStmts)
+  appcg(p, cpsStmts, "#popSafePoint();")
+  discard pop(p.nestedTryStmts)
   if (i < length) and (t.sons[i].kind == nkFinally): 
     genStmts(p, t.sons[i].sons[0])
   if rethrowFlag != nil: 
@@ -560,7 +573,7 @@ proc genTryStmt(p: BProc, t: PNode) =
     app(p.s[cpsStmts], "framePtr = (TFrame*)&F;" & tnl)
   appf(p.s[cpsStmts], "if ($1.status == 0) {$n", [safePoint])
   var length = sonsLen(t)
-  inc(p.nestedTryStmts)
+  add(p.nestedTryStmts, t)
   genStmts(p, t.sons[0])
   appcg(p, cpsStmts, "#popSafePoint();$n} else {$n#popSafePoint();$n")
   var i = 1
@@ -587,7 +600,7 @@ proc genTryStmt(p: BProc, t: PNode) =
            [safePoint])
     inc(i)
   app(p.s[cpsStmts], '}' & tnl) # end of if statement
-  dec(p.nestedTryStmts)
+  discard pop(p.nestedTryStmts)
   if i < length and t.sons[i].kind == nkFinally: 
     genStmts(p, t.sons[i].sons[0])
   appcg(p, cpsStmts, "if ($1.status != 0) #reraiseException();$n", [safePoint])
diff --git a/rod/cgen.nim b/rod/cgen.nim
index 6d39623bb..d4592c185 100755
--- a/rod/cgen.nim
+++ b/rod/cgen.nim
@@ -59,21 +59,21 @@ type
                               # has been used (i.e. the label should be emitted)
     nestedTryStmts*: int      # how many try statements is it nested into
   
-  TCProc{.final.} = object    # represents C proc that is currently generated
-    s*: TCProcSections        # the procs sections; short name for readability
-    prc*: PSym                # the Nimrod proc that this C proc belongs to
-    BeforeRetNeeded*: bool    # true iff 'BeforeRet' label for proc is needed
-    nestedTryStmts*: Natural  # in how many nested try statements we are
-                              # (the vars must be volatile then)
-    labels*: Natural          # for generating unique labels in the C proc
-    blocks*: seq[TBlock]      # nested blocks
-    options*: TOptions        # options that should be used for code
-                              # generation; this is the same as prc.options
-                              # unless prc == nil
-    frameLen*: int            # current length of frame descriptor
-    sendClosure*: PType       # closure record type that we pass
-    receiveClosure*: PType    # closure record type that we get
-    module*: BModule          # used to prevent excessive parameter passing
+  TCProc{.final.} = object   # represents C proc that is currently generated
+    s: TCProcSections        # the procs sections; short name for readability
+    prc: PSym                # the Nimrod proc that this C proc belongs to
+    BeforeRetNeeded: bool    # true iff 'BeforeRet' label for proc is needed
+    nestedTryStmts: seq[PNode] # in how many nested try statements we are
+                               # (the vars must be volatile then)
+    labels: Natural          # for generating unique labels in the C proc
+    blocks: seq[TBlock]      # nested blocks
+    options: TOptions        # options that should be used for code
+                             # generation; this is the same as prc.options
+                             # unless prc == nil
+    frameLen: int            # current length of frame descriptor
+    sendClosure: PType       # closure record type that we pass
+    receiveClosure: PType    # closure record type that we get
+    module: BModule          # used to prevent excessive parameter passing
   
   TTypeSeq = seq[PType]
   TCGen = object of TPassContext # represents a C source file
@@ -158,7 +158,8 @@ proc newProc(prc: PSym, module: BModule): BProc =
   result.module = module
   if prc != nil: result.options = prc.options
   else: result.options = gOptions
-  result.blocks = @ []
+  result.blocks = @[]
+  result.nestedTryStmts = @[]
 
 proc isSimpleConst(typ: PType): bool = 
   result = not (skipTypes(typ, abstractVar).kind in
@@ -373,7 +374,7 @@ proc assignLocalVar(p: BProc, s: PSym) =
   else: 
     app(p.s[cpsLocals], getTypeDesc(p.module, s.loc.t))
     if sfRegister in s.flags: app(p.s[cpsLocals], " register")
-    if (sfVolatile in s.flags) or (p.nestedTryStmts > 0): 
+    if (sfVolatile in s.flags) or (p.nestedTryStmts.len > 0): 
       app(p.s[cpsLocals], " volatile")
     appf(p.s[cpsLocals], " $1;$n", [s.loc.r])
   localDebugInfo(p, s)
diff --git a/tests/accept/run/spec.csv b/tests/accept/run/spec.csv
index ec98fa70e..66eb7eb6a 100755
--- a/tests/accept/run/spec.csv
+++ b/tests/accept/run/spec.csv
@@ -19,7 +19,9 @@ tconstr2.nim;69
 tcopy.nim;TEMP=C:\Programs\xyz\bin
 tcurrncy.nim;25
 texplicitgeneric1.nim;Key: 12 value: 12Key: 13 value: 13 Key: A value: 12 Key: B value: 13
-tfinally.nim;came here
+tfinally.nim;came here 3
+tfinally2.nim;ABCD
+tfinally3.nim;false
 tfloat1.nim;Error: unhandled exception: FPU operation caused an overflow [EFloatOverflow]
 tfloat2.nim;Error: unhandled exception: FPU operation caused a NaN result [EFloatInvalidOp]
 tformat.nim;Hi Andreas! How do you feel, Rumpf?
diff --git a/tests/accept/run/tfinally.nim b/tests/accept/run/tfinally.nim
index df6397125..034653b2b 100755
--- a/tests/accept/run/tfinally.nim
+++ b/tests/accept/run/tfinally.nim
@@ -2,9 +2,14 @@
 
 proc main: int = 
   try:
-    return 1
+    try:
+      return 1
+    finally:
+      stdout.write("came ")
+      return 2
   finally: 
-    echo "came here"
+    stdout.write("here ")
+    return 3
     
-discard main() #OUT came here
+echo main() #OUT came here 1
 
diff --git a/todo.txt b/todo.txt
index 1f461e255..afbb49196 100755
--- a/todo.txt
+++ b/todo.txt
@@ -2,7 +2,6 @@ For version 0.8.10
 ==================
 
 - fix implicit generic routines
-- bugfix: make ``finally`` more robust
 
 
 High priority (version 0.9.0)