fixes finally being skipped when except T as e re-raises (cpp backend)#25775
Open
puffball1567 wants to merge 1 commit intonim-lang:develfrom
Open
fixes finally being skipped when except T as e re-raises (cpp backend)#25775puffball1567 wants to merge 1 commit intonim-lang:develfrom
except T as e re-raises (cpp backend)#25775puffball1567 wants to merge 1 commit intonim-lang:develfrom
Conversation
5d72b24 to
f0edfc3
Compare
[backport]
When a Nim `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`:
try:
try:
raise newException(CatchableError, "orig")
except CatchableError as e:
raise newException(CatchableError, "re:" & e.msg)
finally:
echo "finally" # never runs on cpp backend (arc/orc)
except CatchableError as outer:
echo "outer: ", outer.msg
Expected output: `finally / outer: re:orig`.
Actual output on `nim cpp --mm:arc` (and `--mm:orc`): just
`outer: re:orig` — the `finally` line is missing.
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 dropped
entirely.
The bug is specific to memory managers that use destructor injection
(arc/orc). Under refc, no destructor wrapper is injected, so the
topmost entry is the user's try and the existing check fires
correctly.
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 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.
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`.
f0edfc3 to
056b2fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
When an
except T as e:handler in the cpp backend raises a new exception, the enclosingfinallyblock is silently dropped under--mm:arcand--mm:orc:Expected output:
Actual output on
nim cpp --mm:arc(and--mm:orc):The
finallyline is missing. The bug is specific to memory managers that use destructor injection (arc/orc); under--mm:refcthe 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-generatednkHiddenTryStmtwrapper around the handler body to call=destroyonewhen it goes out of scope. That wrapper sits at the top ofp.nestedTryStmtswithinExcept = false.finallyActions(which inlines the user-finally body before a raise propagates) only inspected the topmost entry ofnestedTryStmts. Because the wrapper hasinExcept = 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(...)/finallyemitted bygenTryCppnever runs either, so the user's finally is silently dropped.Fix
isHiddenflag tonestedTryStmtsentries, set tot.kind == nkHiddenTryStmtso compiler-injected try wrappers can be distinguished from user-written ones.finallyActions, walk pastisHiddenwrappers 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 ecase under arc/orc. Stopping at user trys preserves the existing correct behaviour for nested try/except/finally constructs (e.g.tests/exception/tfinally.nim'snested_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.nimcovering:except T as e:re-raise + outer finallyexcept:re-raise + outer finallyThe test runs on
--mm:arc,--mm:orc, and--mm:refc.Locally verified on both
develandversion-2-2:tests/exception/— 42 PASS, 0 FAIL, 3 SKIPtests/destructor/— all PASStests/cpp/— all PASS (single unrelated failure:tasync_cpp.nimneeds thejesterpackage)megatest— PASS for both--mm:arcand--mm:refc, including the previously regressingtfinally.nim'snested_finallyBackport
Tagged
[backport]in the commit message for inclusion inversion-2-2.