From cbe02aa9de741c0f1fbf6f114b67c86fa3b6f84d Mon Sep 17 00:00:00 2001 From: puffball1567 Date: Tue, 5 May 2026 22:27:33 +0900 Subject: [PATCH] fixes finally being skipped when `except T as e` re-raises (cpp backend) (#25775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Bug When an `except T as e:` handler in the cpp backend raises a new exception, the enclosing `finally` block is silently dropped under `--mm:arc` and `--mm:orc`: ```nim proc main() = try: try: raise newException(CatchableError, "orig") except CatchableError as e: echo "inner: ", e.msg raise newException(CatchableError, "re:" & e.msg) finally: echo "finally" except CatchableError as outer: echo "outer: ", outer.msg main() ``` Expected output: ``` inner: orig finally outer: re:orig ``` Actual output on `nim cpp --mm:arc` (and `--mm:orc`): ``` inner: orig outer: re:orig ``` The `finally` line is missing. The bug is specific to memory managers that use destructor injection (arc/orc); under `--mm:refc` the original code path works correctly because no destructor wrapper is injected. ## Root cause When the body of `except T as e:` is processed under ARC/ORC, the destructor injection pass injects a compiler-generated `nkHiddenTryStmt` wrapper around the handler body to call `=destroy` on `e` when it goes out of scope. That wrapper sits at the top of `p.nestedTryStmts` with `inExcept = false`. `finallyActions` (which inlines the user-finally body before a raise propagates) only inspected the topmost entry of `nestedTryStmts`. Because the wrapper has `inExcept = false`, the check short-circuited and the user's finally was never inlined. After the raise, C++'s rule that sibling catch clauses do not catch each other's throws means the surrounding `catch(...)/finally` emitted by `genTryCpp` never runs either, so the user's finally is silently dropped. ## Fix - Add an `isHidden` flag to `nestedTryStmts` entries, set to `t.kind == nkHiddenTryStmt` so compiler-injected try wrappers can be distinguished from user-written ones. - In `finallyActions`, walk past `isHidden` wrappers but stop at the first user try. If that user try is in its except branch with a finally, inline the finally body before the raise; otherwise leave the raise untouched (the raise will be caught by that user try's own except branches and the inner finally will run via normal unwinding, which is what already happens correctly under refc). Walking past wrappers fixes the `as e` case under arc/orc. Stopping at user trys preserves the existing correct behaviour for nested try/except/finally constructs (e.g. `tests/exception/tfinally.nim`'s `nested_finally`), which would otherwise see the outer finally inlined too eagerly when an inner raise is processed. ## Tests Adds `tests/exception/tcpp_handler_raise_finally.nim` covering: - `except T as e:` re-raise + outer finally - typeless `except:` re-raise + outer finally - try/finally without except (exception propagation through finally) The test runs on `--mm:arc`, `--mm:orc`, and `--mm:refc`. Locally verified on both `devel` and `version-2-2`: - `tests/exception/` — 42 PASS, 0 FAIL, 3 SKIP - `tests/destructor/` — all PASS - `tests/cpp/` — all PASS (single unrelated failure: `tasync_cpp.nim` needs the `jester` package) - `megatest` — PASS for both `--mm:arc` and `--mm:refc`, including the previously regressing `tfinally.nim`'s `nested_finally` ## Backport Tagged `[backport]` in the commit message for inclusion in `version-2-2`. --------- Co-authored-by: puffball1567 <17452514+puffball1567@users.noreply.github.com> --- compiler/ccgstmts.nim | 36 +++++++---- compiler/cgendata.nim | 7 ++- .../exception/tcpp_handler_raise_finally.nim | 61 +++++++++++++++++++ tests/exception/tcpp_imported_exc.nim | 2 +- 4 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 tests/exception/tcpp_handler_raise_finally.nim diff --git a/compiler/ccgstmts.nim b/compiler/ccgstmts.nim index a80ef37efd..5ea23d1f80 100644 --- a/compiler/ccgstmts.nim +++ b/compiler/ccgstmts.nim @@ -230,7 +230,7 @@ proc blockLeaveActions(p: BProc, howManyTrys, howManyExcepts: int, isReturnStmt # Called by return and break stmts. # Deals with issues faced when jumping out of try/except/finally stmts. - var stack = newSeq[tuple[fin: PNode, inExcept: bool, label: Natural]](0) + var stack = newSeq[tuple[fin: PNode, inExcept: bool, isHidden: bool, label: Natural]](0) inc p.withinBlockLeaveActions for i in 1..howManyTrys: @@ -836,12 +836,26 @@ proc raiseExitCleanup(p: BProc, destroy: string) = p.s(cpsStmts).addGoto("LA" & $p.nestedTryStmts[^1].label & "_") proc finallyActions(p: BProc) = - if p.config.exc != excGoto and p.nestedTryStmts.len > 0 and p.nestedTryStmts[^1].inExcept: - # if the current try stmt have a finally block, - # we must execute it before reraising - let finallyBlock = p.nestedTryStmts[^1].fin - if finallyBlock != nil: - genSimpleBlock(p, finallyBlock[0]) + if p.config.exc != excGoto: + # Walk past compiler-injected `nkHiddenTryStmt` wrappers (e.g. ARC's + # destructor try/finally that wraps `except T as e:` bodies) to reach + # the user's actual try. We must NOT walk past a real user try whose + # body we are currently in, because a raise from there will be caught + # by that try's own except branches rather than escaping outward. + # + # If after skipping wrappers the next entry is a user try in its + # except branch (inExcept=true), inline its finally body before the + # raise propagates — without this, the C++ sibling-catch rule would + # cause the user's catch(...)/finally pair to be bypassed and the + # finally would be silently dropped. + for i in countdown(p.nestedTryStmts.high, 0): + if p.nestedTryStmts[i].isHidden: + continue + if p.nestedTryStmts[i].inExcept: + let finallyBlock = p.nestedTryStmts[i].fin + if finallyBlock != nil: + genSimpleBlock(p, finallyBlock[0]) + return proc raiseInstr(p: BProc; result: var Builder) = if p.config.exc == excGoto: @@ -1185,7 +1199,7 @@ proc genTryCpp(p: BProc, t: PNode, d: var TLoc) = lineCg(p, cpsLocals, "std::exception_ptr T$1_;$n", [etmp]) let fin = if t[^1].kind == nkFinally: t[^1] else: nil - p.nestedTryStmts.add((fin, false, 0.Natural)) + p.nestedTryStmts.add((fin, false, t.kind == nkHiddenTryStmt, 0.Natural)) if t.kind == nkHiddenTryStmt: lineCg(p, cpsStmts, "try {$n", []) @@ -1371,7 +1385,7 @@ proc genTryCppOld(p: BProc, t: PNode, d: var TLoc) = genLineDir(p, t) cgsym(p.module, "popCurrentExceptionEx") let fin = if t[^1].kind == nkFinally: t[^1] else: nil - p.nestedTryStmts.add((fin, false, 0.Natural)) + p.nestedTryStmts.add((fin, false, t.kind == nkHiddenTryStmt, 0.Natural)) startBlockWith(p): p.s(cpsStmts).add("try {\n") expr(p, t[0], d) @@ -1450,7 +1464,7 @@ proc genTryGoto(p: BProc; t: PNode; d: var TLoc) = let lab = p.labels let hasExcept = t[1].kind == nkExceptBranch if hasExcept: inc p.withinTryWithExcept - p.nestedTryStmts.add((fin, false, Natural lab)) + p.nestedTryStmts.add((fin, false, t.kind == nkHiddenTryStmt, Natural lab)) p.flags.incl nimErrorFlagAccessed @@ -1656,7 +1670,7 @@ proc genTrySetjmp(p: BProc, t: PNode, d: var TLoc) = initElifBranch(p.s(cpsStmts), nonQuirkyIf, removeSinglePar( cOp(Equal, dotField(safePoint, "status"), cIntValue(0)))) let fin = if t[^1].kind == nkFinally: t[^1] else: nil - p.nestedTryStmts.add((fin, quirkyExceptions, 0.Natural)) + p.nestedTryStmts.add((fin, quirkyExceptions, t.kind == nkHiddenTryStmt, 0.Natural)) expr(p, t[0], d) var quirkyIf = default(IfBuilder) var quirkyScope = default(ScopeBuilder) diff --git a/compiler/cgendata.nim b/compiler/cgendata.nim index 5b5668024a..fb8f2086cb 100644 --- a/compiler/cgendata.nim +++ b/compiler/cgendata.nim @@ -75,10 +75,13 @@ type flags*: set[TCProcFlag] lastLineInfo*: TLineInfo # to avoid generating excessive 'nimln' statements currLineInfo*: TLineInfo # AST codegen will make this superfluous - nestedTryStmts*: seq[tuple[fin: PNode, inExcept: bool, label: Natural]] + nestedTryStmts*: seq[tuple[fin: PNode, inExcept: bool, isHidden: bool, label: Natural]] # in how many nested try statements we are # (the vars must be volatile then) - # bool is true when are in the except part of a try block + # `inExcept` is true when we are in the except part of a try block. + # `isHidden` is true for compiler-injected `nkHiddenTryStmt` wrappers + # (e.g. ARC's destructor try/finally around `except T as e:` bodies); + # finallyActions walks past such wrappers to reach the user's try. finallySafePoints*: seq[Rope] # For correctly cleaning up exceptions when # using return in finally statements labels*: Natural # for generating unique labels in the C proc diff --git a/tests/exception/tcpp_handler_raise_finally.nim b/tests/exception/tcpp_handler_raise_finally.nim new file mode 100644 index 0000000000..880903d5a9 --- /dev/null +++ b/tests/exception/tcpp_handler_raise_finally.nim @@ -0,0 +1,61 @@ +discard """ + targets: "cpp" + matrix: "--mm:arc; --mm:orc; --mm:refc" + output: ''' +inner: orig +finally +outer: re:orig +inner-typeless: orig +finally-typeless +outer-typeless: re-tl:orig +no-catch-finally +caught-propagated: prop +''' +""" + +# When an `except` handler raises a new exception, the enclosing `finally` +# block must still run before the new exception propagates to the outer +# try. +# +# The C++ backend previously emitted the finally's `catch (...)` as a +# sibling of the user-written catches. C++ does not allow sibling catches +# to catch each other's throws, so a handler-raised exception bypassed the +# finally entirely. The fix wraps the inner try/catch sequence in an +# outer try, so any escaping exception (whether from the body or from a +# handler) is captured before the finally runs. + +block typed_except: + try: + try: + raise newException(CatchableError, "orig") + except CatchableError as e: + echo "inner: ", e.msg + raise newException(CatchableError, "re:" & e.msg) + finally: + echo "finally" + except CatchableError as outer: + echo "outer: ", outer.msg + +block typeless_except: + try: + try: + raise newException(CatchableError, "orig") + except: + let e = getCurrentException() + echo "inner-typeless: ", e.msg + raise newException(CatchableError, "re-tl:" & e.msg) + finally: + echo "finally-typeless" + except CatchableError as outer: + echo "outer-typeless: ", outer.msg + +# try/finally without an except: the body's exception must still propagate +# after the finally runs. +block no_catch_finally: + try: + try: + raise newException(CatchableError, "prop") + finally: + echo "no-catch-finally" + except CatchableError as e: + echo "caught-propagated: ", e.msg diff --git a/tests/exception/tcpp_imported_exc.nim b/tests/exception/tcpp_imported_exc.nim index 0c7846956b..8ee008bd96 100644 --- a/tests/exception/tcpp_imported_exc.nim +++ b/tests/exception/tcpp_imported_exc.nim @@ -1,5 +1,5 @@ discard """ -matrix: "--mm:refc" +matrix: "--mm:refc; --mm:orc" targets: "cpp" output: ''' caught as std::exception