Skip to content

AddressLowering: speculative tightening of assertion #62385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 10, 2022

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Dec 3, 2022

On Windows CI builds, we have observed a failure to build the Swift Standard Library after 10/27 (first noticed on 11/11 snapshot due to other failures). The failure is an invalid isa cast where the instance is a nullptr. The SILPipeline suggests that the SILOptimizer might be the one triggering the incorrect use of isa. The only instance of isa introduced in that range in the SILOptimizer/Mandatory region for that duration is this particular instance. Tighten the assertion to ensure that oper->getUser() returns a non-nullptr value.

Thanks for @gwynne for helping narrow down the search area.

On Windows CI builds, we have observed a failure to build the Swift
Standard Library after 10/27 (first noticed on 11/11 snapshot due to
other failures).  The failure is an invalid `isa` cast where the
instance is a `nullptr`.  The SILPipeline suggests that the
SILOptimizer might be the one triggering the incorrect use of `isa`.
The only instance of `isa` introduced in that range in the
SILOptimizer/Mandatory region for that duration is this particular
instance.  Tighten the assertion to ensure that `oper->getUser()`
returns a non-`nullptr` value.

Thanks for @gwynne for helping narrow down the search area.
@compnerd
Copy link
Member Author

compnerd commented Dec 3, 2022

CC: @etcwilde @nate-chandler

@compnerd
Copy link
Member Author

compnerd commented Dec 3, 2022

@swift-ci please test

@etcwilde
Copy link
Member

etcwilde commented Dec 3, 2022

Yeah, this seems fine to me. The semantics don't change, but it makes it easier to debug without having to sift through a bunch of isa::doIt calls.

@nate-chandler nate-chandler self-requested a review December 3, 2022 23:56
@nate-chandler
Copy link
Contributor

Note that this pass is not yet being run when building the standard library.

@compnerd
Copy link
Member Author

compnerd commented Dec 4, 2022

@compnerd
Copy link
Member Author

Although this is really not the root cause of the current windows issue, I'm going to merge this as this can be helpful in the future. This ensures that we fail at the assertion rather than in isa if the user is ever null.

@compnerd compnerd merged commit 77b5e13 into swiftlang:main Dec 10, 2022
@compnerd compnerd deleted the isa-dead branch December 10, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants