Order-independent protocol witness table instantiation #15387
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
force-pushed
the
protocol-resilience
branch
8 times, most recently
from
f2e2f12 to
ee16e7e
Compare
slavapestov
force-pushed
the
protocol-resilience
branch
from
ee16e7e to
844dc38
Compare
slavapestov
changed the title
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
slavapestov
force-pushed
the
protocol-resilience
branch
from
844dc38 to
1b27dac
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
slavapestov
force-pushed
the
protocol-resilience
branch
2 times, most recently
from
5e2f457 to
63fb724
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
slavapestov
force-pushed
the
protocol-resilience
branch
from
63fb724 to
9f7a0cc
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
rjmccall
reviewed
Mar 29, 2018
| @@ -834,6 +834,10 @@ void Remangler::mangleGenericProtocolWitnessTable(Node *node) { | |||
| mangleSingleChildNode(node); // protocol conformance | |||
| } | |||
|
|
|||
| void Remangler::mangleResilientProtocolWitnessTable(Node *node) { | |||
| unreachable("todo"); | |||
| } | |||
There was a problem hiding this comment.
If we're definitively not going to support remangling these things with the old remangler, we should probably do something consistent for all them. At any rate, "todo" does not seem like the right message. :)
There was a problem hiding this comment.
We only want to remangle things that come up in a nominal type, and these new symbols are definitely not it. I'll change the todo to something else.
| @@ -1056,6 +1056,11 @@ void Remangler::mangleGenericProtocolWitnessTableInstantiationFunction(Node *nod | |||
| Buffer << "WI"; | |||
| } | |||
|
|
|||
| void Remangler::mangleResilientProtocolWitnessTable(Node *node) { | |||
| mangleSingleChildNode(node); | |||
| Buffer << "Wr"; | |||
There was a problem hiding this comment.
Please add this to Mangling.rst. Also, I think I poked you about an earlier mangling you should've done that for.
| // Use it as the initializer for an anonymous constant. LLVM can treat this as | ||
| // equivalent to the global's GOT entry. | ||
| llvm::SmallString<64> name; | ||
| entity.mangle(name); |
There was a problem hiding this comment.
Is this trying to give the GOT equivalent the same name as the original global?
There was a problem hiding this comment.
createGOTEquivalent() prepends the "got."
lib/IRGen/IRGenModule.h
Outdated
| } | ||
| } | ||
|
|
||
| void forceLocalEmitOfLazyFunction(SILFunction *f) { | ||
| DefaultIGMForFunction[f] = CurrentIGM; | ||
| } |
There was a problem hiding this comment.
Does this work if the function is needed locally in multiple IGMs?
There was a problem hiding this comment.
The only thing that can relative reference a witness thunk is a witness table and we don’t share thunks between tables. If we ever did we would need a different hack.
…on't have to go at the end NFC until the new witness table instantiation mechanism is enabled.
…itness table Otherwise, they might end up in a different translation unit, and we will be unable to form a relative reference.
- Outlined value operations are now 'WO' and not 'W' - Protocol witness table pattern 'Wp' - Resilient protocol witness table 'Wr' - Protocol requirement table 'WR'
slavapestov
force-pushed
the
protocol-resilience
branch
from
9f7a0cc to
be07366
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
1 similar comment
|
@swift-ci Please test source compatibility |
|
Build failed |
|
@aciidb0mb3r Still seeing this: |
|
@swift-ci Please smoke test macOS |
|
@slavapestov Sorry about that. I thought the crash was a linux only thing. I'll disable it completely. |
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.
As part of resilience, we want to have the ability to re-order the requirements in a protocol without breaking client code. The caller side of this is already implemented by having protocol method calls go through thunks that are emitted as part of the protocol definition. This PR adds the missing piece, which is order-independent witness table instantiation.
The idea is that instead of emitting the witness table directly, we emit a "resilient" witness table consisting of a list of requirement/witness pairs which can appear in any order. Missing entires are permitted as long as the requirement has a default. The requirement is identified by its protocol method thunk.