Cater for new Swift reference count representation #1525
Conversation
|
@gparker42 any comment on this one? |
CoreFoundation/Base.subproj/CFBase.h
Outdated
| @@ -449,11 +449,13 @@ CF_EXPORT double kCFCoreFoundationVersionNumber; | |||
| #endif | |||
|
|
|||
| #if __LLP64__ | |||
| typedef uint32_t _CFSwiftRCCount; | |||
There was a problem hiding this comment.
This still doesn't belong in CFBase.h. It is for CFRuntime.h and CFString.h only, and I would prefer they just put the #ifdef directly.
There was a problem hiding this comment.
The reason is that we're adding a new API and name here, and for all platforms - even those that don't use the Swift RC pattern.
|
A better placeholder for Swift's data would be a single uintptr_t on all architectures. The refcount storage is certainly not divided into two equal-size fields as described here; better to not tease the possibility of reading useful information from them. |
| uint32_t _swift_strong_rc; | ||
| uint32_t _swift_weak_rc; | ||
| _CFSwiftRCCount _swift_strong_rc; | ||
| _CFSwiftRCCount _swift_weak_rc; |
There was a problem hiding this comment.
So here I would write:
uintptr_t _cfisa;
#if __LLP64__
uint32_t _swift_strong_rc;
uint32_t _swift_weak_rc;
#else
int16_t _swift_strong_rc;
int16_t _swift_weak_rc;
#endif
_Atomic(uint64_t) _cfinfoa;
and so forth, for the others
|
@gparker42 thanks, even better |
|
How does it look now? |
|
LGTM. @parkera? |
Foundation/NSCFString.swift
Outdated
| let offTemp1 = MemoryLayout<OpaquePointer>.size + MemoryLayout<Int32>.size | ||
| let offTemp2 = MemoryLayout<Int32>.size + MemoryLayout<_CFInfo>.size | ||
| let offTemp1 = MemoryLayout<OpaquePointer>.size + MemoryLayout<uintptr_t>.size | ||
| let offset = offTemp1 + MemoryLayout<_CFInfo>.size | ||
| return offTemp1 + offTemp2 + MemoryLayout<UnsafePointer<UInt8>>.size |
There was a problem hiding this comment.
yeah, noticed that.. should be ok now
| @@ -412,11 +412,11 @@ CF_EXPORT void *__CFConstantStringClassReference[]; | |||
| #endif | |||
|
|
|||
| #define CONST_STRING_DECL(S, V) \ | |||
| const struct __CFConstStr __##S CONST_STRING_SECTION = {{(uintptr_t)&__CFConstantStringClassReference, _CF_CONSTANT_OBJECT_STRONG_RC, 0, 0x000007c8U}, (uint8_t *)(V), sizeof(V) - 1}; \ | |||
| const struct __CFConstStr __##S CONST_STRING_SECTION = {{(uintptr_t)&__CFConstantStringClassReference, _CF_CONSTANT_OBJECT_STRONG_RC, 0x000007c8U}, (uint8_t *)(V), sizeof(V) - 1}; \ | |||
There was a problem hiding this comment.
Do you need to change the definition of _CF_CONSTANT_OBJECT_STRONG_RC? My guess is that the bits generated here will be wrong if you do not.
There was a problem hiding this comment.
This will work on little-endian only perhaps?
#if DEPLOYMENT_RUNTIME_SWIFT
#define _CF_SWIFT_RC_PINNED_FLAG 0x1
#define _CF_SWIFT_RC_FLAGS_COUNT 2
#define _CF_CONSTANT_OBJECT_STRONG_RC ((1 << _CF_SWIFT_RC_FLAGS_COUNT) | _CF_SWIFT_RC_PINNED_FLAG)
There was a problem hiding this comment.
I think the point of splitting it into strong/weak here was so we could set that pinned flag. There may now be a better way.
There was a problem hiding this comment.
The Android tests run through ok. Are you saying separate them again so big-endian 64 bits works?
There was a problem hiding this comment.
I don't think you were setting the pinned flag correctly before. The pinned flag is the LSB of that uintptr_t on all platforms.
I don't think you are setting the refcount correctly now. The strong refcount is stored differently on 32-bit versus 64-bit, but on all platforms it is biased: a strong refcount of 1 is represented as bit value 0.
There was a problem hiding this comment.
#define _CF_CONSTANT_OBJECT_STRONG_RC (_CF_SWIFT_RC_PINNED_FLAG) then?
|
I’ve amended the commit and this is running on Android so should be good to go. Do you want to do a @swift-ci please test to check we’re on the right track? |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
once @gparker42 approves (since it's his struct we are messing with) we are good to merge |
|
Looks good now. |
|
Thanks! |
|
Looks like we’re done. Can someone push the merge button please and I can rebase the original PR? |
|
@swift-ci please test and merge |
|
(Ping me again if this doesn't merge in a timely fashion.) |
|
Actually bypassing swift-ci since it's not starting. |
|
Thanks @millenomi et all. |
The representation of reference counts for 32 bit systems has changed swiftlang/swift@ae1c984 and needs to be adjusted in the matching structs in CoreFoundation (__CFRuntimeBase & __CFConstStr). Swift ivars now start at offset 8 bytes from self on 32 bit systems. Part of #1518