Protocol witness thunks don't need public linkage #8387
Merged
Conversation
This file contains hidden or 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
protocol-witness-thunk-linkage
branch
from
6483b2f to
f8ca963
Compare
slavapestov
commented
Mar 30, 2017
| @@ -0,0 +1,42 @@ | |||
| // RUN: %target-swift-frontend -Xllvm -sil-full-demangle -emit-silgen %s | %FileCheck %s | |||
|
|
|||
| public protocol P { | |||
There was a problem hiding this comment.
@jrose-apple @huonw I invite you to marvel at the subtle complexity and delicious texture of this test case.
|
@swift-ci Please test |
slavapestov
changed the title
|
@jrose-apple The thing I don't get is that removing 'public' from 'publicRequirement()' causes Sema to reject the conformance, but SILGen just ignores it anyway because R is 'fileprivate'. I think is a consequence of how in SE-0025 (?) we decided that making things more public than the scope that contains them should be allowed, but ignored. I strongly feel this was a mistake. |
|
Build failed |
|
Build failed |
The existence of a shared_external function in itself is not an error; it just means we deserialized a witness table or vtable but did not need to deserialize a thunk. However, a direct reference to such a function is an error, because we should have deserialized the body in that case. This fixes a crasher, but the SIL crashers are kind of silly because the SIL parser does not try at all not to crash on invalid input.
slavapestov
force-pushed
the
protocol-witness-thunk-linkage
branch
from
f8ca963 to
dbdd0f0
Compare
|
@swift-ci Please test |
|
@swift-ci Please benchmark |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please benchmark |
|
Build failed |
Build comment file:Build failed before running benchmark. |
We used to give witness thunks public linkage if the conforming type and the protocol are public. This is completely unnecessary. If the conformance is fragile, the thunk should be [shared] [serialized], allowing the thunk to be serialized into callers after devirtualization. Otherwise for private protocols or resilient modules, witness thunks can just always be private. This should reduce the size of compiled binaries. There are two other mildly interesting consequences: 1) In the bridged cast tests, we now inline the witness thunks from the bridgeable conformances, which removes one level of indirection. 2) This uncovered a flaw in our accessibility checking model. Usually, we reject a witness that is less visible than the protocol; however, we fail to reject it in the case that it comes from an extension. This is because members of an extension can be declared 'public' even if the extended type is not public, and it appears that in this case the 'public' keyword has no effect. I would prefer it if a) 'public' generated a warning here, and b) the conformance also generated a warning. In Swift 4 mode, we could then make this kind of sillyness into an error. But for now, live with the broken behavior, and add a test to exercise it to ensure we don't crash. There are other places where this "allow public but ignore it, kinda, except respect it in some places" behavior causes problems. I don't know if it was intentional or just emergent behavior from general messiness in Sema. 3) In the TBD code, there is one less 'failure' because now that witness thunks are no longer public, TBDGen does not need to reason about them (except for the case swiftlang#2 above, which will probably require a similar workaround in TBDGen as what I put into SILGen).
slavapestov
force-pushed
the
protocol-witness-thunk-linkage
branch
from
dbdd0f0 to
6a83e73
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
@swift-ci Please benchmark |
1 similar comment
|
@swift-ci Please benchmark |
|
I think your analysis is correct. SE-0025 says this does not get a warning, and then something must be checking getFormalAccess where it should be looking at getFormalAccessScope. (The former really needs a rename.) |
Build comment file:Optimized (O) Regression (5)
Improvement (41)
No Changes (245)
Regression (2)
Improvement (0)No Changes (289)
|
|
Wow, those are some amazing wins and confusing losses. |
|
I don't understand the results. There wasn't supposed to be a change. |
|
@swift-ci Please benchmark |
|
Cost of interposable symbols, perhaps? Some things might be direct jumps instead of indirect now. But I don't know how things could get worse. |
Build comment file:Optimized (O) Regression (5)
Improvement (42)
No Changes (244)
Regression (1)
Improvement (0)No Changes (290)
|
|
@jrose-apple Same result! I know @aschwaighofer is still iterating on performance of existentials, and 42 improvements seems like a decent outcome, so can we live with that one regression? I'm glad I ran the benchmarks -- I was just expecting unexpected slowdowns or crashes. |
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.
Follow-up to #8422. Giving the thunks public linkage is unnecessary, so we make them shared (if the witness table is going to be serialized) or private (otherwise).
This exposes a flaw in how we map AST-level accessibility to symbol visibility. We seem to allow protocol conformances that we really should reject. I added a workaround but we should consider making the relevant case an error in Swift 4 mode, since it's going to be an issue for ABI resilience and TBDGen.