SILGen: Correctly emit vtables when an override is more visible than the base #25137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slavapestov
changed the title
slavapestov
force-pushed
the
vtable-derived-more-visible-than-base
branch
from
a51b136
to
7056a5c
Compare
slavapestov
commented
May 31, 2019
| // This is really unfortunate. In Swift 5.0, the method descriptor for this | ||
| // initializer was public and subclasses would "inherit" it, referencing its | ||
| // method descriptor from their class override table. | ||
| @usableFromInline |
There was a problem hiding this comment.
@airspeedswift I'm sorry.
When checking if a vtable override is ABI compatible with the base class method, make sure to check yields too. Also, add support for coroutines to vtable thunks, using code that I've copy and pasted and tweaked from witness thunks. (It would be nice to combine witness thunks and vtable thunks into a single code path for 'method thunks', but that requires some additional refactoring, so live with the copy and paste for now).
We used to try to emit a 'default initializer' in a class with a superclass, as long as the superclass was 'default initializable', meaning it had at least one designated initializer where all parameters had a default expression. However this code path was never taken, because the designated initializer inheritance mechanism supercedes it. Probably it became dead once we implemented inheritance of initializers with default arguments.
There was a behavioral difference between binary modules and textual interfaces. Since a binary module includes information about all members, including private and internal members, we would emit overrides for all designated initializers when subclassing a class, including those we cannot access. This was problematic because then we reference the superclass initializer's method descriptor, which had to be forced to have public linkage so that the subclass could reference it. However a textual interface only includes public members so any such initializers were simply dropped. The IRGen hack is thus no longer necessary when textual interfaces are used. In reality neither behavior is correct because the presence of inaccessible initializers should inhibit the inheritance of any designated initializer; however I'm going to try to fix that separately since it is a source breaking change and thus needs to be staged in as a warning. The latter fix is tracked by <rdar://problem/51249311>.
…ld be @usableFromInline
…rs public linkage This was done even for non-public inits because subclasses would always override the base class's designated initializers, even if they were inaccessible. This is an ABI break, however in practice the only affected class initializer was ManagedBuffer.init(_doNotCallMe:()), and we can just make it @usableFromInline.
…the base If an override B.f() is more visible than a base method A.f(), it is possible that an override C.f() of B.f() cannot see the original method A.f(). In this case, we would encounter linker errors if we referenced the method descriptor or method dispatch thunk for A.f(). Make this work by treating B.f() as the least derived method in this case, and ensuring that the vtable thunk for B.f() dispatches through the vtable again. Fixes <rdar://problem/48330571>, <https://bugs.swift.org/browse/SR-10648>.
slavapestov
force-pushed
the
vtable-derived-more-visible-than-base
branch
from
7056a5c
to
a312489
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
slavapestov
changed the title
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If an override B.f() is more visible than a base method A.f(), it is
possible that an override C.f() of B.f() cannot see the original method
A.f().
In this case, we would encounter linker errors if we referenced the
method descriptor or method dispatch thunk for A.f().
Make this work by treating B.f() as the least derived method in this
case, and ensuring that the vtable thunk for B.f() dispatches through
the vtable again.
Fixes rdar://problem/48330571, https://bugs.swift.org/browse/SR-10648.