-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix KeyPath failure on s390x #25467
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
Fix KeyPath failure on s390x #25467
Conversation
fadcbbe
to
a814994
Compare
Hi, @jckarter, as we discussed in https://forums.swift.org/t/keypath-issue-with-s390x/24473, can you please review this PR? I have also got another related PR #25329 outstanding. Any feedback on that one will be appreciated as well. |
@@ -1495,7 +1495,7 @@ struct TargetTupleTypeMetadata : public TargetMetadata<Runtime> { | |||
ConstTargetMetadataPointer<Runtime, swift::TargetMetadata> Type; | |||
|
|||
/// The offset of the tuple element within the tuple. | |||
StoredSize Offset; | |||
uint32_t Offset; |
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.
Looks good! For ABI compatibility on Apple platforms, we'd want to conditionalize this, because existing binaries will try to load the full 64 bits, and we want to keep those zeroed:
#if __APPLE__
StoredSize Offset;
#else
uint32_t Offset;
#endif
I think the IRGen side is fine, though, because in practice nobody is going to have a 4GB tuple (they'll run into other practical limits first), and new binaries doing a 32-bit little-endian load of the 64-bit field should be safe, and would open the door to us possibly being able to drop this compatibility hack in time.
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.
@jckarter Change made. Thanks.
a814994
to
c151d8b
Compare
@swift-ci Please test |
Build failed |
Build failed |
@swift-ci Please test |
Build failed |
Build failed |
Looks like a test needs to be updated:
|
c151d8b
to
e08359c
Compare
@jckarter , yes, i have just updated the test. |
@swift-ci Please test |
Build failed |
Build failed |
This is to change the tuple metadata to use a 32-bit offset to address the failure seen in KeyPath test on big-endian platform (s390x). Details of the issue and discussion can be found at https://forums.swift.org/t/keypath-issue-with-s390x/24473