Skip to content

Fix for KeyPathMultiModule test failure #25329

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.

Sign up for GitHub

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

Jump to bottom
Merged
jckarter merged 1 commit into from
Jun 25, 2019
Merged

Fix for KeyPathMultiModule test failure #25329

jckarter merged 1 commit into from
Jun 25, 2019

Conversation

@levivic
Copy link
Contributor

@levivic levivic commented Jun 10, 2019

This commit changes the pointer of unresolvedIndirectOffset from size UInt32 to size UInt64, so that it is compatible on both big (s390x) and little endian platforms. It resolves the failure of stdlib/KeyPathMultiModule.swift on big-endian platform. The related discussion can be found at https://forums.swift.org/t/keypath-test-case-v5-0-failed-on-big-endian-platform/19529

@levivic levivic mentioned this pull request Jun 20, 2019
Merged
@levivic
Copy link
Contributor Author

levivic commented Jun 24, 2019

Can somebody take a look at this request?

@ianpartridge
Copy link
Contributor

ianpartridge commented Jun 24, 2019

@jckarter

@jckarter
Copy link
Contributor

jckarter commented Jun 24, 2019 •
edited
Loading

Is this superseded by #25467 ? It is not correct to do a 64-bit load for structs. I see this is only used for out-of-line class offsets. This should be fine, then. Thanks!

@jckarter
Copy link
Contributor

jckarter commented Jun 24, 2019

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jun 24, 2019

Build failed
Swift Test OS X Platform
Git Sha - 5e4c3005e92d94be5b219492b7bbb7e89531f630

@jckarter
Copy link
Contributor

jckarter commented Jun 24, 2019

Ah, the field is pointer-sized, not always 64 bit. For compatibility on 32-bit platforms, we ought to use UInt.

jckarter
jckarter requested changes Jun 24, 2019
View reviewed changes
stdlib/public/core/KeyPath.swift Outdated
Copy link
Contributor

@jckarter jckarter Jun 24, 2019

Choose a reason for hiding this comment

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

This should be UInt, for 32-bit portability. LGTM otherwise.

Copy link
Contributor Author

@levivic levivic Jun 24, 2019

Choose a reason for hiding this comment

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

Thanks! Change applied.

@levivic levivic force-pushed the s390x-keypathmultimodule-fix branch from 5e4c300 to 6ac15e9 Compare June 24, 2019 20:19
@jckarter
Copy link
Contributor

jckarter commented Jun 24, 2019

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jun 24, 2019

Build failed
Swift Test OS X Platform
Git Sha - 5e4c3005e92d94be5b219492b7bbb7e89531f630

@swift-ci
Copy link
Contributor

swift-ci commented Jun 24, 2019

Build failed
Swift Test Linux Platform
Git Sha - 5e4c3005e92d94be5b219492b7bbb7e89531f630

@jckarter jckarter merged commit a868cfb into swiftlang:master Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jckarter jckarter jckarter approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@levivic @ianpartridge @jckarter @swift-ci