Skip to content

[6.0🍒] NCGenerics: fix runtime metadata request strategy (demangling vs response) #75147

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

Conversation

kavon
Copy link
Member

@kavon kavon commented Jul 10, 2024

  • Explanation: Fixes the way we obtain type metadata; limits which field metadata is available to reflection for non-stdlib types.
  • Scope: Important for correct operation of types using noncopyable generics on older runtimes.
  • Issue: rdar://131337585
  • Original PR: NCGenerics: fix runtime metadata request strategy (demangling vs response) #74864
  • Risk: Low. This generally changes the strategy used for type metadata so it's more broadly applicable to more systems. Also, by limiting field metadata for some non-stdlib types, we're eliminating a runtime crash when iterating over fields of a type. We preserve the ability to reflect Optional and such.
  • Testing: Swift CI.
  • Reviewer: TBD

@kavon
Copy link
Member Author

kavon commented Jul 10, 2024

@swift-ci test

kavon added 2 commits July 15, 2024 22:19
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

(cherry picked from commit 76bb266)
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.

(cherry picked from commit d755e90)
@kavon kavon force-pushed the 6.0-ncgenerics-runtime-demangling-fix branch from 801fd85 to 73c882b Compare July 16, 2024 05:20
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@swift-ci test

@kavon kavon marked this pull request as ready for review July 16, 2024 05:24
@kavon kavon requested a review from a team as a code owner July 16, 2024 05:24
(cherry picked from commit ec2a72e)
@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

@tbkka
Copy link
Contributor

tbkka commented Jul 16, 2024

This needs a careful review before we accept it into 6.0. I'm not entirely certain I understand the implications here.

CC: @jckarter @slavapestov

@kavon kavon requested review from jckarter and DougGregor July 16, 2024 17:12
Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can refine the field reflection filter further—since it's running code already, for instance, we could precisely detect whether the field type is copyable or not after instantiating its metadata. But I think this is still better than what we have currently.

@kavon kavon enabled auto-merge July 16, 2024 20:38
@kavon
Copy link
Member Author

kavon commented Jul 16, 2024

@tbkka There are two strategies we use for obtaining type metadata, one via demangling, and the other as a metadata "accessor" which is a function emitted into the binary to build the metadata manually. The latter works on all systems, but the former depends on there being a correct runtime to do the demangling and will save on code size.

This PR has us be more cautious and use the function accessor when targeting a darwin platform prior to Swift 6.0, unless if the type is part of the stdlib. We assume that types in the stdlib were upgraded with tricks to preserve the mangling of symbols.

Similarly, the field reflection metadata filter gets a bit more narrow to prevent runtime crashes when reflecting a type's non-stdlib child member, but is still a coarse approximation of what we could do, which is to use the "accessor" strategy and augment it with a dynamic check for copyability. Nevertheless, this is an improvement in terms of safety upon what we were doing before.

@kavon kavon merged commit de50caa into swiftlang:release/6.0 Jul 16, 2024
5 checks passed
@kavon kavon deleted the 6.0-ncgenerics-runtime-demangling-fix branch July 16, 2024 22:58
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.

3 participants