Skip to content

Commit 5c8bcfd

Browse files
beccadaxbeccadax
committed
[NFC] Correct AFD::needsNewVTableEntry()
Serialization depended on a longstanding bug in NeedsNewVTableEntryRequest: For a member of a non-class, it always returned `true`, not `false`. It turns out this was because serialization was conflating the concepts of vtable entries and witness table entries, so it needed NeedsNewVTableEntryRequest to return true for members of protocols. Untangle this logic so that NeedsNewVTableEntryRequest can be given the logical behavior.
1 parent d8bfa5a commit 5c8bcfd

File tree

5 files changed

+51
-51
lines changed

5 files changed

+51
-51
lines changed

‎lib/Sema/TypeCheckDecl.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1051,10 +1051,8 @@ NeedsNewVTableEntryRequest::evaluate(Evaluator &evaluator,
10511051
AbstractFunctionDecl *decl) const {
10521052
auto *dc = decl->getDeclContext();
10531053

1054-
// FIXME: This is mysterious and seems wrong. However, changing it to return
1055-
// false (as it seems like it should) breaks a couple Serialization tests.
10561054
if (!isa<ClassDecl>(dc))
1057-
return true;
1055+
return false;
10581056

10591057
// Destructors always use a fixed vtable entry.
10601058
if (isa<DestructorDecl>(decl))

‎lib/Serialization/Deserialization.cpp‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3426,7 +3426,7 @@ class DeclDeserializer {
34263426
GenericSignatureID genericSigID;
34273427
uint8_t storedInitKind, rawAccessLevel;
34283428
DeclID overriddenID;
3429-
bool overriddenAffectsABI, needsNewVTableEntry, firstTimeRequired;
3429+
bool overriddenAffectsABI, needsNewTableEntry, firstTimeRequired;
34303430
unsigned numArgNames;
34313431
ArrayRef<uint64_t> argNameAndDependencyIDs;
34323432

@@ -3439,7 +3439,7 @@ class DeclDeserializer {
34393439
overriddenID,
34403440
overriddenAffectsABI,
34413441
rawAccessLevel,
3442-
needsNewVTableEntry,
3442+
needsNewTableEntry,
34433443
firstTimeRequired,
34443444
numArgNames,
34453445
argNameAndDependencyIDs);
@@ -3455,11 +3455,11 @@ class DeclDeserializer {
34553455
getActualCtorInitializerKind(storedInitKind);
34563456

34573457
DeclDeserializationError::Flags errorFlags;
3458-
unsigned numVTableEntries = 0;
3458+
unsigned numTableEntries = 0;
34593459
if (initKind == CtorInitializerKind::Designated)
34603460
errorFlags |= DeclDeserializationError::DesignatedInitializer;
3461-
if (needsNewVTableEntry) {
3462-
numVTableEntries = 1;
3461+
if (needsNewTableEntry) {
3462+
numTableEntries = 1;
34633463
DeclAttributes attrs;
34643464
attrs.setRawAttributeChain(DAttrs);
34653465
}
@@ -3479,7 +3479,7 @@ class DeclDeserializer {
34793479
MF.diagnoseAndConsumeError(overriddenOrError.takeError());
34803480
if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) {
34813481
return llvm::make_error<OverrideError>(name, errorFlags,
3482-
numVTableEntries);
3482+
numTableEntries);
34833483
}
34843484

34853485
overridden = nullptr;
@@ -3490,7 +3490,7 @@ class DeclDeserializer {
34903490
if (!dependency) {
34913491
return llvm::make_error<TypeError>(
34923492
name, takeErrorInfo(dependency.takeError()),
3493-
errorFlags, numVTableEntries);
3493+
errorFlags, numTableEntries);
34943494
}
34953495
}
34963496

@@ -3545,8 +3545,6 @@ class DeclDeserializer {
35453545
if (initKind.has_value())
35463546
ctx.evaluator.cacheOutput(InitKindRequest{ctor},
35473547
std::move(initKind.value()));
3548-
ctx.evaluator.cacheOutput(NeedsNewVTableEntryRequest{ctor},
3549-
std::move(needsNewVTableEntry));
35503548

35513549
ctor->setOverriddenDecl(cast_or_null<ConstructorDecl>(overridden));
35523550
if (auto *overridden = ctor->getOverriddenDecl()) {
@@ -3882,7 +3880,7 @@ class DeclDeserializer {
38823880
DeclID associatedDeclID;
38833881
DeclID overriddenID;
38843882
DeclID accessorStorageDeclID;
3885-
bool overriddenAffectsABI, needsNewVTableEntry, isTransparent;
3883+
bool overriddenAffectsABI, needsNewTableEntry, isTransparent;
38863884
DeclID opaqueReturnTypeID;
38873885
bool isUserAccessible;
38883886
bool isDistributedThunk;
@@ -3901,7 +3899,7 @@ class DeclDeserializer {
39013899
overriddenAffectsABI,
39023900
numNameComponentsBiased,
39033901
rawAccessLevel,
3904-
needsNewVTableEntry,
3902+
needsNewTableEntry,
39053903
opaqueReturnTypeID,
39063904
isUserAccessible,
39073905
isDistributedThunk,
@@ -3920,14 +3918,14 @@ class DeclDeserializer {
39203918
accessorStorageDeclID,
39213919
rawAccessorKind,
39223920
rawAccessLevel,
3923-
needsNewVTableEntry,
3921+
needsNewTableEntry,
39243922
isTransparent,
39253923
isDistributedThunk,
39263924
nameAndDependencyIDs);
39273925
}
39283926

39293927
DeclDeserializationError::Flags errorFlags;
3930-
unsigned numVTableEntries = needsNewVTableEntry ? 1 : 0;
3928+
unsigned numTableEntries = needsNewTableEntry ? 1 : 0;
39313929

39323930
// Parse the accessor-specific fields.
39333931
AbstractStorageDecl *storage = nullptr;
@@ -3940,7 +3938,7 @@ class DeclDeserializer {
39403938
// FIXME: "TypeError" isn't exactly correct for this.
39413939
return llvm::make_error<TypeError>(
39423940
DeclName(), takeErrorInfo(storageResult.takeError()),
3943-
errorFlags, numVTableEntries);
3941+
errorFlags, numTableEntries);
39443942
}
39453943

39463944
if (auto accessorKindResult = getActualAccessorKind(rawAccessorKind))
@@ -3986,7 +3984,7 @@ class DeclDeserializer {
39863984
if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) {
39873985
MF.diagnoseAndConsumeError(overriddenOrError.takeError());
39883986
return llvm::make_error<OverrideError>(name, errorFlags,
3989-
numVTableEntries);
3987+
numTableEntries);
39903988
}
39913989
// Pass through deserialization errors.
39923990
if (overriddenOrError.errorIsA<FatalDeserializationError>())
@@ -4001,7 +3999,7 @@ class DeclDeserializer {
40013999
if (!dependency) {
40024000
return llvm::make_error<TypeError>(
40034001
name, takeErrorInfo(dependency.takeError()),
4004-
errorFlags, numVTableEntries);
4002+
errorFlags, numTableEntries);
40054003
}
40064004
}
40074005

@@ -4098,8 +4096,6 @@ class DeclDeserializer {
40984096
fn->setImplicit();
40994097
fn->setIsObjC(isObjC);
41004098
fn->setForcedStaticDispatch(hasForcedStaticDispatch);
4101-
ctx.evaluator.cacheOutput(NeedsNewVTableEntryRequest{fn},
4102-
std::move(needsNewVTableEntry));
41034099

41044100
if (opaqueReturnTypeID) {
41054101
auto declOrError = MF.getDeclChecked(opaqueReturnTypeID);
@@ -4799,7 +4795,7 @@ class DeclDeserializer {
47994795
uint8_t rawAccessLevel, rawSetterAccessLevel, rawStaticSpelling;
48004796
uint8_t opaqueReadOwnership, readImpl, writeImpl, readWriteImpl;
48014797
unsigned numArgNames, numAccessors;
4802-
unsigned numVTableEntries;
4798+
unsigned numTableEntries;
48034799
ArrayRef<uint64_t> argNameAndDependencyIDs;
48044800

48054801
decls_block::SubscriptLayout::readRecord(scratch, contextID,
@@ -4815,7 +4811,7 @@ class DeclDeserializer {
48154811
rawSetterAccessLevel,
48164812
rawStaticSpelling, numArgNames,
48174813
opaqueReturnTypeID,
4818-
numVTableEntries,
4814+
numTableEntries,
48194815
argNameAndDependencyIDs);
48204816
// Resolve the name ids.
48214817
SmallVector<Identifier, 2> argNames;
@@ -4841,7 +4837,7 @@ class DeclDeserializer {
48414837

48424838
DeclDeserializationError::Flags errorFlags;
48434839
return llvm::make_error<OverrideError>(
4844-
name, errorFlags, numVTableEntries);
4840+
name, errorFlags, numTableEntries);
48454841
}
48464842

48474843
for (TypeID dependencyID : argNameAndDependencyIDs) {
@@ -4850,7 +4846,7 @@ class DeclDeserializer {
48504846
DeclDeserializationError::Flags errorFlags;
48514847
return llvm::make_error<TypeError>(
48524848
name, takeErrorInfo(dependency.takeError()),
4853-
errorFlags, numVTableEntries);
4849+
errorFlags, numTableEntries);
48544850
}
48554851
}
48564852

@@ -7611,12 +7607,12 @@ Decl *ModuleFile::handleErrorAndSupplyMissingClassMember(
76117607
if (error.isDesignatedInitializer())
76127608
context.evaluator.cacheOutput(
76137609
HasMissingDesignatedInitializersRequest{containingClass}, true);
7614-
if (error.getNumberOfVTableEntries() > 0)
7610+
if (error.getNumberOfTableEntries() > 0)
76157611
containingClass->setHasMissingVTableEntries();
76167612

76177613
suppliedMissingMember = MissingMemberDecl::create(
76187614
context, containingClass, error.getName(),
7619-
error.getNumberOfVTableEntries(),
7615+
error.getNumberOfTableEntries(),
76207616
error.needsFieldOffsetVectorEntry());
76217617
};
76227618

@@ -7651,12 +7647,12 @@ Decl *handleErrorAndSupplyMissingProtoMember(ASTContext &context,
76517647
[&](const DeclDeserializationError &error) {
76527648
assert(error.needsFieldOffsetVectorEntry() == 0);
76537649

7654-
if (error.getNumberOfVTableEntries() > 0)
7650+
if (error.getNumberOfTableEntries() > 0)
76557651
containingProto->setHasMissingRequirements(true);
76567652

76577653
suppliedMissingMember = MissingMemberDecl::create(
76587654
context, containingProto, error.getName(),
7659-
error.getNumberOfVTableEntries(), 0);
7655+
error.getNumberOfTableEntries(), 0);
76607656
};
76617657
llvm::handleAllErrors(std::move(error), handleMissingProtocolMember);
76627658
return suppliedMissingMember;

‎lib/Serialization/DeserializationErrors.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ class DeclDeserializationError : public llvm::ErrorInfoBase {
269269
protected:
270270
DeclName name;
271271
Flags flags;
272-
uint8_t numVTableEntries = 0;
272+
uint8_t numTableEntries = 0;
273273

274274
public:
275275
DeclName getName() const {
@@ -279,8 +279,8 @@ class DeclDeserializationError : public llvm::ErrorInfoBase {
279279
bool isDesignatedInitializer() const {
280280
return flags.contains(Flag::DesignatedInitializer);
281281
}
282-
unsigned getNumberOfVTableEntries() const {
283-
return numVTableEntries;
282+
unsigned getNumberOfTableEntries() const {
283+
return numTableEntries;
284284
}
285285
bool needsFieldOffsetVectorEntry() const {
286286
return flags.contains(Flag::NeedsFieldOffsetVectorEntry);
@@ -424,10 +424,10 @@ class OverrideError : public llvm::ErrorInfo<OverrideError,
424424

425425
public:
426426
explicit OverrideError(DeclName name,
427-
Flags flags={}, unsigned numVTableEntries=0) {
427+
Flags flags={}, unsigned numTableEntries=0) {
428428
this->name = name;
429429
this->flags = flags;
430-
this->numVTableEntries = numVTableEntries;
430+
this->numTableEntries = numTableEntries;
431431
}
432432

433433
void log(raw_ostream &OS) const override {
@@ -489,11 +489,11 @@ class TypeError : public llvm::ErrorInfo<TypeError, DeclDeserializationError>,
489489

490490
public:
491491
explicit TypeError(DeclName name, std::unique_ptr<ErrorInfoBase> reason,
492-
Flags flags={}, unsigned numVTableEntries=0)
492+
Flags flags={}, unsigned numTableEntries=0)
493493
: ErrorWithUnderlyingReason(std::move(reason)) {
494494
this->name = name;
495495
this->flags = flags;
496-
this->numVTableEntries = numVTableEntries;
496+
this->numTableEntries = numTableEntries;
497497
}
498498

499499
void diagnose(const ModuleFile *MF) const;

‎lib/Serialization/ModuleFormat.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
5858
/// describe what change you made. The content of this comment isn't important;
5959
/// it just ensures a conflict if two people change the module format.
6060
/// Don't worry about adhering to the 80-column limit for this line.
61-
const uint16_t SWIFTMODULE_VERSION_MINOR = 812; // @_extern(wasm)
61+
const uint16_t SWIFTMODULE_VERSION_MINOR = 813; // VTable bit redefinition
6262

6363
/// A standard hash seed used for all string hashes in a serialized module.
6464
///
@@ -1524,7 +1524,7 @@ namespace decls_block {
15241524
DeclIDField, // overridden decl
15251525
BCFixed<1>, // whether the overridden decl affects ABI
15261526
AccessLevelField, // access level
1527-
BCFixed<1>, // requires a new vtable slot
1527+
BCFixed<1>, // requires a new vtable/witness table slot
15281528
BCFixed<1>, // 'required' but overridden is not (used for recovery)
15291529
BCVBR<5>, // number of parameter name components
15301530
BCArray<IdentifierIDField> // name components,
@@ -1561,7 +1561,7 @@ namespace decls_block {
15611561
AccessLevelField, // setter access, if applicable
15621562
DeclIDField, // opaque return type decl
15631563
BCFixed<2>, // # of property wrapper backing properties
1564-
BCVBR<4>, // total number of vtable entries introduced by all accessors
1564+
BCVBR<4>, // total number of vtable/witness table entries introduced by all accessors
15651565
BCArray<TypeIDField> // accessors, backing properties, and dependencies
15661566
>;
15671567

@@ -1605,7 +1605,7 @@ namespace decls_block {
16051605
BCVBR<5>, // 0 for a simple name, otherwise the number of parameter name
16061606
// components plus one
16071607
AccessLevelField, // access level
1608-
BCFixed<1>, // requires a new vtable slot
1608+
BCFixed<1>, // requires a new vtable/witness table slot
16091609
DeclIDField, // opaque result type decl
16101610
BCFixed<1>, // isUserAccessible?
16111611
BCFixed<1>, // is distributed thunk
@@ -1666,7 +1666,7 @@ namespace decls_block {
16661666
DeclIDField, // AccessorStorageDecl
16671667
AccessorKindField, // accessor kind
16681668
AccessLevelField, // access level
1669-
BCFixed<1>, // requires a new vtable slot
1669+
BCFixed<1>, // requires a new vtable/witness table slot
16701670
BCFixed<1>, // is transparent
16711671
BCFixed<1>, // is distributed thunk
16721672
BCArray<IdentifierIDField> // name components,
@@ -1754,7 +1754,7 @@ namespace decls_block {
17541754
StaticSpellingKindField, // is subscript static?
17551755
BCVBR<5>, // number of parameter name components
17561756
DeclIDField, // opaque return type decl
1757-
BCVBR<4>, // total number of vtable entries introduced by all accessors
1757+
BCVBR<4>, // total number of vtable/witness table entries introduced by all accessors
17581758
BCArray<IdentifierIDField> // name components,
17591759
// followed by DeclID accessors,
17601760
// followed by TypeID dependencies

‎lib/Serialization/Serialization.cpp‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3688,11 +3688,17 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
36883688
InlinableBodyTextLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode, body);
36893689
}
36903690

3691-
unsigned getNumberOfRequiredVTableEntries(
3691+
static bool getNeedsNewTableEntry(const AbstractFunctionDecl *func) {
3692+
if (isa_and_nonnull<ProtocolDecl>(func->getDeclContext()))
3693+
return func->requiresNewWitnessTableEntry();
3694+
return func->needsNewVTableEntry();
3695+
}
3696+
3697+
unsigned getNumberOfRequiredTableEntries(
36923698
const AbstractStorageDecl *storage) const {
36933699
unsigned count = 0;
36943700
for (auto *accessor : storage->getAllAccessors()) {
3695-
if (accessor->needsNewVTableEntry())
3701+
if (getNeedsNewTableEntry(accessor))
36963702
count++;
36973703
}
36983704
return count;
@@ -4296,7 +4302,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
42964302

42974303
auto rawIntroducer = getRawStableVarDeclIntroducer(var->getIntroducer());
42984304

4299-
unsigned numVTableEntries = getNumberOfRequiredVTableEntries(var);
4305+
unsigned numTableEntries = getNumberOfRequiredTableEntries(var);
43004306

43014307
unsigned abbrCode = S.DeclTypeAbbrCodes[VarLayout::Code];
43024308
VarLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode,
@@ -4322,7 +4328,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
43224328
rawAccessLevel, rawSetterAccessLevel,
43234329
S.addDeclRef(var->getOpaqueResultTypeDecl()),
43244330
numBackingProperties,
4325-
numVTableEntries,
4331+
numTableEntries,
43264332
arrayFields);
43274333
}
43284334

@@ -4422,7 +4428,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
44224428
fn->getName().getArgumentNames().size() +
44234429
fn->getName().isCompoundName(),
44244430
rawAccessLevel,
4425-
fn->needsNewVTableEntry(),
4431+
getNeedsNewTableEntry(fn),
44264432
S.addDeclRef(fn->getOpaqueResultTypeDecl()),
44274433
fn->isUserAccessible(),
44284434
fn->isDistributedThunk(),
@@ -4539,7 +4545,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
45394545
S.addDeclRef(fn->getStorage()),
45404546
rawAccessorKind,
45414547
rawAccessLevel,
4542-
fn->needsNewVTableEntry(),
4548+
getNeedsNewTableEntry(fn),
45434549
fn->isTransparent(),
45444550
fn->isDistributedThunk(),
45454551
dependencies);
@@ -4630,7 +4636,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
46304636
uint8_t rawStaticSpelling =
46314637
uint8_t(getStableStaticSpelling(subscript->getStaticSpelling()));
46324638

4633-
unsigned numVTableEntries = getNumberOfRequiredVTableEntries(subscript);
4639+
unsigned numTableEntries = getNumberOfRequiredTableEntries(subscript);
46344640

46354641
unsigned abbrCode = S.DeclTypeAbbrCodes[SubscriptLayout::Code];
46364642
SubscriptLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode,
@@ -4654,7 +4660,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
46544660
rawStaticSpelling,
46554661
subscript->getName().getArgumentNames().size(),
46564662
S.addDeclRef(subscript->getOpaqueResultTypeDecl()),
4657-
numVTableEntries,
4663+
numTableEntries,
46584664
nameComponentsAndDependencies);
46594665

46604666
writeGenericParams(subscript->getGenericParams());
@@ -4703,7 +4709,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
47034709
S.addDeclRef(overridden),
47044710
overriddenAffectsABI,
47054711
rawAccessLevel,
4706-
ctor->needsNewVTableEntry(),
4712+
getNeedsNewTableEntry(ctor),
47074713
firstTimeRequired,
47084714
ctor->getName().getArgumentNames().size(),
47094715
nameComponentsAndDependencies);

0 commit comments

Comments
 (0)