Skip to content

Commit fe280cf

Browse files
AnnaZaksAnnaZaks
authored
Merge pull request #22447 from rjmccall/init-class-metadata-dependencies-compiler-5.0
[5.0] Fix a race condition with the initialization of class metadata
2 parents 8f77f3a + 6e04239 commit fe280cf

12 files changed

+131
-50
lines changed

‎include/swift/Runtime/RuntimeFunctions.def

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -780,7 +780,6 @@ FUNCTION(RelocateClassMetadata,
780780
ARGS(TypeContextDescriptorPtrTy, Int8PtrTy),
781781
ATTRS(NoUnwind))
782782

783-
// struct FieldInfo { size_t Size; size_t AlignMask; };
784783
// void swift_initClassMetadata(Metadata *self,
785784
// ClassLayoutFlags flags,
786785
// size_t numFields,
@@ -794,7 +793,6 @@ FUNCTION(InitClassMetadata,
794793
SizeTy->getPointerTo()),
795794
ATTRS(NoUnwind))
796795

797-
// struct FieldInfo { size_t Size; size_t AlignMask; };
798796
// void swift_updateClassMetadata(Metadata *self,
799797
// ClassLayoutFlags flags,
800798
// size_t numFields,
@@ -808,7 +806,6 @@ FUNCTION(UpdateClassMetadata,
808806
SizeTy->getPointerTo()),
809807
ATTRS(NoUnwind))
810808

811-
// struct FieldInfo { size_t Size; size_t AlignMask; };
812809
// MetadataDependency swift_initClassMetadata2(Metadata *self,
813810
// ClassLayoutFlags flags,
814811
// size_t numFields,
@@ -822,7 +819,6 @@ FUNCTION(InitClassMetadata2,
822819
SizeTy->getPointerTo()),
823820
ATTRS(NoUnwind))
824821

825-
// struct FieldInfo { size_t Size; size_t AlignMask; };
826822
// MetadataDependency swift_updateClassMetadata2(Metadata *self,
827823
// ClassLayoutFlags flags,
828824
// size_t numFields,

‎lib/IRGen/GenMeta.cpp

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1815,24 +1815,33 @@ static void emitInitializeFieldOffsetVector(IRGenFunction &IGF,
18151815
if (!doesClassMetadataRequireRelocation(IGM, classDecl))
18161816
flags |= ClassLayoutFlags::HasStaticVTable;
18171817

1818+
llvm::Value *dependency;
18181819
if (doesClassMetadataRequireInitialization(IGM, classDecl)) {
18191820
// Call swift_initClassMetadata().
1820-
IGF.Builder.CreateCall(IGM.getInitClassMetadataFn(),
1821-
{metadata,
1822-
IGM.getSize(Size(uintptr_t(flags))),
1823-
numFields, fields.getAddress(), fieldVector});
1821+
dependency =
1822+
IGF.Builder.CreateCall(IGM.getInitClassMetadata2Fn(),
1823+
{metadata,
1824+
IGM.getSize(Size(uintptr_t(flags))),
1825+
numFields, fields.getAddress(), fieldVector});
18241826
} else {
18251827
assert(doesClassMetadataRequireUpdate(IGM, classDecl));
18261828
assert(IGM.Context.LangOpts.EnableObjCInterop);
18271829

18281830
// Call swift_updateClassMetadata(). Note that the static metadata
18291831
// already references the superclass in this case, but we still want
18301832
// to ensure the superclass metadata is initialized first.
1831-
IGF.Builder.CreateCall(IGM.getUpdateClassMetadataFn(),
1832-
{metadata,
1833-
IGM.getSize(Size(uintptr_t(flags))),
1834-
numFields, fields.getAddress(), fieldVector});
1833+
dependency =
1834+
IGF.Builder.CreateCall(IGM.getUpdateClassMetadata2Fn(),
1835+
{metadata,
1836+
IGM.getSize(Size(uintptr_t(flags))),
1837+
numFields, fields.getAddress(), fieldVector});
18351838
}
1839+
1840+
// Collect any possible dependency from initializing the class; generally
1841+
// this involves the superclass.
1842+
assert(collector);
1843+
collector->collect(IGF, dependency);
1844+
18361845
} else {
18371846
assert(isa<StructDecl>(target));
18381847

‎lib/IRGen/MetadataRequest.cpp

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -162,29 +162,57 @@ void MetadataDependencyCollector::checkDependency(IRGenFunction &IGF,
162162

163163
// Otherwise, we need to pull out the response state and compare it against
164164
// the request state.
165+
llvm::Value *requiredState = request.getRequiredState(IGF);
166+
167+
// More advanced metadata states are lower numbers.
168+
static_assert(MetadataStateIsReverseOrdered,
169+
"relying on the ordering of MetadataState here");
170+
auto satisfied = IGF.Builder.CreateICmpULE(metadataState, requiredState);
171+
172+
emitCheckBranch(IGF, satisfied, metadata, requiredState);
173+
}
174+
175+
void MetadataDependencyCollector::collect(IRGenFunction &IGF,
176+
llvm::Value *dependency) {
177+
// Having either both or neither of the PHIs is normal.
178+
// Having just RequiredState means that we already finalized this collector
179+
// and shouldn't be using it anymore.
180+
assert((!RequiredMetadata || RequiredState) &&
181+
"checking dependencies on a finished collector");
182+
183+
assert(dependency->getType() == IGF.IGM.TypeMetadataDependencyTy);
165184

166-
// Lazily create the continuation block and phis.
185+
// Split the dependency.
186+
auto metadata = IGF.Builder.CreateExtractValue(dependency, 0);
187+
auto requiredState = IGF.Builder.CreateExtractValue(dependency, 1);
188+
189+
// We have a dependency if the metadata is non-null; otherwise we're
190+
// satisfied and can continue.
191+
auto satisfied = IGF.Builder.CreateIsNull(metadata);
192+
emitCheckBranch(IGF, satisfied, metadata, requiredState);
193+
}
194+
195+
void MetadataDependencyCollector::emitCheckBranch(IRGenFunction &IGF,
196+
llvm::Value *satisfied,
197+
llvm::Value *metadata,
198+
llvm::Value *requiredState) {
199+
// Lazily create the final continuation block and phis.
167200
if (!RequiredMetadata) {
168201
auto contBB = IGF.createBasicBlock("metadata-dependencies.cont");
169202
RequiredMetadata =
170203
llvm::PHINode::Create(IGF.IGM.TypeMetadataPtrTy, 4, "", contBB);
171204
RequiredState = llvm::PHINode::Create(IGF.IGM.SizeTy, 4, "", contBB);
172205
}
173206

174-
llvm::Value *requiredState = request.getRequiredState(IGF);
175-
176-
// More advanced metadata states are lower numbers.
177-
static_assert(MetadataStateIsReverseOrdered,
178-
"relying on the ordering of MetadataState here");
179-
auto satisfied = IGF.Builder.CreateICmpULE(metadataState, requiredState);
180-
207+
// Conditionally branch to the final continuation block.
181208
auto satisfiedBB = IGF.createBasicBlock("dependency-satisfied");
182209
auto curBB = IGF.Builder.GetInsertBlock();
183210
RequiredMetadata->addIncoming(metadata, curBB);
184211
RequiredState->addIncoming(requiredState, curBB);
185212
IGF.Builder.CreateCondBr(satisfied, satisfiedBB,
186213
RequiredMetadata->getParent());
187214

215+
// Otherwise resume emitting code on the main path.
188216
IGF.Builder.emitBlock(satisfiedBB);
189217
}
190218

‎lib/IRGen/MetadataRequest.h

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -430,14 +430,27 @@ class MetadataDependencyCollector {
430430
"failed to finish MetadataDependencyCollector");
431431
}
432432

433-
/// Check the dependency. This takes a metadata and state separately
434-
/// instead of taking a MetadataResponse because it's quite important
435-
/// that we not rely on anything from MetadataResponse that might assume
436-
/// that we've already done dependency collection.
433+
/// Given the result of fetching metadata, check whether it creates a
434+
/// metadata dependency, and branch if so.
435+
///
436+
/// This takes a metadata and state separately instead of taking a
437+
/// MetadataResponse pair because it's quite important that we not rely on
438+
/// anything from MetadataResponse that might assume that we've already
439+
/// done dependency collection.
437440
void checkDependency(IRGenFunction &IGF, DynamicMetadataRequest request,
438441
llvm::Value *metadata, llvm::Value *state);
439442

443+
/// Given an optional MetadataDependency value (e.g. the result of calling
444+
/// a dependency-returning function, in which a dependency is signalled
445+
/// by a non-null metadata value), check whether it indicates a dependency
446+
/// and branch if so.
447+
void collect(IRGenFunction &IGF, llvm::Value *dependencyPair);
448+
440449
MetadataDependency finish(IRGenFunction &IGF);
450+
451+
private:
452+
void emitCheckBranch(IRGenFunction &IGF, llvm::Value *satisfied,
453+
llvm::Value *metadata, llvm::Value *requiredState);
441454
};
442455

443456
enum class MetadataAccessStrategy {

‎stdlib/public/runtime/ProtocolConformance.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,7 @@ bool swift::_checkGenericRequirements(
689689
case GenericRequirementKind::BaseClass: {
690690
// Demangle the base type under the given substitutions.
691691
auto baseType =
692-
swift_getTypeByMangledName(MetadataState::Complete,
692+
swift_getTypeByMangledName(MetadataState::Abstract,
693693
req.getMangledTypeName(), substGenericParam,
694694
substWitnessTable).getMetadata();
695695
if (!baseType) return true;

‎test/IRGen/class_resilience.swift

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,13 @@ extension ResilientGenericOutsideParent {
400400
// CHECK: dependency-satisfied:
401401

402402
// -- ClassLayoutFlags = 0x100 (HasStaticVTable)
403-
// CHECK: void @swift_initClassMetadata(%swift.type* %0, [[INT]] 256, [[INT]] 3, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
403+
// CHECK: [[T0:%.*]] = call swiftcc %swift.metadata_response @swift_initClassMetadata2(%swift.type* %0, [[INT]] 256, [[INT]] 3, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
404+
// CHECK-NEXT: [[INITDEP_METADATA:%.*]] = extractvalue %swift.metadata_response [[T0]], 0
405+
// CHECK-NEXT: [[INITDEP_STATUS:%.*]] = extractvalue %swift.metadata_response [[T0]], 1
406+
// CHECK-NEXT: [[INITDEP_PRESENT:%.*]] = icmp eq %swift.type* [[INITDEP_METADATA]], null
407+
// CHECK-NEXT: br i1 [[INITDEP_PRESENT]], label %dependency-satisfied1, label %metadata-dependencies.cont
408+
409+
// CHECK: dependency-satisfied1:
404410

405411
// CHECK-native: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* {{.*}}
406412
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @"$s16class_resilience26ClassWithResilientPropertyC1s16resilient_struct4SizeVvpWvd"
@@ -412,8 +418,8 @@ extension ResilientGenericOutsideParent {
412418

413419
// CHECK: metadata-dependencies.cont:
414420

415-
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ null, %dependency-satisfied ]
416-
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ 0, %dependency-satisfied ]
421+
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ [[INITDEP_METADATA]], %dependency-satisfied ], [ null, %dependency-satisfied1 ]
422+
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ [[INITDEP_STATUS]], %dependency-satisfied ], [ 0, %dependency-satisfied1 ]
417423
// CHECK-NEXT: [[T0:%.*]] = insertvalue %swift.metadata_response undef, %swift.type* [[PENDING_METADATA]], 0
418424
// CHECK-NEXT: [[T1:%.*]] = insertvalue %swift.metadata_response [[T0]], [[INT]] [[NEW_STATUS]], 1
419425
// CHECK-NEXT: ret %swift.metadata_response [[T1]]
@@ -448,7 +454,13 @@ extension ResilientGenericOutsideParent {
448454
// CHECK: dependency-satisfied:
449455

450456
// -- ClassLayoutFlags = 0x100 (HasStaticVTable)
451-
// CHECK: call void @swift_initClassMetadata(%swift.type* %0, [[INT]] 256, [[INT]] 2, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
457+
// CHECK: [[T0:%.*]] = call swiftcc %swift.metadata_response @swift_initClassMetadata2(%swift.type* %0, [[INT]] 256, [[INT]] 2, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
458+
// CHECK-NEXT: [[INITDEP_METADATA:%.*]] = extractvalue %swift.metadata_response [[T0]], 0
459+
// CHECK-NEXT: [[INITDEP_STATUS:%.*]] = extractvalue %swift.metadata_response [[T0]], 1
460+
// CHECK-NEXT: [[INITDEP_PRESENT:%.*]] = icmp eq %swift.type* [[INITDEP_METADATA]], null
461+
// CHECK-NEXT: br i1 [[INITDEP_PRESENT]], label %dependency-satisfied1, label %metadata-dependencies.cont
462+
463+
// CHECK: dependency-satisfied1:
452464

453465
// CHECK-native: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* {{.*}}
454466
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @"$s16class_resilience33ClassWithResilientlySizedPropertyC1r16resilient_struct9RectangleVvpWvd"
@@ -460,8 +472,8 @@ extension ResilientGenericOutsideParent {
460472

461473
// CHECK: metadata-dependencies.cont:
462474

463-
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ null, %dependency-satisfied ]
464-
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ 0, %dependency-satisfied ]
475+
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ [[INITDEP_METADATA]], %dependency-satisfied ], [ null, %dependency-satisfied1 ]
476+
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ [[INITDEP_STATUS]], %dependency-satisfied ], [ 0, %dependency-satisfied1 ]
465477
// CHECK-NEXT: [[T0:%.*]] = insertvalue %swift.metadata_response undef, %swift.type* [[PENDING_METADATA]], 0
466478
// CHECK-NEXT: [[T1:%.*]] = insertvalue %swift.metadata_response [[T0]], [[INT]] [[NEW_STATUS]], 1
467479
// CHECK-NEXT: ret %swift.metadata_response [[T1]]
@@ -481,7 +493,7 @@ extension ResilientGenericOutsideParent {
481493
// CHECK-LABEL: define internal swiftcc %swift.metadata_response @"$s16class_resilience14ResilientChildCMr"(%swift.type*, i8*, i8**)
482494

483495
// Initialize field offset vector...
484-
// CHECK: call void @swift_initClassMetadata(%swift.type* %0, [[INT]] 0, [[INT]] 1, i8*** {{.*}}, [[INT]]* {{.*}})
496+
// CHECK: call swiftcc %swift.metadata_response @swift_initClassMetadata2(%swift.type* %0, [[INT]] 0, [[INT]] 1, i8*** {{.*}}, [[INT]]* {{.*}})
485497

486498
// CHECK: ret %swift.metadata_response
487499

@@ -519,7 +531,7 @@ extension ResilientGenericOutsideParent {
519531
// CHECK-LABEL: define internal swiftcc %swift.metadata_response @"$s16class_resilience21ResilientGenericChildCMr"
520532
// CHECK-SAME: (%swift.type* [[METADATA:%.*]], i8*, i8**)
521533

522-
// CHECK: call void @swift_initClassMetadata(%swift.type* [[METADATA]], [[INT]] 0,
534+
// CHECK: call swiftcc %swift.metadata_response @swift_initClassMetadata2(%swift.type* [[METADATA]], [[INT]] 0,
523535
// CHECK: ret %swift.metadata_response
524536

525537

‎test/IRGen/completely_fragile_class_layout.sil

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ bb0(%0 : @guaranteed $ClassWithResilientField):
209209
// CHECK-NEXT: ret %objc_class* [[RESULT]]
210210

211211

212-
// Metadata initialization function for ClassWithResilientField calls swift_updateClassMetadata():
212+
// Metadata initialization function for ClassWithResilientField calls swift_updateClassMetadata2():
213213

214214
// CHECK-LABEL: define internal swiftcc %swift.metadata_response @"$s31completely_fragile_class_layout23ClassWithResilientFieldCMr"(%swift.type*, i8*, i8**)
215215

@@ -230,7 +230,13 @@ bb0(%0 : @guaranteed $ClassWithResilientField):
230230
// CHECK: dependency-satisfied:
231231

232232
// -- ClassLayoutFlags = 0x100 (HasStaticVTable)
233-
// CHECK: void @swift_updateClassMetadata(%swift.type* %0, [[INT]] 256, [[INT]] 3, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
233+
// CHECK: [[T0:%.*]] = call swiftcc %swift.metadata_response @swift_updateClassMetadata2(%swift.type* %0, [[INT]] 256, [[INT]] 3, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
234+
// CHECK-NEXT: [[INITDEP_METADATA:%.*]] = extractvalue %swift.metadata_response [[T0]], 0
235+
// CHECK-NEXT: [[INITDEP_STATUS:%.*]] = extractvalue %swift.metadata_response [[T0]], 1
236+
// CHECK-NEXT: [[INITDEP_PRESENT:%.*]] = icmp eq %swift.type* [[INITDEP_METADATA]], null
237+
// CHECK-NEXT: br i1 [[INITDEP_PRESENT]], label %dependency-satisfied1, label %metadata-dependencies.cont
238+
239+
// CHECK: dependency-satisfied1:
234240

235241
// CHECK-native: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* {{.*}}
236242
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @"$s31completely_fragile_class_layout23ClassWithResilientFieldC1s16resilient_struct4SizeVvpWvd"
@@ -242,8 +248,8 @@ bb0(%0 : @guaranteed $ClassWithResilientField):
242248

243249
// CHECK: metadata-dependencies.cont:
244250

245-
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ null, %dependency-satisfied ]
246-
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ 0, %dependency-satisfied ]
251+
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[SIZE_METADATA]], %entry ], [ [[INITDEP_METADATA]], %dependency-satisfied ], [ null, %dependency-satisfied1 ]
252+
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ 63, %entry ], [ [[INITDEP_STATUS]], %dependency-satisfied ], [ 0, %dependency-satisfied1 ]
247253
// CHECK-NEXT: [[T0:%.*]] = insertvalue %swift.metadata_response undef, %swift.type* [[PENDING_METADATA]], 0
248254
// CHECK-NEXT: [[T1:%.*]] = insertvalue %swift.metadata_response [[T0]], [[INT]] [[NEW_STATUS]], 1
249255
// CHECK-NEXT: ret %swift.metadata_response [[T1]]
@@ -262,9 +268,21 @@ bb0(%0 : @guaranteed $ClassWithResilientField):
262268
// CHECK-NEXT: [[FIELDS_PTR:%.*]] = getelementptr inbounds [0 x i8**], [0 x i8**]* [[FIELDS]], i32 0, i32 0
263269

264270
// -- ClassLayoutFlags = 0x100 (HasStaticVTable)
265-
// CHECK: void @swift_updateClassMetadata(%swift.type* %0, [[INT]] 256, [[INT]] 0, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
271+
// CHECK: [[T0:%.*]] = call swiftcc %swift.metadata_response @swift_updateClassMetadata2(%swift.type* %0, [[INT]] 256, [[INT]] 0, i8*** [[FIELDS_PTR]], [[INT]]* [[FIELDS_DEST]])
272+
// CHECK-NEXT: [[INITDEP_METADATA:%.*]] = extractvalue %swift.metadata_response [[T0]], 0
273+
// CHECK-NEXT: [[INITDEP_STATUS:%.*]] = extractvalue %swift.metadata_response [[T0]], 1
274+
// CHECK-NEXT: [[INITDEP_PRESENT:%.*]] = icmp eq %swift.type* [[INITDEP_METADATA]], null
275+
// CHECK-NEXT: br i1 [[INITDEP_PRESENT]], label %dependency-satisfied, label %metadata-dependencies.cont
276+
277+
// CHECK: dependency-satisfied:
278+
// CHECK: br label %metadata-dependencies.cont
266279

267-
// CHECK: ret %swift.metadata_response zeroinitializer
280+
// CHECK: metadata-dependencies.cont:
281+
// CHECK-NEXT: [[PENDING_METADATA:%.*]] = phi %swift.type* [ [[INITDEP_METADATA]], %entry ], [ null, %dependency-satisfied ]
282+
// CHECK-NEXT: [[NEW_STATUS:%.*]] = phi [[INT]] [ [[INITDEP_STATUS]], %entry ], [ 0, %dependency-satisfied ]
283+
// CHECK-NEXT: [[T0:%.*]] = insertvalue %swift.metadata_response undef, %swift.type* [[PENDING_METADATA]], 0
284+
// CHECK-NEXT: [[T1:%.*]] = insertvalue %swift.metadata_response [[T0]], [[INT]] [[NEW_STATUS]], 1
285+
// CHECK-NEXT: ret %swift.metadata_response [[T1]]
268286

269287
// Metadata accessor for ClassWithResilientEnum looks like singleton initialization:
270288

‎test/IRGen/concrete_inherits_generic_base.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,4 +83,4 @@ presentBase(Base(x: 2))
8383

8484
// CHECK-LABEL: define internal swiftcc %swift.metadata_response @"$s3foo12SuperDerivedCMr"(%swift.type*, i8*, i8**)
8585
// -- ClassLayoutFlags = 0x100 (HasStaticVTable)
86-
// CHECK: call void @swift_initClassMetadata(%swift.type* %0, [[INT]] 256, {{.*}})
86+
// CHECK: call swiftcc %swift.metadata_response @swift_initClassMetadata2(%swift.type* %0, [[INT]] 256, {{.*}})

0 commit comments

Comments
 (0)