Automatically :leave the exception frame on the catch edge #52245
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
aviatesk
reviewed
Nov 21, 2023
aviatesk
reviewed
Nov 21, 2023
vtjnash
reviewed
Nov 22, 2023
Keno
added a commit
that referenced
this pull request
Nov 24, 2023
This is cherry-picked from #52245. This is an independent bugfix, and looks like #52245 might need another round of discussion. There were two separate off-by-1's in the codegen code that is trying to detect assignments to slots inside try/catch regions. First, it was asking to include the value of the catch label, which is actually the first statement *not* in the try region. Second, there was a confusion of 0 and 1 based indexing in the iteration bounds. The end result of this was that the code was also looking at the first two statements of the catch region. This wasn't a problem before #52245 (other than a potentially over-conservative marking of some slots as volatile), because our catch blocks always had at least two statements (a :leave and a terminator), but with the `:leave` change, it is possible to have catch blocks with only one statement. If these happened to be at the end of the function, things would blow up. As a side node, this code isn't particularly sound, because it assumes that try/catch regions are lexical, which they are not. The assumption happens to work out ok for the code we generate in the frontend and optimized IR doesn't have slots, so we don't use this code, but it is not in general sound.
Right now, we require a :leave expression at both the end of a try region and as the first expression in the catch block. This doesn't really make sense. Throwing the exception should leave the exception frame implicitly. This isn't a huge saving, but does save one IR node (and generated call in the native code) per try/catch block.
|
Updated as discussed. |
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Dec 9, 2023
This is cherry-picked from JuliaLang#52245. This is an independent bugfix, and looks like JuliaLang#52245 might need another round of discussion. There were two separate off-by-1's in the codegen code that is trying to detect assignments to slots inside try/catch regions. First, it was asking to include the value of the catch label, which is actually the first statement *not* in the try region. Second, there was a confusion of 0 and 1 based indexing in the iteration bounds. The end result of this was that the code was also looking at the first two statements of the catch region. This wasn't a problem before JuliaLang#52245 (other than a potentially over-conservative marking of some slots as volatile), because our catch blocks always had at least two statements (a :leave and a terminator), but with the `:leave` change, it is possible to have catch blocks with only one statement. If these happened to be at the end of the function, things would blow up. As a side node, this code isn't particularly sound, because it assumes that try/catch regions are lexical, which they are not. The assumption happens to work out ok for the code we generate in the frontend and optimized IR doesn't have slots, so we don't use this code, but it is not in general sound.
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Dec 9, 2023
…#52245) Right now, we require a :leave expression at both the end of a try region and as the first expression in the catch block. This doesn't really make sense. Throwing the exception should leave the exception frame implicitly. This isn't a huge saving, but does save one IR node (and generated call in the native code) per try/catch block.
vtjnash
added a commit
to vtjnash/JuliaInterpreter.jl
that referenced
this pull request
Mar 7, 2024
aviatesk
pushed a commit
to JuliaDebug/JuliaInterpreter.jl
that referenced
this pull request
Mar 10, 2024
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.
Right now, we require a :leave expression at both the end of a try region and as the first expression in the catch block. This doesn't really make sense. Throwing the exception should leave the exception frame implicitly. This isn't a huge saving, but does save one IR node (and generated call in the native code) per try/catch block.