[6.2] Fix a few issues around nonisolated(nonsending) and protocol witness thunks/vtable thunks #81615
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
Just to make review easier. (cherry picked from commit 71adae4)
…nabled instead of #ifndef NDEBUG to match the rest of the SILVerifier. I think this was just an oversight. There is really no reason that this should not match the rest of the SILVerifier w here we have moved from using #ifndef NDEBUG to verificationEnabled checking. (cherry picked from commit e25c664)
I just renamed a few variables and eliminated a bunch of unnecessary indentation. Just to make it easier to review the actual functional change. (cherry picked from commit 634435d)
…ir requirements. (cherry picked from commit ed1654c)
(cherry picked from commit 20d7a6c)
This will cause tests today to crash since even though we are placing the isolation now, to make it easier to read, I left in the old isolation selecting code. This code uses the witness's isolation instead of the requirement's isolation which is incorrect since the protocol witness thunk needs to look the requirement from an ABI perspective since the two must be substitutable. The crash comes from the ABI verification I added in earlier commits. (cherry picked from commit ff1cbea)
We were using the isolation from the witness not from the requirement which we are supposed to do. The witness thunk thunks the isolation from the requirement to the witness so from an ABI perspective it should have the interface implied by the requirement's isolation since that is what callers of the witness method will expect. rdar://151394209 (cherry picked from commit 39a013f)
…for vtables This ensures that when we generate the vtable thunk for a nonisolated(nonsending) override (or vis-a-versa), we get the ABI correct. I also added tests for all of the relevant cases for vtables that we check for protocols. rdar://151394209 (cherry picked from commit ef23f97)
|
@swift-ci test |
|
@swift-ci test windows platform |
|
@swift-ci test macOS platform |
DougGregor
approved these changes
May 22, 2025
| // from the reference kind, the attributes, or the context. | ||
|
|
||
| if (constant->kind == SILDeclRef::Kind::Deallocator) { | ||
| return ActorIsolation::forNonisolated(false); |
There was a problem hiding this comment.
Does isolated deinit have an impact here?
| args.push_back(emitNonIsolatedIsolation(loc).getValue()); | ||
| break; | ||
| case ActorIsolation::Erased: | ||
| llvm::report_fatal_error("Found erased actor isolation?!"); |
There was a problem hiding this comment.
Can we fail more gracefully here, by reporting a normal error and sticking in an under or something?
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: This PR does three things:
I discovered that we were not validating in the SILVerifier that protocol witness table witnesses were ABI compatible with the requirements. I added that verification. It also showed we had some stale SIL tests that were very old that just were updated incorrectly and thus tripped this verification. I fixed those. This will only come into play when we test release/6.2 with asserts. It will not impact non-release builds.
I changed SILGen to place the correct ActorIsolation on protocol witness thunks. We were using the isolation from the witness, not the requirement. Before this change as a result of placing the wrong isolation in SILFunctionType, we were not inserting the extra implicit isolated parameter on the thunk causing ABI breakage.
While doing this, I also discovered that we were not properly pushing/popping the implicit isolated parameter when producing vtable thunks.
Scope: This modifies how we generate SIL for protocol witness/vtable thunks when nonisolated(nonsending) is involved.
Resolves: rdar://151394209
Main PR: #81542
Risk: Low. This should only impact code that uses nonisolated(nonsending).
Testing: Added compiler tests
Reviewer: @rjmccall