Revert "https://bugs.swift.org/browse/SR-6812" #1693
Conversation
|
@swift-ci test |
|
Bump |
|
Let's hold off on this for a bit longer; @millenomi is working in these files at the moment |
|
OK, @ me if you want a review of the result. This area is pretty critical for 32bit systems. |
There was a problem hiding this comment.
Can you double-check how this would align with the changes in https://github.com/apple/swift-corelibs-foundation/pull/1708/files ?
|
|
||
| #else | ||
|
|
||
| typedef struct __CFRuntimeBase { | ||
| uintptr_t _cfisa; | ||
| #if defined(__LP64__) || defined(__LLP64__) |
There was a problem hiding this comment.
This change is suspect. Why are we touching Darwin’s definition of this structure?
There was a problem hiding this comment.
Because the PR this one reverts did as well.
The PR that this reverts was an incorrect attempted fix for Arm 32, which honestly just made things worse because _CFInfo just became even more unaligned with CFRuntimeBase. Hence the need to revert. (And @johnno1962 put in the correct fix to _swift_rc which fixed the alignment, clashing with this fix)
I'm fine removing this part out of the revert though. As you say, it is Darwin, and I'm more concerned about fixing the bad "fix" as it was originally applied for Linux.
|
@Kaiede can you coordinate with @johnno1962 on this patch? |
|
@millenomi I’m happy to wait until you merge #1708 and then pick up the pieces after a rebase. There may be none if _cfinfoa regains it 64 bitted-ness unless the bad patch has made it’s way upstream :( |
|
I’m closing this for now as it seems to cover areas not intended. @Kaiede, If you want to pick this up since I’m not working in the area at the moment I can review your PR. The only thing that needs to be reverted is the first part of this diff before @millenomi’s comment and the part in CFString.h. The best source of information is discussion on #1689 (comment). The real solution in the long run is some form of CI on a 32 bit system that extends as far as running the foundation tests. |
|
@johnno1962 Agreed on the CI. One of the goals I have in the short term is getting Arm32 clean on 4.2, and then using that to push forward to the tip. Unfortunately, I'm currently bogged down in compiler-level breaks on Arm32, but once that is addressed, it shouldn't be too horrific. I guess I'll do this again in my fork and open a third PR on this issue. \o/ Honestly, this is one of the key things that Arm 32 (and technically Linux x86 32-bit too) has to have in order for Foundation to work correctly in master. Thankfully, the 4.2 branch didn't get the bad fix this reverts, and we have to patch 4.1 anyways to get builds out for historaical reasons. |
Reverts #1557 as it double solves a problem on 32 bit systems solved in #1525. The two fixes are incompatible with each other. See discussion on #1689 (comment). I’ve built the 4.2 branch which doesn’t have this double fix on 32 bit android and the foundation tests run.