[WIP] SIL: Don't include convenience initializers in vtables. #19073
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
|
Looks like swiftpm exercises a corner case not in our test suite... |
They're never dynamically dispatched (unless `required`), so this entry should never be used.
jckarter
force-pushed
the
no-convenience-vtable-entry
branch
from
95e05be to
81712c7
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
| // CHECK: @$S32convenience_init_peer_delegation1XCACycfc | ||
|
|
||
| // -- no unrequired convenience inits | ||
| // CHECK-NOT: @$S32convenience_init_peer_delegation1XC0A0ACyt_tcfC |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, would it be safe to do a more general verifier check that SILVTables only include entries visited by the SILVTableVisitor?
There was a problem hiding this comment.
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...
| // 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()) { |
There was a problem hiding this comment.
This condition needs to be handled in AbstractFunctionDecl::computeNeedsNewVTableEntry rather than here, or else recovery when decls can't be deserialized won't work.
|
Closing this in favor of #19151, which bundles the vtable fix with the changes to use allocating entry points as vtable entries. It ended up being easier to fix both at the same time. |
They're never dynamically dispatched (unless
required) [in theory], so this entry is never used.