codegen: Restore ct->scope in jl_eh_restore_state
#55907
Merged
Conversation
This file contains 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
This eliminates the need to associate a `catch` with every `with(...) do ... end` block, which was really just acting as a landing pad to restore `jl_current_task->scope` in the majority of cases. This change does not actually update lowering to remove the unnecessary `catch` block - that's left as a follow-up.
topolarity
force-pushed
the
ct/handler-scope
branch
from
b01d1d1
to
c5dc41f
Compare
|
Since yesterday we see the following error in our nightly CI (see thofma/Hecke.jl#1687): TypeError: in typeassert, expected Union{Nothing, Base.ScopedValues.Scope}, got a value of type ZZRingElem
Stacktrace:
[1] get
@ ./scopedvalues.jl:152 [inlined]
[2] _precision_with_base_2
@ ./mpfr.jl:1041 [inlined]
[3] BigFloat
@ ./mpfr.jl:150 [inlined]
[4] _arf_get_mpfr(t::Nemo.arf_struct, ::RoundingMode{:Nearest})
@ Nemo ~/.julia/packages/Nemo/0TzOv/src/arb/arb.jl:129
[...]From the set of changes since this appeared I assume this is due to this PR, but I am not sure if this PR is faulty or just brings already existing misusage of cc @thofma |
|
@thofma reported to see TypeError: in typeassert, expected Union{Nothing, Base.ScopedValues.Scope}, got a value of type Tuple{Base.MP
FR.MPFRRoundingMode}which seems to be unrelated to any So this seems to be a genuine issue with this PR. |
topolarity
added a commit
to topolarity/julia
that referenced
this pull request
Nov 19, 2024
…itted This fixes a bug introduced by JuliaLang#55907, which was neglecting that it's possible for `Expr(:enter, ...)` to have no `catch` destination and still have a scope, especially if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination.
|
Thanks for the heads up @lgoettgens, just opened #56612 to hopefully fix the regression |
topolarity
added a commit
to topolarity/julia
that referenced
this pull request
Nov 19, 2024
…itted This fixes a bug introduced by JuliaLang#55907, which was neglecting that it's possible for `Expr(:enter, ...)` to have no `catch` destination and still have a scope, especially if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination.
topolarity
added a commit
to topolarity/julia
that referenced
this pull request
Nov 19, 2024
…itted This fixes a bug introduced by JuliaLang#55907, which was neglecting that it's possible for `Expr(:enter, ...)` to have no `catch` destination and still have a scope, especially if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination.
topolarity
added a commit
to topolarity/julia
that referenced
this pull request
Nov 19, 2024
…itted This fixes a bug introduced by JuliaLang#55907, which was neglecting that it's possible for `Expr(:enter, ...)` to have no `catch` destination and still have a scope, especially if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination.
topolarity
added a commit
to topolarity/julia
that referenced
this pull request
Nov 19, 2024
…itted This fixes a bug introduced by JuliaLang#55907, which was neglecting that it's possible for `Expr(:enter, ...)` to have no `catch` destination and still have a scope, especially if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination.
topolarity
added a commit
that referenced
this pull request
Nov 19, 2024
…itted (#56612) This fixes a bug introduced by #55907, which was neglecting that it's possible for `EnterNode` to have no `catch` destination and still have a scope. This can especially happen if the compiler has decided that the body is `nothrow` and chooses to optimize away the `catch` destination, but also #55907 intended to make the scope-only form of `:enter` legal (and not need an exception handler) even if the body is _not_ `nothrow`. This fixes all that up to restore the scope correctly on the happy path. ~~Needs tests - will add those soon~~
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 eliminates the need to associate a
catchwith everywith(...) do ... endblock, which was really just acting as a landing pad to restorejl_current_task->scopein the majority of cases.This change does not actually update lowering to remove the unnecessary
catchblock - that's left as a follow-up.