Skip to content

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
merged 5 commits into from
Jul 16, 2024

Conversation

kavon
Copy link
Member

@kavon kavon commented Jul 1, 2024

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.

@kavon
Copy link
Member Author

kavon commented Jul 1, 2024

@swift-ci test

@kavon kavon changed the title NCGenerics: fix partial backdeployment support NCGenerics: fix runtime demangling strategy Jul 1, 2024
@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 85862b0 to 4360c1f Compare July 8, 2024 22:37
@kavon kavon marked this pull request as ready for review July 8, 2024 22:37
@kavon kavon requested a review from rjmccall as a code owner July 8, 2024 22:37
@kavon
Copy link
Member Author

kavon commented Jul 8, 2024

@swift-ci test

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 4360c1f to 30244a4 Compare July 8, 2024 22:45
@kavon
Copy link
Member Author

kavon commented Jul 8, 2024

@swift-ci test

@kavon kavon requested review from slavapestov and jckarter July 8, 2024 22:45
@kavon kavon changed the title NCGenerics: fix runtime demangling strategy NCGenerics: fix runtime metadata request strategy (demangling vs response) Jul 8, 2024
@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 30244a4 to 0b93060 Compare July 9, 2024 22:25
@kavon
Copy link
Member Author

kavon commented Jul 9, 2024

@swift-ci smoke test

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 0b93060 to 9f8dec1 Compare July 10, 2024 17:56
@kavon
Copy link
Member Author

kavon commented Jul 10, 2024

@swift-ci smoke test

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 9f8dec1 to 34e6ed9 Compare July 10, 2024 20:50
@kavon
Copy link
Member Author

kavon commented Jul 10, 2024

@swift-ci smoke test

@kavon
Copy link
Member Author

kavon commented Jul 15, 2024

@swift-ci smoke test macOS

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 34e6ed9 to 6767f2e Compare July 16, 2024 00:07
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci smoke test macOS

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch 2 times, most recently from e892813 to d0ee902 Compare July 16, 2024 02:34
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci smoke test macOS

@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from d0ee902 to 6bf6782 Compare July 16, 2024 04:52
kavon added 2 commits July 15, 2024 22:10
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.
@kavon kavon force-pushed the ncgenerics-runtime-demangling-fix branch from 6bf6782 to d755e90 Compare July 16, 2024 05:14
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci test

@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci test

1 similar comment
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci test

@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

The only test that isn't passing still:

'Swift(iphonesimulator-x86_64) :: Interpreter/moveonly_reflection.swift'

An error was encountered processing the command (domain=NSPOSIXErrorDomain, code=2):
The operation couldn’t be completed. No such file or directory
No such file or directory

This appears to be some test-setup issue. My guess is the not binary isn't in the iphonesimulator.

@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci smoke test

@kavon kavon merged commit fedd5e2 into swiftlang:main Jul 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant