Fix CFInfo on 64-bit big endian platforms. #1709
Conversation
Foundation/NSSwiftRuntime.swift
Outdated
| @@ -87,7 +87,13 @@ extension ObjCBool : CustomStringConvertible { | |||
| #endif | |||
|
|
|||
| internal class __NSCFType : NSObject { | |||
| #if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le) | |||
There was a problem hiding this comment.
Do we not have a word-size macro like C does here?
There was a problem hiding this comment.
Not that I could find sadly. This seems to be the standard way to do it at the moment:
Foundation/NSSwiftRuntime.swift:90:#if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/NSObjCRuntime.swift:218:#if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/CGFloat.swift:16:#elseif arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/CGFloat.swift:188:#elseif arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/NSNumber.swift:710: #if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/NSNumber.swift:718: #if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
Foundation/NSRange.swift:129: #elseif arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64le)
Foundation/NSRange.swift:391:#elseif arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
TestFoundation/TestNSNumber.swift:1061: #if arch(x86_64) || arch(arm64) || arch(s390x) || arch(powerpc64) || arch(powerpc64le)
There was a problem hiding this comment.
This change also conflicts with a backout we've been trying to do for 32-bit that keeps getting shuffled, reopened, and closed, and expected to be re-opened again. And really, this particular use should be a _CFInfo, not a raw Int.
There was a problem hiding this comment.
Because this _cfinfo should match the size of _CFInfo
|
Seems reasonable. Want to make sure this lines up with the work @millenomi is doing to merge right now. |
|
In addition to the specific comment, I'll point out that #1689 already fixes _CFInfo in a cleaner way, but was rejected in favor of a revert. I'd recommend borrowing the implementation from the swift files. |
|
As soon as I'm finished with the merge, I want to sit down and figure out what is the best solution here. #1694 we want for sure, and let's chat about the definition in CFRuntimeBase. |
|
Is there a Slack or something we can use for the three of us for that? Right now, Arm32 and i686 have been having to use patches to build 4.1, 4.2, or the master branch and not immediately segfault. So I've been chomping at the bit and disappointed at the cycle of PRs on this issue for a couple weeks now. #1694 isn't even the blocking issue for us. :( |
|
Yeah, sorry; it's just been a bit of bad timing at the moment because of the behind-the-scenes work for the merge. You can contact me via private message on the Swift forums if necessary! |
|
And I can also see to set up something for multiple people if needed. |
|
Since we're hijacking @mundaym's PR for this, it's only fair to bring them to the table as well, I think? |
|
@Kaiede @mundaym @johnno1962 Can you join a Slack space (via this link) to discuss the direction of this? |
|
Worked for me. |
mundaym
force-pushed
the
s390x-cfinfo-fix
branch
from
2eac2cf to
1d5d9d6
Compare
Prior to this change 64-bit platforms treated _cfinfoa as both a 64-bit and 32-bit variable. When accessing _cfinfoa as a 32-bit variable the code makes the assumption that it is accessing the lowest (or rightmost) bits of _cfinfoa. Unfortunately on big-endian platforms it is actually accessing the high (or leftmost) bits. Fix this issue by unconditionally treating _cfinfoa as a 64-bit value when targeting a 64-bit platform.
mundaym
force-pushed
the
s390x-cfinfo-fix
branch
from
1d5d9d6 to
68412be
Compare
|
@swift-ci Please test. |
|
Hi, I've brought this PR up to date with the changes upstream. Can you take another look? Thanks! |
|
Friendly ping. |
|
friendly ping |
|
ping |
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Prior to this change 64-bit platforms treated _cfinfoa as both a
64-bit and 32-bit variable. When accessing _cfinfoa as a 32-bit
variable the code makes the assumption that it is accessing the
lowest (or rightmost) bits of _cfinfoa. Unfortunately on big-endian
platforms it is actually accessing the high (or leftmost) bits.
Fix this issue by unconditionally treating _cfinfoa as a 64-bit
value when targeting a 64-bit platform.