GSB: Move signature verification from CanGenericSignature::getCanonical() to computeGenericSignature() #36411
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
…al() to computeGenericSignature() Doing this when computing a canonical signature didn't really make sense because canonical signatures are not canonicalized any more strongly _with respect to the builder_; they just canonicalize their requirement types. Instead, let's do these checks after creating the signature in computeGenericSignature(). The old behavior had another undesirable property; since the canonicalization was done by registerGenericSignatureBuilder(), we would always build a new GSB from scratch for every signature we compute. The new location also means we do these checks for protocol requirement signatures as well. This flags an existing fixed crasher where we still emit bogus same-type requirements in the requirement signature, so I moved this test back into an unfixed state.
Contributor
Author
|
@swift-ci Please smoke test |
Contributor
Author
|
@swift-ci Please test source compatibility |
Contributor
|
I looked more into why this PR is regressing on my linux box. This might be helpful: |
slavapestov
added a commit
to slavapestov/swift
that referenced
this pull request
Mar 12, 2021
I added this test case in swiftlang#36402; the change in swiftlang#36411 correctly flags some bogus same-type requirements in a minimized signature and asserts.
Contributor
Author
|
@davezarzycki Sorry about that. The new assertion is correct; previously the test used FileCheck to detect the same failure. I'm going to XFAIL it until the other issue flagged by that test is fixed. |
Contributor
|
Okay. For whatever it may be worth, I was also surprised by the |
Contributor
Author
|
@davezarzycki The first RUN line checks that some generic signatures minimize correctly. Since they don't currently minimize correctly (FileCheck fails because one of the CHECK: lines is different) I added a 'not' in front of it. The second RUN: line ensures that IRGen can generate code without crashing, which exercises some conformance access path code unrelated to the same-type constraint minimization. I didn't realize the other PR actually turned the first into an assert by enabling some existing asserts in more cases. Once I fix the underlying issue, the assert will be fixed and the FileCheck lines will pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Doing this when computing a canonical signature didn't really
make sense because canonical signatures are not canonicalized
any more strongly with respect to the builder; they just
canonicalize their requirement types.
Instead, let's do these checks after creating the signature in
computeGenericSignature().
The old behavior had another undesirable property; since the
canonicalization was done by registerGenericSignatureBuilder(),
we would always build a new GSB from scratch for every
signature we compute.
The new location also means we do these checks for protocol
requirement signatures as well. This flags an existing fixed
crasher where we still emit bogus same-type requirements in
the requirement signature, so I moved this test back into
an unfixed state.