(Arm32) Fix 32-bit Variable Offset Issues with CoreFoundation #1689
Conversation
_CFInfo Assumes a 64-bit _cfinfoa type, when it is really 32-bit on 32-bit systems. _NSCFType assumes a 32-bit _cfinfoa too, making things even more complicated. There's a couple approaches to addressing this: 1) Back _CFInfo with a UInt that, on paper, matches size with __CFInfoType 2) Add padding so we can guarantee that _CFInfo can always be 64-bit. Which one you pick depends on if this is guaranteed to be true: UInt == __CFInfoType Because Android has some interesting macros in the code, this change picks option 2, even though it does mean an extra 4 bytes for each bridgable object. In either case, it makes sense that _NSCFType should be backed by a _CFInfo instead of a raw int type. Share types when possible so we have consistent, understandable behavior.
|
This would add 4 bytes to our CFTypes on other 32 bit platforms, so I think we should go with option #1. It looks like |
|
That's fine. As long as someone is willing to buy off on the assertion that UInt on Swift and __CFInfoType in C should be the same size, and there aren't any back doors into getting a 64-bit __CFInfoType, I'm okay with that. I just saw this, which gave me pause: #if __LP64__ || DEPLOYMENT_TARGET_ANDROID
typedef uint64_t __CFInfoType;
/* ... */
#else
typedef uint32_t __CFInfoType;
/* ... */
#endifPerhaps my change should include a fix there to align it with the other uses: The problem with __NSCFType is that it isn't actually visible to CFRuntime.c, it uses CFRuntimeBase as the proxy. And nothing in the Swift side subclasses from __NSCFType, they insert the _CFInfo directly into the types. The architecture there reads a bit like an unfinished/abandoned thought. EDIT: No, you are right that CFRuntime does get access to it. Interesting. What's surprising is that this doesn't create problems, though, because CFRuntimeBase/CFInfo/NSCFType should all agree on what _cfinfo looks like. |
|
Seems like I should try to page @johnno1962 on the issue with __CFInfoType, as they checked in the change initially that added |
|
Here's Option 1. May need more changes, I haven't validated it yet. Waiting for my local Arm32 swift build to finish so I can test it against a couple real projects I have affected by this bug. |
|
In the meantime, let’s see if it regresses anything on our side: |
|
@swift-ci please test |
|
So, option 1 doesn't actually seem to work, although it's not clear exactly what piece of code is being naughty. In simple cases, alignment/stride of member variables works and matches between C and Swift, and so bridging is okay. But there are other cases where they don't match, which I have yet to find. The result of these cases are heap corruption. I guess my question is, after so long using 8 bytes (4 byte cfinfo + 4 byte pad, and later an 8 byte cfinfo) which kept alignment clean on 8-byte boundaries (16 bytes on 32-bit, 24 bytes on 32-bit), why is it anathema to effectively revert the change that dropped the padding? The history behind the change to use uint32_t is messy:
Here's really what I want to do: Foundation has effectively had an 8-byte aligned CFRuntimeBase since it's origin in this repository. Because of the interaction of these two things, it is no longer 8-byte aligned, and there's problems because of it. Those problems are difficult to track down. (Edit: and likely to only get worse as more focus is poured into 64-bit where this struct is guaranteed to be 8-byte aligned, letting more assumptions about alignment leak into the code without automation running on 32-bit systems to catch them) While my first commit in the PR isn't ideal, it returns Foundation to the status quo that has been working, and has allowed Swift 3 to run so well on these devices. The change to uint32_t wasn't to save memory, and without resources to track down the side-effects of that change, it seems safer to simply revert it and keep CFRuntimeBase 8-byte aligned. |
|
There is a bit of history behind this problem. In Swift 3, the preamble of a Swift Object was 12 bytes on a 32 bit system (isa + two 32 bit refcounts) and the type of _cfinfoa was char[8]. in Swift4 after the CF foundation code was synced _cfinfoa became uint64_t which with 8 byte memory alignment added a new 4 bytes padding to the __CFRuntimeBase structure after the ref counts while in Swift4 preamble became 8 bytes (isa plus 32 bit unified refcount) just before 4.0 was released. This created a problem because while the __CFRuntimeBase structure increased in size the memory allocated for swift instances decreased so the core foundation code was writing off the end of Swift objects when they were bridged and anarchy ensuing. This was fixed by #1525. This has interacted badly with another subsequent fix (for the same problem?) a3e72a9 that should be reverted if it is still in master as it results in an ungainly size for __CFRuntimeBase of 12 bytes which may end up padded and didn’t update struct _CFInfo on the Swift side. I recommend the type of _cfinfoa always be 64 bits for everything to tie together. I do not recommend merging this PR and back out a3e72a9 instead. You’ll need to check the status of the various Apple branches in particular 4.2. If you’re looking for a reference point of something that has run the foundation tests and works on 32 bits use This branch which contains a single commit relative to the original 4.1 to get things working. The DEPLOYMENT_TARGET_ANDROID conditional on __CFInfoType is a red herring that is not visible to Swift, was only added to prevent warnings during the C build and should only affect an Android build. |
|
Yeah, a revert approach would be cleaner. I’ll just do local testing to be sure. :) |
|
Sounds good. I’ve just built the 4.2 branch on Android and apart from minor fixes the tests are in good shape. I’ve raised a PR to revert the second incompatible fix on master. |
|
There's some concern that we need a change to fix a more specific issue being caused by __NSCFType using Int32 instead _CFInfo, a couple of us on swift-arm slack are still looking at it. We may still need parts of my PR here that align __NSCFType if that's true. But it's easy to add that as a separate commit to the revert, we just need to confirm if we need it or not. |
|
__NSCFType should probably have a _CFInfo ivar from the look of things. |
Yeah, that's one of the things this PR does, so I think we should preserve that change with a separate PR. And boo, I had a branch just ready to pull when you beat me to it. :( |
|
Closing this one, because of #1693. A new PR will be opened with the __NSCFType part of the change. |
_CFInfo Assumes a 64-bit _cfinfoa type, when it is really 32-bit on 32-bit systems.
_NSCFType assumes a 32-bit _cfinfoa too, making things even more complicated.
This impacts Swift on arm32v7 platforms like Raspberry Pi 3, and has been causing crashes when using Foundation types that bridge between C and Swift, because shared instance variables are offset by 4 bytes (the extra padding). String being a common example.
There's a couple approaches to addressing this:
Which one you pick depends on if this is guaranteed to be true: Swift's UInt == CoreFoundation's __CFInfoType
Because Android has some interesting macros in the code that prevented me from answering this question with high confidence, this change picks option 2 which is less risky, even though it does mean an extra 4 bytes for each bridgable object. Option 1 would use less memory on 32-bit systems.
In either case, it makes sense that __NSCFType should be backed by a _CFInfo instead
of a raw int type so that things remain consistent. Share types when possible so we have consistent, understandable behavior. Question is, does __NSCFType even get used anymore? Is it meant to back something else that always uses a 32-bit _cfinfo?
In terms of validation, this was tested on Raspbian Stretch, using armhf architecture (armv7 specifically), using an existing project (http://github.com/Kaiede/RPiLight) that was able to produce the original bug against the swift-4.1.2-RELEASE tag doing a 'swift test' on the package. To validate against swift-4.1.2-RELEASE, I also needed to cherry-pick commit 1086b3b which fixes another 32-bit issue. The swift RC field is 32-bit on arm32, not 64, so that commit is required for 32-bit to work as well.