Skip to content

Conversation

@topolarity
Copy link
Member

@topolarity topolarity commented Sep 25, 2024

This change duplicates finally blocks in lowered IR, so that they can have a static nesting depth in the try-catch hierarchy.

Previously, finally control-flow looked like this:

error   non-error
    \   /
     \ /
      |
    finally block
      |
     / \
    /   \
 error   non-error

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 / catch scope). The above finally pattern violates that assumption.

In an upcoming PR, 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.)

Behaviorally this was mostly benign, except for some mis-handling of an erroring entry that turns into a non-erroring exit:

function foo()
    try
        error("foo")
    finally
        return nothing
    end
end

function bar()
    if foo() === nothing
        # ...
    end
    # do things
    error("baz")
end

which spits out internal compiler stack traces (oops!):

julia> bar()
ERROR: baz
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] bar()
   @ Main ./REPL[2]:6
 [3] top-level scope
   @ REPL[3]:1

caused by: foo
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] foo()
    @ Main ./REPL[1]:3
  [3]
    @ Core.Compiler ./compiler/abstractinterpretation.jl:909
  [4]
    @ Core.Compiler ./compiler/abstractinterpretation.jl:794
  [5]
    @ Core.Compiler ./compiler/abstractinterpretation.jl:788
  [6]
    @ Core.Compiler ./compiler/abstractinterpretation.jl:103
# ... <snip>
 [21] typeinf_ext_toplevel(mi::Core.MethodInstance, world::UInt64)
    @ Core.Compiler ./compiler/typeinfer.jl:1078
 [22] top-level scope
    @ REPL[3]:1

That could be fixed by banning break and return within finally blocks or making the lowering more complicated, but this PR instead splits the finally block into an erroring and non-erroring path so that we can attach the catch handler appropriately.

@topolarity topolarity added the compiler:lowering Syntax lowering (compiler front end, 2nd stage) label Sep 25, 2024
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.)
@topolarity
Copy link
Member Author

Planning on landing this today or tomorrow, unless there are any objections.

@topolarity topolarity merged commit 6de6b46 into JuliaLang:master Oct 22, 2024
8 checks passed
@nsajko
Copy link
Member

nsajko commented Dec 25, 2024

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:lowering Syntax lowering (compiler front end, 2nd stage)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants