Skip to content

[WIP] SIL: Don't include convenience initializers in vtables. #19073

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[WIP] SIL: Don't include convenience initializers in vtables. #19073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 7 additions & 4 deletions include/swift/SIL/SILVTableVisitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,13 @@ template <class T> class SILVTableVisitor {
maybeAddEntry(constant, constant.requiresNewVTableEntry());
}

// All constructors have their initializing constructor in the
// vtable, which can be used by a convenience initializer.
SILDeclRef constant(cd, SILDeclRef::Kind::Initializer);
maybeAddEntry(constant, constant.requiresNewVTableEntry());
// Designated and/or required initializers have their initializing
// constructor in the vtable, which can be used by a convenience
// initializer.
if (cd->isDesignatedInit() || cd->isRequired()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition needs to be handled in AbstractFunctionDecl::computeNeedsNewVTableEntry rather than here, or else recovery when decls can't be deserialized won't work.

SILDeclRef constant(cd, SILDeclRef::Kind::Initializer);
maybeAddEntry(constant, constant.requiresNewVTableEntry());
}
}

void maybeAddEntry(SILDeclRef declRef, bool needsNewEntry) {
Expand Down
9 changes: 8 additions & 1 deletion lib/SIL/SILDeclRef.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,15 @@ swift::getMethodDispatch(AbstractFunctionDecl *method) {
return MethodDispatch::Static;

// Members defined directly inside a class are dynamically dispatched.
if (isa<ClassDecl>(dc))
if (isa<ClassDecl>(dc)) {
// Convenience initializers are not dynamically dispatched unless
// required.
if (auto ctor = dyn_cast<ConstructorDecl>(method)) {
if (!ctor->isRequired() && ctor->isConvenienceInit())
return MethodDispatch::Static;
}
return MethodDispatch::Class;
}

// Imported class methods are dynamically dispatched.
if (method->isObjC() && method->hasClangNode())
Expand Down
85 changes: 85 additions & 0 deletions test/SILGen/convenience_init_peer_delegation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// RUN: %target-swift-emit-silgen %s | %FileCheck %s

class X {
init() {
}

// Convenience inits must dynamically dispatch designated inits...
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfC
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfc
// CHECK: class_method %7 : $X, #X.init!initializer.1
convenience init(convenience: ()) {
self.init()
}

// ...but can statically invoke peer convenience inits
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC17doubleConvenienceACyt_tcfC
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC17doubleConvenienceACyt_tcfc
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfc
convenience init(doubleConvenience: ()) {
self.init(convenience: ())
}

// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC8requiredACyt_tcfC
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC8requiredACyt_tcfc
required init(required: ()) {
}

// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC19requiredConvenienceACyt_tcfC
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC19requiredConvenienceACyt_tcfc
required convenience init(requiredConvenience: ()) {
self.init(required: ())
}

// Convenience inits must dynamically dispatch required peer convenience inits
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC25requiredDoubleConvenienceACyt_tcfC
// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation1XC25requiredDoubleConvenienceACyt_tcfc
// CHECK: class_method %7 : $X, #X.init!initializer.1
required convenience init(requiredDoubleConvenience: ()) {
self.init(requiredDoubleConvenience: ())
}
}

// CHECK-LABEL: sil hidden @$S32convenience_init_peer_delegation11invocations2xtyAA1XCm_tF
func invocations(xt: X.Type) {
// CHECK: function_ref @$S32convenience_init_peer_delegation1XCACycfC
_ = X()
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfC
_ = X(convenience: ())
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC17doubleConvenienceACyt_tcfC
_ = X(doubleConvenience: ())
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC8requiredACyt_tcfC
_ = X(required: ())
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC19requiredConvenienceACyt_tcfC
_ = X(requiredConvenience: ())
// CHECK: function_ref @$S32convenience_init_peer_delegation1XC25requiredDoubleConvenienceACyt_tcfC
_ = X(requiredDoubleConvenience: ())

// CHECK: class_method %0 : $@thick X.Type, #X.init!allocator.1
_ = xt.init(required: ())
// CHECK: class_method %0 : $@thick X.Type, #X.init!allocator.1
_ = xt.init(requiredConvenience: ())
// CHECK: class_method %0 : $@thick X.Type, #X.init!allocator.1
_ = xt.init(requiredDoubleConvenience: ())
}

// CHECK-LABEL: sil_vtable X
// -- designated init()
// CHECK-NOT: @$S32convenience_init_peer_delegation1XCACycfC
// CHECK: @$S32convenience_init_peer_delegation1XCACycfc

// -- no unrequired convenience inits
// CHECK-NOT: @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfC
Copy link
Contributor

Choose a reason for hiding this comment

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

CHECK-NOT feels fragile because if the mangling ever changes slightly, and the vtable entry re-appears, the test won't catch it. How about SILVerifier checks instead that non-required constructors don't show up in the vtable?

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, would it be safe to do a more general verifier check that SILVTables only include entries visited by the SILVTableVisitor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, an entry should appear in the vtable iff its visited by the vtable visitor, but you also want the vtable visitor to only visit the right decls...

// CHECK-NOT: @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfc
// CHECK-NOT: @$S32convenience_init_peer_delegation1XC17doubleConvenienceACyt_tcfC
// CHECK-NOT: @$S32convenience_init_peer_delegation1XC17doubleConvenienceACyt_tcfc

// -- designated init(required:)
// CHECK: @$S32convenience_init_peer_delegation1XC8requiredACyt_tcfC
// CHECK: @$S32convenience_init_peer_delegation1XC8requiredACyt_tcfc
// -- convenience init(requiredConvenience:)
// CHECK: @$S32convenience_init_peer_delegation1XC19requiredConvenienceACyt_tcfC
// CHECK: @$S32convenience_init_peer_delegation1XC19requiredConvenienceACyt_tcfc
// -- convenience init(requiredDoubleConvenience:)
// CHECK: @$S32convenience_init_peer_delegation1XC25requiredDoubleConvenienceACyt_tcfC
// CHECK: @$S32convenience_init_peer_delegation1XC25requiredDoubleConvenienceACyt_tcfc