Skip to content

[Serialization] Drop overriding relationship in constructors when safe #63266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3094,7 +3094,7 @@ class DeclDeserializer {
GenericSignatureID genericSigID;
uint8_t storedInitKind, rawAccessLevel;
DeclID overriddenID;
bool needsNewVTableEntry, firstTimeRequired;
bool overriddenAffectsABI, needsNewVTableEntry, firstTimeRequired;
unsigned numArgNames;
ArrayRef<uint64_t> argNameAndDependencyIDs;

Expand All @@ -3104,6 +3104,7 @@ class DeclDeserializer {
async, throws, storedInitKind,
genericSigID,
overriddenID,
overriddenAffectsABI,
rawAccessLevel,
needsNewVTableEntry,
firstTimeRequired,
Expand All @@ -3130,15 +3131,25 @@ class DeclDeserializer {
attrs.setRawAttributeChain(DAttrs);
}

auto overridden = MF.getDeclChecked(overriddenID);
if (!overridden) {
// Pass through deserialization errors.
if (overridden.errorIsA<FatalDeserializationError>())
return overridden.takeError();
Expected<Decl *> overriddenOrError = MF.getDeclChecked(overriddenID);
Decl *overridden;
if (overriddenOrError) {
overridden = overriddenOrError.get();
} else if (overriddenOrError.errorIsA<FatalDeserializationError>()) {
// Pass through fatal deserialization errors.
return overriddenOrError.takeError();
} else if (MF.allowCompilerErrors()) {
// Drop overriding relationship when allowing errors.
llvm::consumeError(overriddenOrError.takeError());
overridden = nullptr;
} else {
llvm::consumeError(overriddenOrError.takeError());
if (overriddenAffectsABI || !ctx.LangOpts.EnableDeserializationRecovery) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we set overridden = nullptr if we're allowing compiler errors in this case (ie. MF.allowCompilerErrors)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure 👍

return llvm::make_error<OverrideError>(name, errorFlags,
numVTableEntries);
}

llvm::consumeError(overridden.takeError());
return llvm::make_error<OverrideError>(
name, errorFlags, numVTableEntries);
overridden = nullptr;
}

for (auto dependencyID : argNameAndDependencyIDs.slice(numArgNames)) {
Expand Down Expand Up @@ -3198,7 +3209,7 @@ class DeclDeserializer {
ctx.evaluator.cacheOutput(NeedsNewVTableEntryRequest{ctor},
std::move(needsNewVTableEntry));

ctor->setOverriddenDecl(cast_or_null<ConstructorDecl>(overridden.get()));
ctor->setOverriddenDecl(cast_or_null<ConstructorDecl>(overridden));
if (auto *overridden = ctor->getOverriddenDecl()) {
if (!attributeChainContains<RequiredAttr>(DAttrs) ||
!overridden->isRequired()) {
Expand Down
3 changes: 2 additions & 1 deletion lib/Serialization/ModuleFormat.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
/// Don't worry about adhering to the 80-column limit for this line.
const uint16_t SWIFTMODULE_VERSION_MINOR = 741; // SIL pack types
const uint16_t SWIFTMODULE_VERSION_MINOR = 742; // Constructor affects ABI

/// A standard hash seed used for all string hashes in a serialized module.
///
Expand Down Expand Up @@ -1455,6 +1455,7 @@ namespace decls_block {
CtorInitializerKindField, // initializer kind
GenericSignatureIDField, // generic environment
DeclIDField, // overridden decl
BCFixed<1>, // whether the overridden decl affects ABI
AccessLevelField, // access level
BCFixed<1>, // requires a new vtable slot
BCFixed<1>, // 'required' but overridden is not (used for recovery)
Expand Down
17 changes: 12 additions & 5 deletions lib/Serialization/Serialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3535,17 +3535,20 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
// its overrides after they've been compiled: if the declaration is '@objc'
// and 'dynamic'. In that case, all accesses to the method or property will
// go through the Objective-C method tables anyway.
if (overridden->hasClangNode() || overridden->shouldUseObjCDispatch())
if (!isa<ConstructorDecl>(override) &&
(overridden->hasClangNode() || overridden->shouldUseObjCDispatch()))
return false;

// In a public-override-internal case, the override doesn't have ABI
// implications.
// implications. This corresponds to hiding the override keyword from the
// module interface.
auto isPublic = [](const ValueDecl *VD) {
return VD->getFormalAccessScope(VD->getDeclContext(),
/*treatUsableFromInlineAsPublic*/true)
.isPublic();
};
if (isPublic(override) && !isPublic(overridden))
if (override->getDeclContext()->getParentModule()->isResilient() &&
isPublic(override) && !isPublic(overridden))
return false;

return true;
Expand Down Expand Up @@ -4499,9 +4502,12 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
uint8_t rawAccessLevel = getRawStableAccessLevel(ctor->getFormalAccess());

bool firstTimeRequired = ctor->isRequired();
if (auto *overridden = ctor->getOverriddenDecl())
auto *overridden = ctor->getOverriddenDecl();
if (overridden) {
if (firstTimeRequired && overridden->isRequired())
firstTimeRequired = false;
}
bool overriddenAffectsABI = overriddenDeclAffectsABI(ctor, overridden);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the relationship actually useful if it doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. For non-resilient modules it could be used by optimizations. For resilient modules in most cases it should now be dropped by deserialization safety for the public-override-internal case, and the clang case is there more as tolerance for poor modularization and such I believe. I think we could try always dropping it when reading from a resilient module.


unsigned abbrCode = S.DeclTypeAbbrCodes[ConstructorLayout::Code];
ConstructorLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode,
Expand All @@ -4517,7 +4523,8 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
ctor->getInitKind()),
S.addGenericSignatureRef(
ctor->getGenericSignature()),
S.addDeclRef(ctor->getOverriddenDecl()),
S.addDeclRef(overridden),
overriddenAffectsABI,
rawAccessLevel,
ctor->needsNewVTableEntry(),
firstTimeRequired,
Expand Down
10 changes: 10 additions & 0 deletions test/Serialization/Safety/override-internal-func.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
//--- Lib.swift

open class Base {
internal init() {}

public func publicMethod() -> Int {
return 1
}
Expand All @@ -33,6 +35,10 @@ open class Base {
}

open class Derived : Base {
public override init() {
super.init()
}

open override func publicMethod() -> Int {
return super.publicMethod() + 1
}
Expand Down Expand Up @@ -63,3 +69,7 @@ public class OtherFinalDerived : Derived {
return super.internalMethod() + 1
}
}

func foo() {
let a = Derived()
}