[IRGen/Distributed] Adjust protocol requirement and witness table to handle distributed thunks #60184
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
|
@swift-ci please test |
…lRef` While building witness table record, let's use `hasAsync()` check on `SILDeclRef` of the requirement instead of reaching for the underlying declaration because the reference should be the source of truth about `async` and other attributes. In case of distributed thunk witness, which is always `async throws`, the underlying declaration could be sync because it's the protocol requirement declaration.
… async Fixes a bug where descriptor for protocol requirement associated with distributed thunk wasn't marked as `async` that results in ptrauth failures because discriminator would be misaligned between requirement and witness. Resolves: rdar://96520492
|
@swift-ci please test |
|
@swift-ci please test apple silicon |
1 similar comment
|
@swift-ci please test apple silicon |
ktoso
reviewed
Jul 21, 2022
| if (entry.getFunction().isDistributedThunk()) { | ||
| flags = flags.withIsAsync(true); | ||
| declRef = declRef.asDistributed(); | ||
| } |
There was a problem hiding this comment.
okey so this piece we did yesterday did matter... 👍
ktoso
reviewed
Jul 21, 2022
| @@ -1388,8 +1389,9 @@ class AccessorConformanceInfo : public ConformanceInfo { | |||
| witness = llvm::ConstantExpr::getBitCast(witness, IGM.Int8PtrTy); | |||
|
|
|||
| PointerAuthSchema schema = | |||
| afd->hasAsync() ? IGM.getOptions().PointerAuth.AsyncProtocolWitnesses | |||
| : IGM.getOptions().PointerAuth.ProtocolWitnesses; | |||
| isAsyncRequirement | |||
There was a problem hiding this comment.
aha that's what we were missing I see 👀
|
Apple silicon failing to start, times out; |
|
@swift-ci please test apple silicon |
|
Also resolves rdar://97228310 |
|
swiftlang/swift-package-manager#5684 |
|
The revert got merged so the above AS run failed: |
|
@swift-ci please test apple silicon |
1 similar comment
|
@swift-ci please test apple silicon |
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.
Determine whether protocol requirement is async using
SILDeclRefThis is necessary because SILDeclRef of the distributed thunk witness is anchored
on the requirement declaration which might not be async, but the witness itself is
always
async throws.Mark protocol requirement descriptor for distributed thunk as async
Fixes a bug where descriptor for protocol requirement associated with
distributed thunk wasn't marked as
asyncthat results in ptrauth failuresbecause discriminator would be misaligned between requirement and witness.
Resolves: rdar://96520492
Resolves: rdar://97228310