[6.0] SILGen: Move error values into indirect error returns in proper order with cleanups #73093
Conversation
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
… with cleanups. There's an unfortunate layering difference in the cleanup order between address-only and loadable error values during `catch` pattern matching: for address-only values, the value is copied into a temporary stack slot, and the stack slot is cleaned up on exit from the pattern match, meaning the value must be moved into the error return slot on the "no catch" case before cleanups run. But if it's a loadable value, then we borrow it for the duration of the switch, and the borrow is released during cleanup on exit from the pattern match, so the value must be forwarded after running cleanups. The way the code is structured, it handles these cases properly when the convention of the function being emitted is in sync with the fundamental properties of the error type (when the error type is loadable and the error return is by value, or when the error type is address-only and the error return is indirect, in other words). But when a closure literal with a loadable error type is emitted in an argument context that expects a function with an indirect error return, we would try to forward the loadable error value into the error return slot while a borrow is still active on it, leading to verifier errors. Defer forwarding the value into memory until after cleanups are popped, fixing rdar://126576356. A tidier solution might be to always emit the function body to use a bbarg on the throw block to pass the error value from the body emission to the epilog when the type is loadable, deferring the move into memory to the epilog block. This would make the right behavior fall out of the existing implementation, but would require a bit more invasive changes (pretty much everywhere that checks IndirectErrorReturn would need to check a different-tracked AddressOnlyErrorType bit instead or in addition). This change is more localized.
|
@swift-ci Please test |
tbkka
approved these changes
Apr 18, 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.
Explanation: Fixes a bug where a closure literal that throws untyped errors or another "loadable" error type but which appears in an argument context where the callee expects an indirect generic error return would lead to a crash or miscompile.
Scope: Bug fix for typed throws.
Issue: rdar://126576356
Original PR: #73092
Risk: Low. Fixes a targeted use case.
Testing: Swift CI, test case from issue
Reviewer: @eeckstein