-
Notifications
You must be signed in to change notification settings - Fork 10.5k
NCGenerics: fix runtime metadata request strategy (demangling vs response) #74864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
kavon
merged 5 commits into
swiftlang:main
from
kavon:ncgenerics-runtime-demangling-fix
Jul 16, 2024
Merged
NCGenerics: fix runtime metadata request strategy (demangling vs response) #74864
kavon
merged 5 commits into
swiftlang:main
from
kavon:ncgenerics-runtime-demangling-fix
Jul 16, 2024
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
@swift-ci test |
85862b0
to
4360c1f
Compare
@swift-ci test |
4360c1f
to
30244a4
Compare
@swift-ci test |
30244a4
to
0b93060
Compare
@swift-ci smoke test |
0b93060
to
9f8dec1
Compare
@swift-ci smoke test |
9f8dec1
to
34e6ed9
Compare
@swift-ci smoke test |
@swift-ci smoke test macOS |
34e6ed9
to
6767f2e
Compare
@swift-ci smoke test macOS |
e892813
to
d0ee902
Compare
@swift-ci smoke test macOS |
d0ee902
to
6bf6782
Compare
We generally should use the open-coded, metadata function "accessor" strategy when targeting older runtimes, but if the type is part of the stdlib, assume the runtime demangler will produce correct metadata, as the retrofitting of Optional, etc, was done Carefully. resolves rdar://131337585
We have had a heuristic that lets you reflect fields of types, where the field's type is conditionally Copyable, despite the reflection infrastructure always copying a field. That heuristic was added to allow ubiquitous types like Optional in the stdlib to continue to be reflected, even if the Optional at runtime really isn't Copyable. As a consequence, we were also allowing reflection of fields containing user-defined conditionally copyable types, when that's unsafe for no real benefit, yielding runtime crashes! I think in that case it's better to fall-back on the non-crashy case of reflection seeing it as the EmptyTupleType, which isn't inhabited, so it won't try to copy the field and instead basically skip-over it until a future runtime supports the reflection safely. So, this patch limits the dangerous reflection to only stdlib-defined types, until Mirror and friends are updated.
6bf6782
to
d755e90
Compare
@swift-ci test |
This was referenced Jul 16, 2024
@swift-ci test |
1 similar comment
@swift-ci test |
The only test that isn't passing still:
This appears to be some test-setup issue. My guess is the |
@swift-ci smoke test |
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.
NCGenerics: fix runtime metadata request strategy
We generally should use the open-coded, metadata function "accessor"
strategy when targeting older runtimes, but if the type is part of
the stdlib, assume the runtime demangler will produce correct
metadata, as the retrofitting of Optional, etc, was done Carefully.
resolves rdar://131337585
NCGenerics: limit field metadata heuristic
We have had a heuristic that lets you reflect fields of types, where
the field's type is conditionally Copyable, despite the reflection
infrastructure always copying a field. That heuristic was added to allow
ubiquitous types like Optional in the stdlib to continue to be
reflected, even if the Optional at runtime really isn't Copyable.
As a consequence, we were also allowing reflection of fields containing
user-defined conditionally copyable types, when that's unsafe for no
real benefit, yielding runtime crashes!
I think in that case it's better to fall-back on the non-crashy case of
reflection seeing it as the EmptyTupleType, which isn't inhabited, so it
won't try to copy the field and instead basically skip-over it until a
future runtime supports the reflection safely.
So, this patch limits the dangerous reflection to only stdlib-defined
types, until Mirror and friends are updated.