lowering: split finally blocks for exceptional control-flow#55876
Merged
topolarity merged 1 commit intoJuliaLang:masterfrom Oct 22, 2024
Merged
lowering: split finally blocks for exceptional control-flow#55876topolarity merged 1 commit intoJuliaLang:masterfrom
finally blocks for exceptional control-flow#55876topolarity merged 1 commit intoJuliaLang:masterfrom
Conversation
This change duplicates `finally` blocks in lowered IR, so that they can
have a static nesting depth in the `try-catch` hierarchy.
Previously, a `finally` block had many non-excepting control-flow paths
and an exceptional control-flow path which passed through the same
block:
```
error non-error
\ /
\ /
|
finally block
|
/ \
/ \
error non-error
```
The branching logic dynamically guaranteed that the `try/catch` blocks
are respected here (you never flow in an error path and out a non-error
one), but the compiler doesn't know that.
In a couple places, the compiler assumes that it can actually "color"
the CFG such that there is a static nesting depth at each BasicBlock
(i.e. each BasicBlock can be labeled w/ a unique enclosing `try` /
`catch` scope). The above `finally` pattern violates that assumption.
This was mostly benign as far as I can tell, but I want to extend the
lifetimes of our Event Handlers (`jl_handler_t`) until the end of a
`catch` block (rather than the start) which noticeably breaks
`llvm-lower-handlers.cpp`.
(@Keno was very clear about this assumption in the comments for that pass.)
fa25070 to
02faff9
Compare
Member
Author
|
Planning on landing this today or tomorrow, unless there are any objections. |
Member
|
This causes spurious syntax errors: #56904 |
giordano
pushed a commit
that referenced
this pull request
Feb 20, 2025
Fixes #56904. The associated PR (#55876) compiles a finally block, then compiles a renumbered version of it. This works if `compile` doesn't mutate its input, but in reality, lambda bodies were being `set!` when linearized. The "invalid syntax" error was a result of attempting to linearize a lambda twice.
KristofferC
pushed a commit
that referenced
this pull request
Feb 21, 2025
Fixes #56904. The associated PR (#55876) compiles a finally block, then compiles a renumbered version of it. This works if `compile` doesn't mutate its input, but in reality, lambda bodies were being `set!` when linearized. The "invalid syntax" error was a result of attempting to linearize a lambda twice. (cherry picked from commit 414aca2)
topolarity
pushed a commit
that referenced
this pull request
Apr 9, 2026
Alternative to #61523, fix JuliaLang/JuliaLowering.jl#171. Push to `ctx.handler_token_stack` before construction of the `FinallyHandler`, since the `JumpTarget` constructor needs it. The extra entry is ignored in `enter_finally_block`'s call to `emit_leave_handler`, but not in the one in `emit_break`. These changes should directly match flisp. I think we're still missing some `pop_exception`s compared to flisp, but wanted to get this PR up given the other one. As noted in the tests, I need to port #55876, which duplicates the finally block and only adds `pop_exception` to one copy.
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.
This change duplicates
finallyblocks in lowered IR, so that they can have a static nesting depth in thetry-catchhierarchy.Previously,
finallycontrol-flow looked like this:This kind of flow is a problem, because in a couple places the compiler assumes that it can actually "color" the CFG such that there is a static nesting depth at each BasicBlock (i.e. each BasicBlock can be labeled w/ a unique enclosing
try/catchscope). The abovefinallypattern violates that assumption.In an upcoming PR, I want to extend the lifetimes of our Event Handlers (
jl_handler_t) until the end of acatchblock (rather than the start) which noticeably breaksllvm-lower-handlers.cpp. (@Keno was very clear about this assumption in the comments for that pass.)Behaviorally this was mostly benign, except for some mis-handling of an erroring entry that turns into a non-erroring exit:
which spits out internal compiler stack traces (oops!):
That could be fixed by banning
breakandreturnwithinfinallyblocks or making the lowering more complicated, but this PR instead splits thefinallyblock into an erroring and non-erroring path so that we can attach thecatchhandler appropriately.