Avoid losing the high bits of bridged constant tagged pointers #31110
Conversation
|
This is not the right approach actually |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
Catfish-Man
changed the title
|
@swift-ci please test |
|
Build failed |
|
Build failed |
Catfish-Man
force-pushed
the
tagged-too-two
branch
from
9032822 to
c6100f8
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
Catfish-Man
force-pushed
the
tagged-too-two
branch
from
c6100f8 to
bf4fd69
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
| providesFastUTF8: false, //TODO: if contentsPtr is UTF8 compatible, use it | ||
| isASCII: taggedContents.asciiContentsPointer != nil, | ||
| isASCII: true, | ||
| length: taggedContents.utf16Length |
There was a problem hiding this comment.
Is this what we want, or do we want to bring it in as an immortal string? Have you tested that ref counting and message sending Swift-side works on that untagged object?
There was a problem hiding this comment.
I think we want to, but I'm trying to be conservative about the changes initially both to unblock people and to reduce risk. This limits the changes to just the bridging paths.
There was a problem hiding this comment.
(and yes, that's been tested, it's an ordinary NSCFConstantString)
There was a problem hiding this comment.
They'd also have be null-terminated.
| @@ -590,7 +619,8 @@ extension String { | |||
|
|
|||
| _internalInvariant(_guts._object.hasObjCBridgeableObject, | |||
| "Unknown non-bridgeable object case") | |||
| return _guts._object.objCBridgeableObject | |||
| let result = _guts._object.objCBridgeableObject | |||
| return formConstantTaggedCocoaString(untaggedCocoa: result) ?? result | |||
There was a problem hiding this comment.
Here we'd be checking a spare perf flag on the second word instead.
There was a problem hiding this comment.
Yeah. I think that's ultimately where we want to end up.
…eObject doesn't stomp the high bits
|
@swift-ci please test |
Catfish-Man
force-pushed
the
tagged-too-two
branch
from
bf4fd69 to
220f0cc
Compare
|
…git failed to push, so that previous test run was useless |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
| let retaggedPointer = UInt(bitPattern: constantPtr) | expectedConstantTagValue | ||
|
|
||
| return unsafeBitCast(retaggedPointer, to: AnyObject.self) | ||
| #endif |
There was a problem hiding this comment.
This still seems a little sketchy without at least a defer { _fixLifetime(untaggedCocoa) }. Make sure to check with @atrick or @eeckstein.
In practice, would ref counting always be disabled here? How are the retain/release counts balanced?
There was a problem hiding this comment.
These objects are in immutable memory, so can't be refcounted
| let payload = taggedValue & payloadMask | ||
| let ivarPointer = UnsafePointer<_swift_shims_builtin_CFString>( | ||
| bitPattern: payload | ||
| )! | ||
|
|
||
| guard _swift_stdlib_dyld_is_objc_constant_string( | ||
| unsafeBitCast(ivarPointer, to: UnsafeRawPointer.self) |
There was a problem hiding this comment.
I wonder if going through ObjectIdentifier is more sanctioned. @lorentey , thoughts?
| let length = ivarPointer.pointee.length | ||
| let isUTF16Mask:UInt = 0x0000_0000_0000_0004 //CFStringFlags bit 4: isUnicode | ||
| let isASCII = ivarPointer.pointee.flags & isUTF16Mask == 0 | ||
| precondition(isASCII) // we don't currently support non-ASCII here |
There was a problem hiding this comment.
What should be the behavior if this check fails? Should we trap in the stdlib?
There was a problem hiding this comment.
I think so. It's pretty much "assert the world is as we expected"
Fixes rdar://problem/61273899