[Swift 3.0] Fix s390x Enum load/storeMultiPayloadValue #4463
Conversation
|
@swift-ci Please test macOS |
|
Looks good, I'm assuming the tests pass on your platform :) |
|
Actually, I don't think we can take this for the 3.0 branch. |
|
Is it OK if we just check in the fix to master and leave 3.0 branch as-is to lower risk? @tkremenek |
| // If the payload is larger than the value, zero out the rest. | ||
| if (layout.payloadSize > sizeof(payloadValue)) | ||
| memset(bytes + sizeof(payloadValue), 0, | ||
| layout.payloadSize - sizeof(payloadValue)); | ||
| #endif |
There was a problem hiding this comment.
The #endif is now after the code that would previously had been available when __BIG_ENDIAN__ is not defined. Is this change intended?
There was a problem hiding this comment.
There is also clearly a behavioral change here, but no test case reflecting that. I don't accepting this change would be acceptable for the swift-3.0-branch in the current state without a test case. It's also not clear to me if the change is correct, per my comment about the preprocessor guard.
There was a problem hiding this comment.
Thanks for the review.
The code from lines 365-368 are still available when __BIG_ENDIAN__ is not defined (as they are between #else and #endif). Nothing has changed for little endian platforms.
The Interpreter/enum.swift test was failing on s390x and is now passing with this change. We didn't observe any regression in other tests with this change.
@garyliu1, FYI
There was a problem hiding this comment.
Further explaining the move of the #endif...
The new code between line 354-360 is the corresponding memset work required for big endian platforms. Therefore, the original memset code (which was common before) would need to be made specific for little endian platforms or else the big endian platform would perform another memset and corrupt the existing data.
There was a problem hiding this comment.
You both are absolutely right. Sorry for misreading the code. This looks perfectly safe to take.
What's in this pull request?
This is the same fix as #4461 for the swift-3.0-branch.
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.