Skip to content

Commit 03de964

Browse files
slavapestovslavapestov
committed
AST: Change RequirementEnvironment::getRequirementToWitnessThunkSubs() to use contextual types
In the provided test case, the generic signature of the protocol requirement is <Self, T where Self == T.A, T: P2> The conformance requirement `Self: P1` is derived from `T: P2` and `Self.A: P1` in P1. So given a substitution map `{ Self := X, T := T }` that replaces T with a concrete type X and T with a type parameter T, there are two ways to recover a substituted conformance for `Self: P1`: - We can apply the substitution map to Self to get X, and look up conformance of X to P1, to get a concrete conformance. - We can evaluate the conformance path `(T: P2)(Self.A: P1)`, to get an abstract conformance. Both answers are correct, but SILGenModule::emitProtocolWitness() was assuming it would always get a concrete conformance back. This was the case until e3c8f42, but then we started returning an abstract conformance. SILGen would then mangle the protocol witness thunk in a way that was not sufficiently unique, and as a result, we could miscompile a program where two witness tables both hit this same scenario. By using contextual types in the getRequirementToWitnessThunkSubs() substitution map, we ensure that evaluating the conformance path against the substitution map produces the same result as performing the global lookup. Also, to prevent this from happening again, add a check to SILGen to guard against emitting two protocol witness thunks with the same mangled name. Unfortunately, this is done intentionally as part of some backward deployment logic for coroutine accessors. There is a hack to allow duplicate thunks with the same name in this case, but this should be revisited some day. Fixes rdar://problem/155624135..
1 parent dbedb21 commit 03de964

File tree

7 files changed

+95
-36
lines changed

7 files changed

+95
-36
lines changed

‎include/swift/AST/RequirementEnvironment.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ class RequirementEnvironment {
114114
}
115115

116116
/// Retrieve the substitution map that maps the interface types of the
117-
/// requirement to the interface types of the witness thunk signature.
117+
/// requirement to the archetypes of the witness thunk signature's
118+
/// generic environment.
118119
SubstitutionMap getRequirementToWitnessThunkSubs() const {
119120
return reqToWitnessThunkSigMap;
120121
}

‎lib/AST/RequirementEnvironment.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "swift/AST/ASTContext.h"
2020
#include "swift/AST/Decl.h"
2121
#include "swift/AST/DeclContext.h"
22+
#include "swift/AST/GenericEnvironment.h"
2223
#include "swift/AST/GenericSignature.h"
2324
#include "swift/AST/ProtocolConformance.h"
2425
#include "swift/AST/TypeCheckRequests.h"
@@ -156,6 +157,11 @@ RequirementEnvironment::RequirementEnvironment(
156157
reqSig.getRequirements().size() == 1) {
157158
witnessThunkSig = conformanceDC->getGenericSignatureOfContext()
158159
.getCanonicalSignature();
160+
if (witnessThunkSig) {
161+
reqToWitnessThunkSigMap = reqToWitnessThunkSigMap.subst(
162+
witnessThunkSig.getGenericEnvironment()
163+
->getForwardingSubstitutionMap());
164+
}
159165
return;
160166
}
161167

@@ -219,4 +225,7 @@ RequirementEnvironment::RequirementEnvironment(
219225
std::move(genericParamTypes),
220226
std::move(requirements),
221227
/*allowInverses=*/false);
228+
reqToWitnessThunkSigMap = reqToWitnessThunkSigMap.subst(
229+
witnessThunkSig.getGenericEnvironment()
230+
->getForwardingSubstitutionMap());
222231
}

‎lib/SILGen/SILGenType.cpp

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,7 @@ SILGenModule::getWitnessTable(NormalProtocolConformance *conformance) {
727727
}
728728

729729
SILFunction *SILGenModule::emitProtocolWitness(
730-
ProtocolConformanceRef conformance, SILLinkage linkage,
730+
ProtocolConformanceRef origConformance, SILLinkage linkage,
731731
SerializedKind_t serializedKind, SILDeclRef requirement,
732732
SILDeclRef witnessRef, IsFreeFunctionWitness_t isFree, Witness witness) {
733733
auto requirementInfo =
@@ -767,7 +767,8 @@ SILFunction *SILGenModule::emitProtocolWitness(
767767
// The type of the witness thunk.
768768
auto reqtSubstTy = cast<AnyFunctionType>(
769769
reqtOrigTy->substGenericArgs(reqtSubMap)
770-
->getReducedType(genericSig));
770+
->mapTypeOutOfContext()
771+
->getCanonicalType());
771772

772773
// Rewrite the conformance in terms of the requirement environment's Self
773774
// type, which might have a different generic signature than the type
@@ -777,14 +778,18 @@ SILFunction *SILGenModule::emitProtocolWitness(
777778
// in a protocol extension, the generic signature will have an additional
778779
// generic parameter representing Self, so the generic parameters of the
779780
// class will all be shifted down by one.
780-
if (reqtSubMap) {
781-
auto requirement = conformance.getProtocol();
782-
auto self = requirement->getSelfInterfaceType()->getCanonicalType();
781+
auto conformance = reqtSubMap.lookupConformance(M.getASTContext().TheSelfType,
782+
origConformance.getProtocol())
783+
.mapConformanceOutOfContext();
784+
ASSERT(conformance.isAbstract() == origConformance.isAbstract());
783785

784-
conformance = reqtSubMap.lookupConformance(self, requirement);
785-
786-
if (genericEnv)
787-
reqtSubMap = reqtSubMap.subst(genericEnv->getForwardingSubstitutionMap());
786+
ProtocolConformance *manglingConformance = nullptr;
787+
if (conformance.isConcrete()) {
788+
manglingConformance = conformance.getConcrete();
789+
if (auto *inherited = dyn_cast<InheritedProtocolConformance>(manglingConformance)) {
790+
manglingConformance = inherited->getInheritedConformance();
791+
conformance = ProtocolConformanceRef(manglingConformance);
792+
}
788793
}
789794

790795
// Generic signatures where all parameters are concrete are lowered away
@@ -806,20 +811,14 @@ SILFunction *SILGenModule::emitProtocolWitness(
806811
// But this is expensive, so we only do it for coroutine lowering.
807812
// When they're part of the AST function type, we can remove this
808813
// parameter completely.
814+
bool allowDuplicateThunk = false;
809815
std::optional<SubstitutionMap> witnessSubsForTypeLowering;
810816
if (auto accessor = dyn_cast<AccessorDecl>(requirement.getDecl())) {
811817
if (accessor->isCoroutine()) {
812818
witnessSubsForTypeLowering =
813819
witness.getSubstitutions().mapReplacementTypesOutOfContext();
814-
}
815-
}
816-
817-
ProtocolConformance *manglingConformance = nullptr;
818-
if (conformance.isConcrete()) {
819-
manglingConformance = conformance.getConcrete();
820-
if (auto *inherited = dyn_cast<InheritedProtocolConformance>(manglingConformance)) {
821-
manglingConformance = inherited->getInheritedConformance();
822-
conformance = ProtocolConformanceRef(manglingConformance);
820+
if (accessor->isRequirementWithSynthesizedDefaultImplementation())
821+
allowDuplicateThunk = true;
823822
}
824823
}
825824

@@ -863,8 +862,9 @@ SILFunction *SILGenModule::emitProtocolWitness(
863862
InlineStrategy = AlwaysInline;
864863

865864
SILFunction *f = M.lookUpFunction(nameBuffer);
866-
if (f)
865+
if (allowDuplicateThunk && f)
867866
return f;
867+
ASSERT(!f);
868868

869869
SILGenFunctionBuilder builder(*this);
870870
f = builder.createFunction(
@@ -891,7 +891,8 @@ SILFunction *SILGenModule::emitProtocolWitness(
891891
bool isPreconcurrency = false;
892892
if (conformance.isConcrete()) {
893893
if (auto *C =
894-
dyn_cast<NormalProtocolConformance>(conformance.getConcrete()))
894+
dyn_cast<NormalProtocolConformance>(
895+
conformance.getConcrete()->getRootConformance()))
895896
isPreconcurrency = C->isPreconcurrency();
896897
}
897898

‎lib/Sema/TypeCheckProtocol.cpp

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,7 +1007,8 @@ findMissingGenericRequirementForSolutionFix(
10071007
};
10081008

10091009
auto selfTy = conformance->getProtocol()->getSelfInterfaceType()
1010-
.subst(reqEnvironment.getRequirementToWitnessThunkSubs());
1010+
.subst(reqEnvironment.getRequirementToWitnessThunkSubs())
1011+
->mapTypeOutOfContext();
10111012

10121013
auto sig = conformance->getGenericSignature();
10131014
auto *env = conformance->getGenericEnvironment();
@@ -1162,14 +1163,9 @@ swift::matchWitness(WitnessChecker::RequirementEnvironmentCache &reqEnvCache,
11621163
// the required type and the witness type.
11631164
cs.emplace(dc, ConstraintSystemFlags::AllowFixes);
11641165

1165-
auto syntheticSig = reqEnvironment.getWitnessThunkSignature();
1166-
auto *syntheticEnv = syntheticSig.getGenericEnvironment();
11671166
auto reqSubMap = reqEnvironment.getRequirementToWitnessThunkSubs();
11681167

11691168
Type selfTy = proto->getSelfInterfaceType().subst(reqSubMap);
1170-
if (syntheticEnv)
1171-
selfTy = syntheticEnv->mapTypeIntoContext(selfTy);
1172-
11731169

11741170
// Open up the type of the requirement.
11751171
SmallVector<OpenedType, 4> reqReplacements;
@@ -1194,10 +1190,6 @@ swift::matchWitness(WitnessChecker::RequirementEnvironmentCache &reqEnvCache,
11941190
if (replacedInReq->hasError())
11951191
continue;
11961192

1197-
if (syntheticEnv) {
1198-
replacedInReq = syntheticEnv->mapTypeIntoContext(replacedInReq);
1199-
}
1200-
12011193
if (auto packType = replacedInReq->getAs<PackType>()) {
12021194
if (auto unwrapped = packType->unwrapSingletonPackExpansion())
12031195
replacedInReq = unwrapped->getPatternType();
@@ -7295,7 +7287,8 @@ void TypeChecker::inferDefaultWitnesses(ProtocolDecl *proto) {
72957287
RequirementEnvironment reqEnv(proto, reqSig, proto, nullptr, nullptr);
72967288
auto match =
72977289
RequirementMatch(asd, MatchKind::ExactMatch, asdTy, reqEnv);
7298-
match.WitnessSubstitutions = reqEnv.getRequirementToWitnessThunkSubs();
7290+
match.WitnessSubstitutions = reqEnv.getRequirementToWitnessThunkSubs()
7291+
.mapReplacementTypesOutOfContext();
72997292
checker.recordWitness(asd, match);
73007293
}
73017294
}

‎test/SILGen/conditional_conformance.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ extension Conformance: P1 where A: P2 {
2020
// This is defined below but is emitted before any witness tables.
2121
// Just make sure it does not have a generic signature.
2222
//
23-
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s23conditional_conformance16SameTypeConcreteVyxGAA2P1AASiRszlAaEP6normalyyFTW : $@convention(witness_method: P1) (@in_guaranteed SameTypeConcrete<Int>) -> ()
23+
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s23conditional_conformance11ConformanceVyxGAA2P1A2A2P2RzlAaEP6normalyyFTW : $@convention(witness_method: P1) <τ_0_0 where τ_0_0 : P2> (@in_guaranteed Conformance<τ_0_0>) -> () {
2424

2525

2626
// CHECK-LABEL: sil_witness_table hidden <A where A : P2> Conformance<A>: P1 module conditional_conformance {
@@ -48,8 +48,8 @@ extension SameTypeConcrete: P1 where B == Int {
4848
}
4949

5050
// CHECK-LABEL: sil_witness_table hidden <B where B == Int> SameTypeConcrete<B>: P1 module conditional_conformance {
51-
// CHECK-NEXT: method #P1.normal: <Self where Self : P1> (Self) -> () -> () : @$s23conditional_conformance16SameTypeConcreteVyxGAA2P1AASiRszlAaEP6normalyyFTW // protocol witness for P1.normal() in conformance <A> SameTypeConcrete<A>
52-
// CHECK-NEXT: method #P1.generic: <Self where Self : P1><T where T : P3> (Self) -> (T) -> () : @$s23conditional_conformance16SameTypeConcreteVyxGAA2P1AASiRszlAaEP7genericyyqd__AA2P3Rd__lFTW // protocol witness for P1.generic<A>(_:) in conformance <A> SameTypeConcrete<A>
51+
// CHECK-NEXT: method #P1.normal: <Self where Self : P1> (Self) -> () -> () : @$s23conditional_conformance16SameTypeConcreteVySiGAA2P1A2aEP6normalyyFTW // protocol witness for P1.normal() in conformance SameTypeConcrete<Int>
52+
// CHECK-NEXT: method #P1.generic: <Self where Self : P1><T where T : P3> (Self) -> (T) -> () : @$s23conditional_conformance16SameTypeConcreteVySiGAA2P1A2aEP7genericyyqd__AA2P3Rd__lFTW // protocol witness for P1.generic<A>(_:) in conformance SameTypeConcrete<Int>
5353
// CHECK-NEXT: }
5454

5555
struct SameTypeGeneric<C, D> {}

‎test/SILGen/constrained_extensions.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ extension S: HasX where T == Int {
254254
}
255255
}
256256

257-
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s22constrained_extensions1SVyxGAA4HasXAASiRszlAaEP1xSivMTW : $@yield_once @convention(witness_method: HasX) @substituted <τ_0_0> (@inout τ_0_0) -> @yields @inout Int for <S<Int>> {
257+
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s22constrained_extensions1SVySiGAA4HasXA2aEP1xSivMTW : $@yield_once @convention(witness_method: HasX) @substituted <τ_0_0> (@inout τ_0_0) -> @yields @inout Int for <S<Int>> {
258258
// CHECK: [[WITNESS_FN:%.*]] = function_ref @$s22constrained_extensions1SVAASiRszlE1xSivM : $@yield_once @convention(method) (@inout S<Int>) -> @yields @inout Int
259259
// CHECK: begin_apply [[WITNESS_FN]](%0) : $@yield_once @convention(method) (@inout S<Int>) -> @yields @inout Int
260260
// CHECK: yield

‎test/SILGen/witness_same_type.swift

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,58 @@ public struct LazySequenceOf<SS : Sequence, A> : Sequence where SS.Iterator.Elem
3333
}
3434
}
3535
}
36+
37+
// rdar://problem/155465011
38+
39+
// Due to a series of unfortunate coincidences, we would assign the same mangling
40+
// to the witness thunks for P1.f() in the S1: P1 and S2: P1 conformance.
41+
42+
public protocol P1 {
43+
func f<T: P2>(_: T) where T.A == Self
44+
}
45+
46+
public protocol P2 {
47+
associatedtype A: P1
48+
}
49+
50+
struct N1: P2 {
51+
typealias A = S1
52+
}
53+
54+
struct N2: P2 {
55+
typealias A = S2
56+
}
57+
58+
struct S1: P1 {
59+
func f<T: P2>(_: T) where T.A == Self {
60+
print("Hello from S1")
61+
}
62+
}
63+
64+
struct S2: P1 {
65+
func f<T: P2>(_: T) where T.A == Self {
66+
print("Hello from S2")
67+
}
68+
}
69+
70+
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s17witness_same_type2S1VAA2P1A2aDP1fyyqd__1AQyd__RszAA2P2Rd__lFTW : $@convention(witness_method: P1) <τ_0_0 where τ_0_0 : P2, τ_0_0.A == S1> (@in_guaranteed τ_0_0, @in_guaranteed S1) -> () {
71+
72+
// CHECK-LABEL: sil private [transparent] [thunk] [ossa] @$s17witness_same_type2S2VAA2P1A2aDP1fyyqd__1AQyd__RszAA2P2Rd__lFTW : $@convention(witness_method: P1) <τ_0_0 where τ_0_0 : P2, τ_0_0.A == S2> (@in_guaranteed τ_0_0, @in_guaranteed S2) -> () {
73+
74+
// CHECK-LABEL: sil_witness_table hidden N1: P2 module witness_same_type {
75+
// CHECK-NEXT: associated_conformance (A: P1): S1: P1 module witness_same_type
76+
// CHECK-NEXT: associated_type A: S1
77+
// CHECK-NEXT: }
78+
79+
// CHECK-LABEL: sil_witness_table hidden N2: P2 module witness_same_type {
80+
// CHECK-NEXT: associated_conformance (A: P1): S2: P1 module witness_same_type
81+
// CHECK-NEXT: associated_type A: S2
82+
// CHECK-NEXT: }
83+
84+
// CHECK-LABEL: sil_witness_table hidden S1: P1 module witness_same_type {
85+
// CHECK-NEXT: method #P1.f: <Self><T where Self == T.A, T : P2> (Self) -> (T) -> () : @$s17witness_same_type2S1VAA2P1A2aDP1fyyqd__1AQyd__RszAA2P2Rd__lFTW
86+
// CHECK-NEXT: }
87+
88+
// CHECK-LABEL: sil_witness_table hidden S2: P1 module witness_same_type {
89+
// CHECK-NEXT: method #P1.f: <Self><T where Self == T.A, T : P2> (Self) -> (T) -> () : @$s17witness_same_type2S2VAA2P1A2aDP1fyyqd__1AQyd__RszAA2P2Rd__lFTW
90+
// CHECK-NEXT: }

0 commit comments

Comments
 (0)