Add a flag to track null termination of Strings #80075

Catfish-Man · 2025-03-18T00:03:49Z

Fixes rdar://148134880

Not tracking termination has several issues:

If there exist any cases where we’re not terminated today (unclear, but not disallowed), we will produce incorrect results when bridging to ObjC and calling CFStringGetCStringPtr()
Even if no such cases exist today, we are likely to introduce them in the future
There exist cases where we are terminated today, but don’t know it, sacrificing performance for withCString, which should be a very fast API

Catfish-Man · 2025-03-18T00:05:26Z

@swift-ci please smoke test

Catfish-Man · 2025-03-18T01:06:46Z

@swift-ci please smoke test

Catfish-Man · 2025-03-18T01:14:25Z

@swift-ci please smoke test

Catfish-Man · 2025-03-18T08:33:14Z

@swift-ci please smoke test

Catfish-Man · 2025-03-18T08:33:28Z

@swift-ci please Apple Silicon benchmark

stdlib/public/core/StringBridge.swift

stdlib/public/core/StringObject.swift

stephentyrone · 2025-03-18T15:26:47Z

stdlib/public/core/StringObject.swift

+  @_alwaysEmitIntoClient
+  @inline(__always) // Swift 6.1
+  internal var isKnownNullTerminated: Bool {
+    if isSmall { return true }


How does this work when the small count is 15? Need to be slightly careful with this if it's dependent on the spill-for-contiguous-access path zeroing that byte, because I don't think we get to use that path when producing a Span/UTF8Span. (@glessard, @milseman?)

(I.e. I think this probably works today, but if I'm right in my suspicion, we'd need to make sure no one tries to use this for a Span fast-path directly)

This is mirroring what the existing isFastZeroTerminated accessor does already, but I'm open to discussion if that's the right thing.

I spoke to @glessard about UTF8Span already and he says it doesn't rely on termination anyway, which is… awkward in general, but convenient here.

Given that we have already handled isSmall where this flag is used, I think I would simply not handle it here and make the definition just be _countAndFlags.isNullTerminated to dodge the question.

When the small count is 15 (or whatever the longest small string is for a particular runtime,) then we are pedantically not null-terminated. I think the point is to know whether we could pass an in-place address and rely on a null being in the right place? In that case I would say if isSmall { return count < _SmallString.capacity }.

Sounds good

…-want-to-live-forever' into come-with-me-if-you-want-to-live-forever

Catfish-Man · 2025-03-18T20:31:49Z

@swift-ci please smoke test

Catfish-Man · 2025-03-18T20:31:55Z

@swift-ci please Apple Silicon benchmark

…o for smol strings

Catfish-Man · 2025-03-19T01:06:44Z

Huh. I feel like this one isn't me?

<unknown>:0: error: could not find module 'CxxStdlib' for target 'x86_64-apple-macos'; found: arm64-apple-macos, at: /Users/ec2-user/jenkins/workspace/swift-PR-macos-perf-apple-silicon/Ninja-Release/swift-macosx-arm64/lib/swift/macosx/CxxStdlib.swiftmodule
<unknown>:0: error: could not find module 'CxxStdlib' for target 'x86_64-apple-macos'; found: arm64-apple-macos, at: /Users/ec2-user/jenkins/workspace/swift-PR-macos-perf-apple-silicon/Ninja-Release/swift-macosx-arm64/lib/swift/macosx/CxxStdlib.swiftmodule
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/x86_64-apple-macos.swiftinterface:1:1: error: failed to build module 'Darwin'; this SDK is not supported by the compiler (the SDK is built with 'Apple Swift version 5.9.2 (swiftlang-5.9.2.2.11 clang-1500.1.0.2.2)', while this compiler is 'Swift version 6.2-dev effective-5.10 (LLVM 7135e91d29f6694, Swift 347c10f0e167997)'). Please select a toolchain which matches the SDK.

stephentyrone · 2025-03-19T17:51:39Z

That is not you.

Catfish-Man · 2025-03-19T21:03:40Z

@swift-ci please smoke test

Catfish-Man · 2025-03-19T21:03:46Z

@swift-ci please Apple Silicon benchmark

Catfish-Man · 2025-03-20T00:20:16Z

@swift-ci please test

Catfish-Man · 2025-03-20T00:26:13Z

stdlib/public/SwiftShims/swift/shims/CoreFoundationShims.h


+SWIFT_RUNTIME_STDLIB_API
+const __swift_uint8_t * _Nullable
+_swift_stdlib_NSStringUTF8StringTrampoline(id _Nonnull obj);


My kingdom for a way to force swiftc to generate an objc_msgSend_super call so I don't have to do this ridiculous hoop jumping

stdlib/public/core/StringStorageBridge.swift

Catfish-Man · 2025-03-20T18:10:08Z

@swift-ci please test

Catfish-Man · 2025-03-20T18:10:15Z

@swift-ci please Apple Silicon benchmark

…ant-to-live-forever

test/stdlib/StringBridge.swift

…regular string storage

Catfish-Man · 2025-03-21T00:16:11Z

@swift-ci please test

Catfish-Man · 2025-03-21T00:16:19Z

@swift-ci please Apple Silicon benchmark

stdlib/public/core/StringObject.swift

…rings that can provide a fast UTF8 pointer

Catfish-Man · 2025-03-21T07:58:41Z

@swift-ci please test

Catfish-Man · 2025-03-21T07:59:30Z

stdlib/public/core/StringStorageBridge.swift

  final internal func _utf8String() -> UnsafePointer<UInt8>? {
    if asString._guts._object.isFastZeroTerminated {
-      return unsafe start
+      return start


Not entirely clear on why this doesn't need an unsafe, but it was warning about it

…ASCII

Catfish-Man · 2025-03-28T21:22:11Z

@swift-ci please test

Catfish-Man · 2025-03-28T21:22:18Z

@swift-ci please Apple Silicon benchmark

Catfish-Man · 2025-03-28T22:43:43Z

More unrelated failures

Cancelling nested steps due to timeout
Sending interrupt signal to process
ninja: build stopped: interrupted by user.

Catfish-Man · 2025-03-31T22:21:46Z

Found a problem that I'm not quite sure how to solve:

We want to use the "tail allocated" representation that string literals use, since that's how we avoid allocations
Existing callers of withCString have an inlined check that assumes tail-allocated implies null-terminated.
Unlike the previous inlining problem, they don't call any non-inlined functions in this path, so there's nowhere to insert a followup check

The good news is that withCString is the only callsite that has this problem, and that the only paths we actually create non-terminated strings on are not inlinable. So if we can find a way to detect that the process hasn't been recompiled since before the flag was added, then we could disable creating non-terminated strings, with only a loss of performance.

Another alternative would be to only ever make non-terminated allocated shared strings, which would cost a malloc for the shell object on creation, but avoid this issue completely.

Catfish-Man self-assigned this Mar 18, 2025

Catfish-Man force-pushed the come-with-me-if-you-want-to-live-forever branch from 97f44db to af21302 Compare March 18, 2025 01:06

Catfish-Man force-pushed the come-with-me-if-you-want-to-live-forever branch from af21302 to a8f85e6 Compare March 18, 2025 01:13

WIP: add a flag to track null termination of string literals

76d48b9

Catfish-Man force-pushed the come-with-me-if-you-want-to-live-forever branch from a8f85e6 to 76d48b9 Compare March 18, 2025 08:33

stephentyrone reviewed Mar 18, 2025

View reviewed changes

stdlib/public/core/StringBridge.swift Outdated Show resolved Hide resolved

stephentyrone reviewed Mar 18, 2025

View reviewed changes

stdlib/public/core/StringObject.swift Outdated Show resolved Hide resolved

stephentyrone reviewed Mar 18, 2025

View reviewed changes

stdlib/public/core/StringObject.swift Outdated Show resolved Hide resolved

stephentyrone reviewed Mar 18, 2025

View reviewed changes

Catfish-Man added 5 commits March 18, 2025 13:15

Remove unnecessary ABI

8715633

Add new ABI

79e8358

Merge branch 'main' into come-with-me-if-you-want-to-live-forever

da98c52

Rename as requested

0c8fb4c

Merge remote-tracking branch 'refs/remotes/origin/come-with-me-if-you…

b1f4e1f

…-want-to-live-forever' into come-with-me-if-you-want-to-live-forever

Sidestep the question of whether isNullTerminated should say yes or n…

f8522f4

…o for smol strings

Make non-terminated shared strings bridging-safe

6c53a52

Catfish-Man commented Mar 20, 2025

View reviewed changes

stdlib/public/core/StringStorageBridge.swift Outdated Show resolved Hide resolved

Catfish-Man marked this pull request as ready for review March 20, 2025 19:10

Catfish-Man requested a review from a team as a code owner March 20, 2025 19:10

Catfish-Man added 3 commits March 20, 2025 15:10

Merge remote-tracking branch 'origin/main' into come-with-me-if-you-w…

a9d5370

…ant-to-live-forever

Add a test for bridging the new SPI

69385a2

Add symbols on x86 too

37b1770

glessard reviewed Mar 20, 2025

View reviewed changes

test/stdlib/StringBridge.swift Outdated Show resolved Hide resolved

Catfish-Man added 2 commits March 20, 2025 16:28

Fix availability and fix up test

344f41a

Plumb flags through bridging properly, and check them in shared, not …

eaced32

…regular string storage

stephentyrone reviewed Mar 21, 2025

View reviewed changes

stdlib/public/core/StringObject.swift Show resolved Hide resolved

stephentyrone approved these changes Mar 21, 2025

View reviewed changes

Set null terminated to true for non-shared strings and for bridged st…

1f8e86a

…rings that can provide a fast UTF8 pointer

Catfish-Man commented Mar 21, 2025

View reviewed changes

Catfish-Man added 5 commits March 28, 2025 12:17

Merge branch 'main' into come-with-me-if-you-want-to-live-forever

093135c

Fix withCString in the presence of old binaries, and optimize it for …

a34dfc8

…ASCII

Remove now-redundant thing

0eed4cc

Fix a warning

f33b8ad

Merge branch 'main' into come-with-me-if-you-want-to-live-forever

8025cc0

Catfish-Man enabled auto-merge (squash) March 29, 2025 00:02

Catfish-Man changed the title ~~Add a flag to track null termination of string literals~~ Add a flag to track null termination of Strings Mar 29, 2025

Catfish-Man mentioned this pull request Mar 29, 2025

Adopt indirect tagged strings #80385

Closed

Catfish-Man disabled auto-merge March 31, 2025 22:21

Add a flag to track null termination of Strings #80075

Are you sure you want to change the base?

Add a flag to track null termination of Strings #80075

Uh oh!

Conversation

Catfish-Man commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentyrone Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

stephentyrone Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

glessard Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 18, 2025

Uh oh!

Catfish-Man commented Mar 19, 2025

Uh oh!

stephentyrone commented Mar 19, 2025

Uh oh!

Catfish-Man commented Mar 19, 2025

Uh oh!

Catfish-Man commented Mar 19, 2025

Uh oh!

Catfish-Man commented Mar 20, 2025

Uh oh!

Catfish-Man Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Catfish-Man commented Mar 20, 2025

Uh oh!

Catfish-Man commented Mar 20, 2025

Uh oh!

Uh oh!

Catfish-Man commented Mar 21, 2025

Uh oh!

Catfish-Man commented Mar 21, 2025

Uh oh!

Uh oh!

Catfish-Man commented Mar 21, 2025

Uh oh!

Catfish-Man Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Mar 28, 2025

Uh oh!

Catfish-Man commented Mar 28, 2025

Uh oh!

Catfish-Man commented Mar 28, 2025

Uh oh!

Catfish-Man commented Mar 31, 2025

Uh oh!

Reviewers

Catfish-Man commented Mar 18, 2025 •

edited

Loading

stephentyrone Mar 18, 2025 •

edited

Loading

glessard Mar 18, 2025 •

edited

Loading