Revert Attempted Fix of SR6812 #1727
Conversation
The original fix being reverted was #1557 made _cfinfoa 32-bit, which is generally correct, but it was really attempting to fix the same issue that #1525 had fixed. Historically, _cfinfoa has been 64-bit on Linux dating back to at least Swift 3. In the short term, because forward fixing the issues with #1557 uncover other lurking 32-bit alignment issues on Linux, it is safer to revert #1557. But because it did make for some cleaner ifdefs on the Darwin side as well, the Darwin pieces are being left alone. Going forward, it will take some investigation to uncover the reasons behind the other 32-bit alongment issues. Original context around this problem and previous attempted fixes/reverts:
|
@swift-ci test |
|
I’ll be reviewing this on Monday. |
| @@ -194,19 +194,11 @@ typedef struct __CFRuntimeBase { | |||
| // This matches the isa and retain count storage in Swift | |||
| uintptr_t _cfisa; | |||
| uintptr_t _swift_rc; | |||
| // This is for CF's use, and must match _NSCFType layout | |||
| #if defined(__LP64__) || defined(__LLP64__) | |||
| // This is for CF's use, and must match __NSCFType/_CFInfo layout | |||
There was a problem hiding this comment.
Just one comment: do we want to move _CFInfo to match (one UInt64 instead of two UInt32s?) @mundaym
There was a problem hiding this comment.
By which I mean: in this same patch.
There was a problem hiding this comment.
I generally try to keep reverts away from other changes, personally. But that comes from a non-git background. In general, this PR, #1694 and #1709 are already getting pretty entangled as it is.
Is the purpose of this PR to unblock ARM32 caused by a regression, or to generally start to fix the underlying issues that caused the regression? If the former, I'd say keep this focused, and then it should be easy to follow up with the other fixes. If the latter, than we should probably have these commits coming in as a group, but then it gets a bit more difficult since I can't really test PPC BE on behalf of @mundaym.
There was a problem hiding this comment.
Then I’m good with this and let’s figure out the rest step by step.
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
|
Our national nightmare etc. |
The original fix being reverted was #1557 made _cfinfoa 32-bit, which is generally correct, but it was really attempting to fix the same issue that #1525 had fixed. Historically, _cfinfoa has been 64-bit on Linux dating back to at least Swift 3.
In the short term, because forward fixing the issues with #1557 uncover other lurking 32-bit alignment issues on Linux, it is safer to revert #1557. But because it did make for some cleaner ifdefs on the Darwin side as well, the Darwin pieces are being left alone. Going forward, it will take some investigation to uncover the reasons behind the other 32-bit alongment issues.
Original context around this problem and previous attempted fixes/reverts:
#1689
#1693
This does not attempt to fix PPC Big-Endian Issues also present here, brought up in #1709. That PR will need to be brought into alignment with this one and #1694