Skip to content

IRGen: Fix enable-testing of internal with resilient super class #42063

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
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
22 changes: 13 additions & 9 deletions lib/IRGen/ClassMetadataVisitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ template <class Impl> class ClassMetadataVisitor
asImpl().addIVarDestroyer();

// Class members.
addClassMembers(Target);
addClassMembers(Target, Target);
}

/// Notes the beginning of the field offset vector for a particular ancestor
Expand All @@ -110,9 +110,11 @@ template <class Impl> class ClassMetadataVisitor

private:
/// Add fields associated with the given class and its bases.
void addClassMembers(ClassDecl *theClass) {
void addClassMembers(ClassDecl *theClass,
ClassDecl *rootClass) {
// Visit the superclass first.
if (auto *superclassDecl = theClass->getSuperclassDecl()) {

if (superclassDecl->hasClangNode()) {
// Nothing to do; Objective-C classes do not add new members to
// Swift class metadata.
Expand All @@ -123,10 +125,9 @@ template <class Impl> class ClassMetadataVisitor
// not publically accessible (e.g private or internal). This would
// normally not happen except if we compile theClass's module with
// enable-testing.
} else if (IGM.hasResilientMetadata(superclassDecl, ResilienceExpansion::Maximal) &&
(theClass->getModuleContext() == IGM.getSwiftModule() ||
theClass->getFormalAccessScope(/*useDC=*/nullptr,
/*treatUsableFromInlineAsPublic=*/true).isPublic())) {
} else if (IGM.hasResilientMetadata(superclassDecl,
ResilienceExpansion::Maximal,
rootClass)) {
// Runtime metadata instantiation will initialize our field offset
// vector and vtable entries.
//
Expand All @@ -137,7 +138,8 @@ template <class Impl> class ClassMetadataVisitor
// NB: We don't apply superclass substitutions to members because we want
// consistent metadata layout between generic superclasses and concrete
// subclasses.
addClassMembers(superclassDecl);
addClassMembers(superclassDecl,
rootClass);
}
}

Expand All @@ -151,7 +153,8 @@ template <class Impl> class ClassMetadataVisitor

// If the class has resilient storage, we cannot make any assumptions about
// its storage layout, so skip the rest of this method.
if (IGM.isResilient(theClass, ResilienceExpansion::Maximal))
if (IGM.isResilient(theClass, ResilienceExpansion::Maximal,
rootClass))
return;

// A class only really *needs* a field-offset vector in the
Expand All @@ -175,7 +178,8 @@ template <class Impl> class ClassMetadataVisitor

// If the class has resilient metadata, we cannot make any assumptions
// about its metadata layout, so skip the rest of this method.
if (IGM.hasResilientMetadata(theClass, ResilienceExpansion::Maximal))
if (IGM.hasResilientMetadata(theClass, ResilienceExpansion::Maximal,
rootClass))
return;

// Add vtable entries.
Expand Down
12 changes: 8 additions & 4 deletions lib/IRGen/GenClass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,14 @@ namespace {
auto superclassDecl = superclassType.getClassOrBoundGenericClass();
assert(superclassType && superclassDecl);

if (IGM.hasResilientMetadata(superclassDecl, ResilienceExpansion::Maximal))
if (IGM.hasResilientMetadata(superclassDecl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fold this into a new IGM.hasResilientSuperclass() which takes the class (and not the superclass) and then does the necessary checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to "pass down" the fact that the super class is not to be treated as resilient to the recursive call addFieldsForClass /addClassMembers on the super class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, maybe there's some other way, but this is the second bug we've fixed related to enable-testing vs class resilience, and it looks like both fixes added accidental complexity. We're just factoring the computations incorrectly here.

ResilienceExpansion::Maximal,
rootClass))
Options |= ClassMetadataFlags::ClassHasResilientAncestry;

// If the superclass has resilient storage, don't walk its fields.
if (IGM.isResilient(superclassDecl, ResilienceExpansion::Maximal)) {
if (IGM.isResilient(superclassDecl, ResilienceExpansion::Maximal,
rootClass)) {
Options |= ClassMetadataFlags::ClassHasResilientMembers;

// If the superclass is generic, we have to assume that its layout
Expand Down Expand Up @@ -263,10 +266,11 @@ namespace {
if (classHasIncompleteLayout(IGM, theClass))
Options |= ClassMetadataFlags::ClassHasMissingMembers;

if (IGM.hasResilientMetadata(theClass, ResilienceExpansion::Maximal))
if (IGM.hasResilientMetadata(theClass, ResilienceExpansion::Maximal,
rootClass))
Options |= ClassMetadataFlags::ClassHasResilientAncestry;

if (IGM.isResilient(theClass, ResilienceExpansion::Maximal)) {
if (IGM.isResilient(theClass, ResilienceExpansion::Maximal, rootClass)) {
Options |= ClassMetadataFlags::ClassHasResilientMembers;
return;
}
Expand Down
34 changes: 32 additions & 2 deletions lib/IRGen/GenDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5334,6 +5334,22 @@ llvm::Constant *IRGenModule::getAddrOfGlobalUTF16String(StringRef utf8) {
return address;
}

/// Can not treat a treat the layout of a class as resilient if the current
/// class is defined in an external module and
/// not publically accessible (e.g private or internal).
/// This would normally not happen except if we compile theClass's module with
/// enable-testing.
static bool shouldTreatClassAsFragileBecauseOfEnableTesting(ClassDecl *D,
IRGenModule &IGM) {
if (!D)
return false;

return D->getModuleContext() != IGM.getSwiftModule() &&
!D->getFormalAccessScope(/*useDC=*/nullptr,
/*treatUsableFromInlineAsPublic=*/true)
.isPublic();
}

/// Do we have to use resilient access patterns when working with this
/// declaration?
///
Expand All @@ -5343,13 +5359,21 @@ llvm::Constant *IRGenModule::getAddrOfGlobalUTF16String(StringRef utf8) {
/// - For classes, the superclass might change the size or number
/// of stored properties
bool IRGenModule::isResilient(NominalTypeDecl *D,
ResilienceExpansion expansion) {
ResilienceExpansion expansion,
ClassDecl *asViewedFromRootClass) {
assert(!asViewedFromRootClass || isa<ClassDecl>(D));

if (D->getModuleContext()->getBypassResilience())
return false;
if (expansion == ResilienceExpansion::Maximal &&
Types.getLoweringMode() == TypeConverter::Mode::CompletelyFragile) {
return false;
}

if (shouldTreatClassAsFragileBecauseOfEnableTesting(asViewedFromRootClass,
*this))
return false;

return D->isResilient(getSwiftModule(), expansion);
}

Expand All @@ -5359,11 +5383,17 @@ bool IRGenModule::isResilient(NominalTypeDecl *D,
/// For classes, this means that virtual method calls use dispatch thunks
/// rather than accessing metadata members directly.
bool IRGenModule::hasResilientMetadata(ClassDecl *D,
ResilienceExpansion expansion) {
ResilienceExpansion expansion,
ClassDecl *asViewedFromRootClass) {
if (expansion == ResilienceExpansion::Maximal &&
Types.getLoweringMode() == TypeConverter::Mode::CompletelyFragile) {
return false;
}

if (shouldTreatClassAsFragileBecauseOfEnableTesting(asViewedFromRootClass,
*this))
return false;

return D->hasResilientMetadata(getSwiftModule(), expansion);
}

Expand Down
6 changes: 4 additions & 2 deletions lib/IRGen/IRGenModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -977,8 +977,10 @@ class IRGenModule {
substOpaqueTypesWithUnderlyingTypes(CanType type,
ProtocolConformanceRef conformance);

bool isResilient(NominalTypeDecl *decl, ResilienceExpansion expansion);
bool hasResilientMetadata(ClassDecl *decl, ResilienceExpansion expansion);
bool isResilient(NominalTypeDecl *decl, ResilienceExpansion expansion,
ClassDecl *asViewedFromRootClass = nullptr);
bool hasResilientMetadata(ClassDecl *decl, ResilienceExpansion expansion,
ClassDecl *asViewedFromRootClass = nullptr);
ResilienceExpansion getResilienceExpansionForAccess(NominalTypeDecl *decl);
ResilienceExpansion getResilienceExpansionForLayout(NominalTypeDecl *decl);
ResilienceExpansion getResilienceExpansionForLayout(SILGlobalVariable *var);
Expand Down
15 changes: 13 additions & 2 deletions test/IRGen/Inputs/resilient-class.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
open class Base {
var x = 1
var x = 1
}
internal struct TypeContainer {

enum SomeEnum:String {
case FirstCase = "first"
case SecondCase = "second"
}
}

internal class SubClass : Base {
var y = 2
var y : TypeContainer.SomeEnum

override init() {
y = .FirstCase
}
}
10 changes: 9 additions & 1 deletion test/IRGen/testing-enabled-resilient-super-class.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,22 @@
// RUN: %target-build-swift-dylib(%t/%target-library-name(resilient)) %S/Inputs/resilient-class.swift -module-name resilient -emit-module -emit-module-path %t/resilient.swiftmodule -enable-library-evolution -enable-testing
// RUN: %target-build-swift -I %t -L %t -lresilient %s -o %t/main %target-rpath(%t)
// RUN: %target-build-swift -O -I %t -L %t -lresilient %s -o %t/main %target-rpath(%t)
// RUN: %target-run %t/main %t/%target-library-name(resilient) | %FileCheck --check-prefix=EXEC-CHECK %s

@testable import resilient

// Don't access via the class offset global. Use a fragile access pattern instead.

// CHECK-NOT: s9resilient8SubClassCMo

public func isEqual<T:Equatable>(_ l: T, _ r: T) -> Bool {
return l == r
}

public func testCase() {
let t = SubClass()
print(t.y)
print(t.y) // EXEC-CHECK: FirstCase
print("isEqual \(isEqual(t.y, .FirstCase))") // EXEC-CHECK: isEqual true
}

testCase()