Skip to content

Commit 652f8ee

Browse files
slavapestovslavapestov
committed
SIL: VTable thunks should use the 'override generic signature'
We recently added some checking to ensure that a method override's generic signature does not have any generic requirements not satisfied by the base method. Loosening requirements in the other direction was allowed, because it means the derived method can be called on potentially more types than the base method. However, if the generic signatures don't match, a thunk must be emitted. While we correctly determined whether a thunk should be emitted, the thunk had the wrong generic signature, and therefore the wrong calling convention, which would cause crashes at runtime. Fixes <rdar://problem/57429775>.
1 parent 5c2095e commit 652f8ee

File tree

3 files changed

+128
-37
lines changed

3 files changed

+128
-37
lines changed

‎lib/AST/ASTContext.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4430,11 +4430,13 @@ ASTContext::getOverrideGenericSignature(const ValueDecl *base,
44304430
const ValueDecl *derived) {
44314431
auto baseGenericCtx = base->getAsGenericContext();
44324432
auto derivedGenericCtx = derived->getAsGenericContext();
4433-
auto &ctx = base->getASTContext();
44344433

44354434
if (!baseGenericCtx || !derivedGenericCtx)
44364435
return nullptr;
44374436

4437+
if (base == derived)
4438+
return derivedGenericCtx->getGenericSignature();
4439+
44384440
auto baseClass = base->getDeclContext()->getSelfClassDecl();
44394441
if (!baseClass)
44404442
return nullptr;
@@ -4490,7 +4492,7 @@ ASTContext::getOverrideGenericSignature(const ValueDecl *base,
44904492
}
44914493

44924494
return CanGenericTypeParamType::get(
4493-
gp->getDepth() - baseDepth + derivedDepth, gp->getIndex(), ctx);
4495+
gp->getDepth() - baseDepth + derivedDepth, gp->getIndex(), *this);
44944496
};
44954497

44964498
auto lookupConformanceFn =
@@ -4510,7 +4512,7 @@ ASTContext::getOverrideGenericSignature(const ValueDecl *base,
45104512
}
45114513

45124514
auto genericSig = evaluateOrDefault(
4513-
ctx.evaluator,
4515+
evaluator,
45144516
AbstractGenericSignatureRequest{
45154517
derivedClass->getGenericSignature().getPointer(),
45164518
std::move(addedGenericParams),

‎lib/SIL/SILFunctionType.cpp

Lines changed: 47 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2514,64 +2514,77 @@ TypeConverter::getConstantOverrideInfo(TypeExpansionContext context,
25142514

25152515
assert(base.requiresNewVTableEntry() && "base must not be an override");
25162516

2517+
// Figure out the generic signature for the class method call. This is the
2518+
// signature of the derived class, with requirements transplanted from
2519+
// the base method. The derived method is allowed to have fewer
2520+
// requirements, in which case the thunk will translate the calling
2521+
// convention appropriately before calling the derived method.
2522+
bool hasGenericRequirementDifference = false;
2523+
2524+
auto derivedSig = derived.getDecl()->getAsGenericContext()
2525+
->getGenericSignature();
2526+
auto genericSig = Context.getOverrideGenericSignature(base.getDecl(),
2527+
derived.getDecl());
2528+
if (genericSig) {
2529+
hasGenericRequirementDifference =
2530+
!genericSig->requirementsNotSatisfiedBy(derivedSig).empty();
2531+
}
2532+
25172533
auto baseInfo = getConstantInfo(context, base);
25182534
auto derivedInfo = getConstantInfo(context, derived);
25192535

2520-
// If the derived method is ABI-compatible with the base method, give the
2521-
// vtable thunk the same signature as the derived method.
2522-
auto basePattern = AbstractionPattern(baseInfo.LoweredType);
2523-
2524-
auto baseInterfaceTy = baseInfo.FormalType;
2525-
auto derivedInterfaceTy = derivedInfo.FormalType;
2526-
2527-
auto params = derivedInterfaceTy.getParams();
2536+
auto params = derivedInfo.FormalType.getParams();
25282537
assert(params.size() == 1);
25292538
auto selfInterfaceTy = params[0].getPlainType()->getMetatypeInstanceType();
25302539

25312540
auto overrideInterfaceTy =
2541+
cast<AnyFunctionType>(
25322542
selfInterfaceTy->adjustSuperclassMemberDeclType(
2533-
base.getDecl(), derived.getDecl(), baseInterfaceTy);
2534-
2535-
// Copy generic signature from derived to the override type, to handle
2536-
// the case where the base member is not generic (because the base class
2537-
// is concrete) but the derived member is generic (because the derived
2538-
// class is generic).
2539-
if (auto derivedInterfaceFnTy = derivedInterfaceTy->getAs<GenericFunctionType>()) {
2540-
auto overrideInterfaceFnTy = overrideInterfaceTy->castTo<FunctionType>();
2541-
overrideInterfaceTy =
2542-
GenericFunctionType::get(derivedInterfaceFnTy->getGenericSignature(),
2543-
overrideInterfaceFnTy->getParams(),
2544-
overrideInterfaceFnTy->getResult(),
2545-
overrideInterfaceFnTy->getExtInfo());
2546-
}
2543+
base.getDecl(), derived.getDecl(), baseInfo.FormalType)
2544+
->getCanonicalType());
25472545

2548-
// Lower the formal AST type.
2549-
auto bridgedTypes = getLoweredFormalTypes(derived,
2550-
cast<AnyFunctionType>(overrideInterfaceTy->getCanonicalType()));
2551-
auto overrideLoweredInterfaceTy = bridgedTypes.Uncurried;
2546+
// Build the formal AST function type for the class method call.
2547+
auto basePattern = AbstractionPattern(baseInfo.LoweredType);
2548+
2549+
if (!hasGenericRequirementDifference &&
2550+
!checkASTTypeForABIDifferences(derivedInfo.FormalType,
2551+
overrideInterfaceTy)) {
25522552

2553-
if (!checkASTTypeForABIDifferences(derivedInfo.LoweredType,
2554-
overrideLoweredInterfaceTy)) {
2553+
// The derived method is ABI-compatible with the base method. Let's
2554+
// just use the derived method's formal type.
25552555
basePattern = AbstractionPattern(
25562556
copyOptionalityFromDerivedToBase(
25572557
*this,
25582558
derivedInfo.LoweredType,
25592559
baseInfo.LoweredType));
2560-
overrideLoweredInterfaceTy = derivedInfo.LoweredType;
2560+
overrideInterfaceTy = derivedInfo.FormalType;
2561+
}
2562+
2563+
if (genericSig && !genericSig->areAllParamsConcrete()) {
2564+
overrideInterfaceTy =
2565+
cast<AnyFunctionType>(
2566+
GenericFunctionType::get(genericSig,
2567+
overrideInterfaceTy->getParams(),
2568+
overrideInterfaceTy->getResult(),
2569+
overrideInterfaceTy->getExtInfo())
2570+
->getCanonicalType());
25612571
}
25622572

2563-
// Build the SILFunctionType for the vtable thunk.
2573+
// Build the lowered AST function type for the class method call.
2574+
auto bridgedTypes = getLoweredFormalTypes(derived, overrideInterfaceTy);
2575+
2576+
// Build the SILFunctionType for the class method call.
25642577
CanSILFunctionType fnTy = getNativeSILFunctionType(
2565-
*this, context, basePattern, overrideLoweredInterfaceTy, base, derived,
2578+
*this, context, basePattern, bridgedTypes.Uncurried, base, derived,
25662579
/*reqt subs*/ None, ProtocolConformanceRef());
25672580

25682581
// Build the SILConstantInfo and cache it.
25692582
auto resultBuf = Context.Allocate(sizeof(SILConstantInfo),
25702583
alignof(SILConstantInfo));
25712584
auto result = ::new (resultBuf) SILConstantInfo{
2572-
derivedInterfaceTy,
2573-
bridgedTypes.Pattern,
2574-
overrideLoweredInterfaceTy,
2585+
overrideInterfaceTy,
2586+
basePattern,
2587+
bridgedTypes.Uncurried,
25752588
fnTy};
25762589

25772590
auto inserted = ConstantOverrideTypes.insert({{derived, base}, result});

‎test/SILGen/vtable_generic_signature.swift

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// RUN: %target-swift-emit-silgen %s | %FileCheck %s
2+
3+
protocol P {}
4+
protocol Q : P {}
5+
6+
class ConcreteBase {
7+
func f<U : Q>(_: U) {}
8+
}
9+
10+
class ConcreteDerivedFromConcreteBase : ConcreteBase {
11+
override func f<U : P>(_: U) {}
12+
}
13+
14+
class GenericDerivedFromConcreteBase<T> : ConcreteBase {
15+
override func f<U : P>(_: U) {}
16+
}
17+
18+
class GenericBase<T> {
19+
func f<U : Q>(_: U) {}
20+
}
21+
22+
class ConcreteDerivedFromGenericBase : GenericBase<Int> {
23+
override func f<U : P>(_: U) {}
24+
}
25+
26+
class GenericDerivedFromGenericBase<T> : GenericBase<(T) -> Int> {
27+
override func f<U : P>(_: U) {}
28+
}
29+
30+
// All the vtable thunks should traffic in <U where U : Q>, because that's
31+
// what the base method declares.
32+
33+
// CHECK-LABEL: sil private [thunk] [ossa] @$s24vtable_generic_signature019ConcreteDerivedFromD4BaseC1fyyxAA1PRzlFAA0dG0CADyyxAA1QRzlFTV : $@convention(method) <U where U : Q> (@in_guaranteed U, @guaranteed ConcreteDerivedFromConcreteBase) -> ()
34+
// CHECK-LABEL: sil private [thunk] [ossa] @$s24vtable_generic_signature30GenericDerivedFromConcreteBaseC1fyyqd__AA1PRd__lFAA0gH0CADyyxAA1QRzlFTV : $@convention(method) <T><U where U : Q> (@in_guaranteed U, @guaranteed GenericDerivedFromConcreteBase<T>) -> ()
35+
// CHECK-LABEL: sil private [thunk] [ossa] @$s24vtable_generic_signature30ConcreteDerivedFromGenericBaseC1fyyxAA1PRzlFAA0gH0CADyyqd__AA1QRd__lFTV : $@convention(method) <U where U : Q> (@in_guaranteed U, @guaranteed ConcreteDerivedFromGenericBase) -> ()
36+
// CHECK-LABEL: sil private [thunk] [ossa] @$s24vtable_generic_signature018GenericDerivedFromD4BaseC1fyyqd__AA1PRd__lFAA0dG0CADyyqd__AA1QRd__lFTV : $@convention(method) <T><U where U : Q> (@in_guaranteed U, @guaranteed GenericDerivedFromGenericBase<T>) -> ()
37+
38+
// CHECK-LABEL: sil_vtable ConcreteBase {
39+
// CHECK-NEXT: #ConcreteBase.f!1: <U where U : Q> (ConcreteBase) -> (U) -> () : @$s24vtable_generic_signature12ConcreteBaseC1fyyxAA1QRzlF
40+
// CHECK-NEXT: #ConcreteBase.init!allocator.1: (ConcreteBase.Type) -> () -> ConcreteBase : @$s24vtable_generic_signature12ConcreteBaseCACycfC
41+
// CHECK-NEXT: #ConcreteBase.deinit!deallocator.1: @$s24vtable_generic_signature12ConcreteBaseCfD
42+
// CHECK-NEXT: }
43+
44+
// CHECK-LABEL: sil_vtable ConcreteDerivedFromConcreteBase {
45+
// CHECK-NEXT: #ConcreteBase.f!1: <U where U : Q> (ConcreteBase) -> (U) -> () : @$s24vtable_generic_signature019ConcreteDerivedFromD4BaseC1fyyxAA1PRzlFAA0dG0CADyyxAA1QRzlFTV [override]
46+
// CHECK-NEXT: #ConcreteBase.init!allocator.1: (ConcreteBase.Type) -> () -> ConcreteBase : @$s24vtable_generic_signature019ConcreteDerivedFromD4BaseCACycfC [override]
47+
// CHECK-NEXT: #ConcreteDerivedFromConcreteBase.f!1: <U where U : P> (ConcreteDerivedFromConcreteBase) -> (U) -> () : @$s24vtable_generic_signature019ConcreteDerivedFromD4BaseC1fyyxAA1PRzlF
48+
// CHECK-NEXT: #ConcreteDerivedFromConcreteBase.deinit!deallocator.1: @$s24vtable_generic_signature019ConcreteDerivedFromD4BaseCfD
49+
// CHECK-NEXT: }
50+
51+
// CHECK-LABEL: sil_vtable GenericDerivedFromConcreteBase {
52+
// CHECK-NEXT: #ConcreteBase.f!1: <U where U : Q> (ConcreteBase) -> (U) -> () : @$s24vtable_generic_signature30GenericDerivedFromConcreteBaseC1fyyqd__AA1PRd__lFAA0gH0CADyyxAA1QRzlFTV [override]
53+
// CHECK-NEXT: #ConcreteBase.init!allocator.1: (ConcreteBase.Type) -> () -> ConcreteBase : @$s24vtable_generic_signature30GenericDerivedFromConcreteBaseCACyxGycfC [override]
54+
// CHECK-NEXT: #GenericDerivedFromConcreteBase.f!1: <T><U where U : P> (GenericDerivedFromConcreteBase<T>) -> (U) -> () : @$s24vtable_generic_signature30GenericDerivedFromConcreteBaseC1fyyqd__AA1PRd__lF
55+
// CHECK-NEXT: #GenericDerivedFromConcreteBase.deinit!deallocator.1: @$s24vtable_generic_signature30GenericDerivedFromConcreteBaseCfD
56+
// CHECK-NEXT: }
57+
58+
// CHECK-LABEL: sil_vtable GenericBase {
59+
// CHECK-NEXT: #GenericBase.f!1: <T><U where U : Q> (GenericBase<T>) -> (U) -> () : @$s24vtable_generic_signature11GenericBaseC1fyyqd__AA1QRd__lF
60+
// CHECK-NEXT: #GenericBase.init!allocator.1: <T> (GenericBase<T>.Type) -> () -> GenericBase<T> : @$s24vtable_generic_signature11GenericBaseCACyxGycfC
61+
// CHECK-NEXT: #GenericBase.deinit!deallocator.1: @$s24vtable_generic_signature11GenericBaseCfD
62+
// CHECK-NEXT: }
63+
64+
// CHECK-LABEL: sil_vtable ConcreteDerivedFromGenericBase {
65+
// CHECK-NEXT: #GenericBase.f!1: <T><U where U : Q> (GenericBase<T>) -> (U) -> () : @$s24vtable_generic_signature30ConcreteDerivedFromGenericBaseC1fyyxAA1PRzlFAA0gH0CADyyqd__AA1QRd__lFTV [override]
66+
// CHECK-NEXT: #GenericBase.init!allocator.1: <T> (GenericBase<T>.Type) -> () -> GenericBase<T> : @$s24vtable_generic_signature30ConcreteDerivedFromGenericBaseCACycfC [override]
67+
// CHECK-NEXT: #ConcreteDerivedFromGenericBase.f!1: <U where U : P> (ConcreteDerivedFromGenericBase) -> (U) -> () : @$s24vtable_generic_signature30ConcreteDerivedFromGenericBaseC1fyyxAA1PRzlF
68+
// CHECK-NEXT: #ConcreteDerivedFromGenericBase.deinit!deallocator.1: @$s24vtable_generic_signature30ConcreteDerivedFromGenericBaseCfD
69+
// CHECK-NEXT: }
70+
71+
// CHECK-LABEL: sil_vtable GenericDerivedFromGenericBase {
72+
// CHECK-NEXT: #GenericBase.f!1: <T><U where U : Q> (GenericBase<T>) -> (U) -> () : @$s24vtable_generic_signature018GenericDerivedFromD4BaseC1fyyqd__AA1PRd__lFAA0dG0CADyyqd__AA1QRd__lFTV [override]
73+
// CHECK-NEXT: #GenericBase.init!allocator.1: <T> (GenericBase<T>.Type) -> () -> GenericBase<T> : @$s24vtable_generic_signature018GenericDerivedFromD4BaseCACyxGycfC [override]
74+
// CHECK-NEXT: #GenericDerivedFromGenericBase.f!1: <T><U where U : P> (GenericDerivedFromGenericBase<T>) -> (U) -> () : @$s24vtable_generic_signature018GenericDerivedFromD4BaseC1fyyqd__AA1PRd__lF
75+
// CHECK-NEXT: #GenericDerivedFromGenericBase.deinit!deallocator.1: @$s24vtable_generic_signature018GenericDerivedFromD4BaseCfD
76+
// CHECK-NEXT: }

0 commit comments

Comments
 (0)