-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
[6.0🍒] NCGenerics: fix runtime metadata request strategy (demangling vs response) #75147
Conversation
@swift-ci test |
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)
801fd85
to
73c882b
Compare
@swift-ci test |
(cherry picked from commit ec2a72e)
@swift-ci test |
@swift-ci test |
This needs a careful review before we accept it into 6.0. I'm not entirely certain I understand the implications here. |
There was a problem hiding this 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.
@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. |
Uh oh!
There was an error while loading. Please reload this page.