[stdlib] Rework String breadcrumbs initialization/loading

This is a wild guess at what might be causing our persistent, random
String failures on the main branch:

```
  Swift(macosx-x86_64) :: Prototypes/CollectionTransformers.swift
  Swift(macosx-x86_64) :: stdlib/NSSlowString.swift
  Swift(macosx-x86_64) :: stdlib/NSStringAPI.swift
  Swift(macosx-x86_64) :: stdlib/StringIndex.swift
  Swift-validation(macosx-x86_64) :: stdlib/String.swift
  Swift-validation(macosx-x86_64) :: stdlib/StringBreadcrumbs.swift
  Swift-validation(macosx-x86_64) :: stdlib/StringUTF8.swift
```

FWIW, it appears this is *not* caused by #62717:
that change has also landed on release/5.8, and I haven’t seen these
issues on that branch.

Our atomic breadcrumbs initialization vs its non-atomic loading
gives me an uneasy feeling that this may in fact be a long standing
synchronization issue that is only now causing problems (for whatever
reason). I am unable to reproduce these issues locally, so this guess
may be (and probably is) wildly off the mark, but this PR is likely
to be a good idea anyway, if only to rule out this possibility.

rdar://104751936

Author: lorentey

stdlib/public/core/Runtime.swift

@@ -131,16 +131,19 @@ func _stdlib_atomicInitializeARCRef(
   object target: UnsafeMutablePointer<AnyObject?>,
   desired: AnyObject
 ) -> Bool {
+  // Note: this assumes that AnyObject? is layout-compatible with a RawPointer
+  // that simply points to the same memory.
   var expected: UnsafeRawPointer?
-  let desiredPtr = Unmanaged.passRetained(desired).toOpaque()
+  let unmanaged = Unmanaged.passRetained(desired)
+  let desiredPtr = unmanaged.toOpaque()
   let rawTarget = UnsafeMutableRawPointer(target).assumingMemoryBound(
     to: Optional<UnsafeRawPointer>.self)
   let wonRace = _stdlib_atomicCompareExchangeStrongPtr(
     object: rawTarget, expected: &expected, desired: desiredPtr)
   if !wonRace {
     // Some other thread initialized the value.  Balance the retain that we
     // performed on 'desired'.
-    Unmanaged.passUnretained(desired).release()
+    unmanaged.release()
   }
   return wonRace
 }
@@ -157,6 +160,44 @@ func _stdlib_atomicLoadARCRef(
   return nil
 }
 
+@_transparent
+@_alwaysEmitIntoClient
+@discardableResult
+public func _stdlib_atomicAcquiringInitializeARCRef<T: AnyObject>(
+  object target: UnsafeMutablePointer<T?>,
+  desired: __owned T
+) -> Unmanaged<T> {
+  // Note: this assumes that AnyObject? is layout-compatible with a RawPointer
+  // that simply points to the same memory, and that `nil` is represented by an
+  // all-zero bit pattern.
+  let unmanaged = Unmanaged.passRetained(desired)
+  let desiredPtr = unmanaged.toOpaque()
+
+  let (value, won) = Builtin.cmpxchg_acqrel_acquire_Word(
+    target._rawValue,
+    0._builtinWordValue,
+    Builtin.ptrtoint_Word(desiredPtr._rawValue))
+
+  if Bool(won) { return unmanaged }
+
+  // Some other thread initialized the value before us. Balance the retain that
+  // we performed on 'desired', and return what we loaded.
+  unmanaged.release()
+  let ptr = UnsafeRawPointer(Builtin.inttoptr_Word(value))
+  return Unmanaged<T>.fromOpaque(ptr)
+}
+
+@_alwaysEmitIntoClient
+@_transparent
+public func _stdlib_atomicAcquiringLoadARCRef<T: AnyObject>(
+  object target: UnsafeMutablePointer<T?>
+) -> Unmanaged<T>? {
+  let value = Builtin.atomicload_acquire_Word(target._rawValue)
+  if Int(value) == 0 { return nil }
+  let opaque = UnsafeRawPointer(Builtin.inttoptr_Word(value))
+  return Unmanaged<T>.fromOpaque(opaque)
+}
+
 //===----------------------------------------------------------------------===//
 // Conversion of primitive types to `String`
 //===----------------------------------------------------------------------===//

stdlib/public/core/StringStorage.swift

@@ -719,8 +719,18 @@ extension __SharedStringStorage {
 
 // Get and populate breadcrumbs
 extension _StringGuts {
+  /// Atomically load and return breadcrumbs, populating them if necessary.
+  ///
+  /// This emits aquire/release barriers to avoid access reordering trouble.
+  ///
+  /// This returns an unmanaged +0 reference to allow accessing breadcrumbs
+  /// without incurring retain/release operations.
   @_effects(releasenone)
-  internal func getBreadcrumbsPtr() -> UnsafePointer<_StringBreadcrumbs> {
+  internal func loadUnmanagedBreadcrumbs() -> Unmanaged<_StringBreadcrumbs> {
+    // FIXME: Returning Unmanaged emulates the original implementation (that
+    // used to return a nonatomic pointer), but it may be an unnecessary
+    // complication. (UTF-16 transcoding seems expensive enough not to worry
+    // about retain overhead.)
     _internalInvariant(hasBreadcrumbs)
 
     let mutPtr: UnsafeMutablePointer<_StringBreadcrumbs?>
@@ -731,25 +741,12 @@ extension _StringGuts {
         Builtin.addressof(&_object.sharedStorage._breadcrumbs))
     }
 
-    if _slowPath(mutPtr.pointee == nil) {
-      populateBreadcrumbs(mutPtr)
+    if let breadcrumbs = _stdlib_atomicAcquiringLoadARCRef(object: mutPtr) {
+      return breadcrumbs
     }
-
-    _internalInvariant(mutPtr.pointee != nil)
-    // assuming optional class reference and class reference can alias
-    return UnsafeRawPointer(mutPtr).assumingMemoryBound(to: _StringBreadcrumbs.self)
-  }
-
-  @inline(never) // slow-path
-  @_effects(releasenone)
-  internal func populateBreadcrumbs(
-    _ mutPtr: UnsafeMutablePointer<_StringBreadcrumbs?>
-  ) {
-    // Thread-safe compare-and-swap
-    let crumbs = _StringBreadcrumbs(String(self))
-    _stdlib_atomicInitializeARCRef(
-      object: UnsafeMutableRawPointer(mutPtr).assumingMemoryBound(to: Optional<AnyObject>.self),
-      desired: crumbs)
+    let desired = _StringBreadcrumbs(String(self))
+    return _stdlib_atomicAcquiringInitializeARCRef(
+      object: mutPtr, desired: desired)
   }
 }
 

stdlib/public/core/StringStorageBridge.swift

@@ -48,6 +48,9 @@ extension _AbstractStringStorage {
   ) {
     _precondition(aRange.location >= 0 && aRange.length >= 0,
                   "Range out of bounds")
+    // Note: `count` is counting UTF-8 code units, while `aRange` is measured in
+    // UTF-16 offsets. This precondition is a necessary, but not sufficient test
+    // for validity. (More precise checks are done in UTF16View._nativeCopy.)
     _precondition(aRange.location + aRange.length <= Int(count),
                   "Range out of bounds")
 

stdlib/public/core/StringUTF16View.swift

@@ -852,16 +852,20 @@ extension String.UTF16View {
       return _utf16Distance(from: startIndex, to: idx)
     }
 
+    let breadcrumbs = _guts.loadUnmanagedBreadcrumbs()
+
     // Simple and common: endIndex aka `length`.
-    let breadcrumbsPtr = _guts.getBreadcrumbsPtr()
-    if idx == endIndex { return breadcrumbsPtr.pointee.utf16Length }
+    if idx == endIndex {
+      return breadcrumbs._withUnsafeGuaranteedRef { $0.utf16Length }
+    }
 
-    // Otherwise, find the nearest lower-bound breadcrumb and count from there
-    // FIXME: Starting from the upper-bound crumb when that is closer would cut
-    // the average cost of the subsequent iteration by 50%.
-    let (crumb, crumbOffset) = breadcrumbsPtr.pointee.getBreadcrumb(
-      forIndex: idx)
-    return crumbOffset + _utf16Distance(from: crumb, to: idx)
+    return breadcrumbs._withUnsafeGuaranteedRef { crumbs in
+      // Otherwise, find the nearest lower-bound breadcrumb and count from there
+      // FIXME: Starting from the upper-bound crumb when that is closer would
+      // cut the average cost of the subsequent iteration by 50%.
+      let (crumb, crumbOffset) = crumbs.getBreadcrumb(forIndex: idx)
+      return crumbOffset + _utf16Distance(from: crumb, to: idx)
+    }
   }
 
   /// Return the index at the given UTF-16 offset, measured from the
@@ -892,14 +896,17 @@ extension String.UTF16View {
     }
 
     // Simple and common: endIndex aka `length`.
-    let breadcrumbsPtr = _guts.getBreadcrumbsPtr()
-    if offset == breadcrumbsPtr.pointee.utf16Length { return endIndex }
+    let breadcrumbs = _guts.loadUnmanagedBreadcrumbs()
+    let utf16Count = breadcrumbs._withUnsafeGuaranteedRef { $0.utf16Length }
+    if offset == utf16Count { return endIndex }
 
     // Otherwise, find the nearest lower-bound breadcrumb and advance that
     // FIXME: Starting from the upper-bound crumb when that is closer would cut
     // the average cost of the subsequent iteration by 50%.
-    let (crumb, remaining) = breadcrumbsPtr.pointee.getBreadcrumb(
-      forOffset: offset)
+    let (crumb, remaining) = breadcrumbs._withUnsafeGuaranteedRef {
+      $0.getBreadcrumb(forOffset: offset)
+    }
+    _internalInvariant(crumb._canBeUTF8 && crumb._encodedOffset <= _guts.count)
     if remaining == 0 { return crumb }
 
     return _guts.withFastUTF8 { utf8 in
@@ -981,6 +988,8 @@ extension String.UTF16View {
       if _slowPath(range.lowerBound.transcodedOffset != 0) {
         _internalInvariant(range.lowerBound.transcodedOffset == 1)
         let (scalar, len) = _decodeScalar(utf8, startingAt: readIdx)
+        // Note: this is intentionally not using the _unchecked subscript.
+        // (We rely on debug assertions to catch out of bounds access.)
         buffer[writeIdx] = scalar.utf16[1]
         readIdx &+= len
         writeIdx &+= 1
@@ -993,6 +1002,8 @@ extension String.UTF16View {
         readIdx &+= len
         writeIdx &+= 1
         if _slowPath(scalar.utf16.count == 2) {
+          // Note: this is intentionally not using the _unchecked subscript.
+          // (We rely on debug assertions to catch out of bounds access.)
           buffer[writeIdx] = scalar.utf16[1]
           writeIdx &+= 1
         }
@@ -1004,6 +1015,8 @@ extension String.UTF16View {
         let (scalar, _) = _decodeScalar(utf8, startingAt: readIdx)
         _internalInvariant(scalar.utf16.count == 2)
 
+        // Note: this is intentionally not using the _unchecked subscript.
+        // (We rely on debug assertions to catch out of bounds access.)
         buffer[writeIdx] = scalar.utf16[0]
         writeIdx &+= 1
       }

validation-test/StdlibUnittest/AtomicInt.swift

@@ -795,6 +795,103 @@ struct AtomicInitializeARCRefRaceTest : RaceTestWithPerTrialData {
   }
 }
 
+struct AtomicAcquiringARCRefRaceTest: RaceTestWithPerTrialData {
+  typealias DummyObject = AtomicInitializeARCRefRaceTest.DummyObject
+
+  class RaceData {
+    var _atomicReference: DummyObject? = nil
+
+    var atomicReferencePtr: UnsafeMutablePointer<DummyObject?> {
+      _getUnsafePointerToStoredProperties(self).assumingMemoryBound(
+        to: Optional<DummyObject>.self)
+    }
+
+    init() {}
+  }
+
+  typealias ThreadLocalData = _stdlib_ShardedAtomicCounter.PRNG
+  typealias Observation = Observation4UInt
+
+  func makeRaceData() -> RaceData {
+    RaceData()
+  }
+
+  func makeThreadLocalData() -> ThreadLocalData {
+    ThreadLocalData()
+  }
+
+  func thread1(
+    _ raceData: RaceData, _ threadLocalData: inout ThreadLocalData
+  ) -> Observation {
+    var observation = Observation4UInt(0, 0, 0, 0)
+    let initializerDestroyed = HeapBool(false)
+    do {
+      let object = DummyObject(
+        destroyedFlag: initializerDestroyed,
+        randomInt: threadLocalData.randomInt())
+      let value = Unmanaged.passUnretained(object)
+      let result = _stdlib_atomicAcquiringInitializeARCRef(
+        object: raceData.atomicReferencePtr, desired: object)
+      observation.data1 = (result.toOpaque() == value.toOpaque()  ? 1 : 0)
+      if let loaded =
+        _stdlib_atomicAcquiringLoadARCRef(object: raceData.atomicReferencePtr) {
+        observation.data2 = UInt(bitPattern: loaded.toOpaque())
+        observation.data3 = loaded._withUnsafeGuaranteedRef { $0.payload }
+      }
+    }
+    observation.data4 = initializerDestroyed.value ? 1 : 0
+    return observation
+  }
+
+  func evaluateObservations(
+    _ observations: [Observation],
+    _ sink: (RaceTestObservationEvaluation) -> Void
+  ) {
+    let ref = observations[0].data2
+    if observations.contains(where: { $0.data2 != ref }) {
+      for observation in observations {
+        sink(.failureInteresting("mismatched reference, expected \(ref): \(observation)"))
+      }
+      return
+    }
+    if observations.contains(where: { $0.data3 != 0x12345678 }) {
+      for observation in observations {
+        sink(.failureInteresting("wrong data: \(observation)"))
+      }
+      return
+    }
+
+    var wonRace = 0
+    var lostRace = 0
+    for observation in observations {
+      switch (observation.data1, observation.data4) {
+      case (1, 0):
+        // Won race, value not destroyed.
+        wonRace += 1
+      case (0, 1):
+        // Lost race, value destroyed.
+        lostRace += 1
+      default:
+        sink(.failureInteresting(String(describing: observation)))
+      }
+    }
+    if wonRace != 1 {
+      for observation in observations {
+        sink(.failureInteresting("zero or more than one thread won race: \(observation)"))
+      }
+      return
+    }
+    if lostRace < 1 {
+      for observation in observations {
+        sink(.failureInteresting("no thread lost race: \(observation)"))
+      }
+      return
+    }
+
+    sink(.pass)
+  }
+}
+
 var AtomicIntTestSuite = TestSuite("AtomicInt")
 
 AtomicIntTestSuite.test("fetchAndAdd/1") {
@@ -841,11 +938,16 @@ AtomicIntTestSuite.test("fetchAndXor/1") {
 
 var AtomicARCRefTestSuite = TestSuite("AtomicARCRef")
 
-AtomicARCRefTestSuite.test("initialize,load") {
+AtomicARCRefTestSuite.test("seqcst_initialize,load") {
   runRaceTest(AtomicInitializeARCRefRaceTest.self,
     operations: 25600, timeoutInSeconds: 60)
   expectEqual(0, dummyObjectCount.getSum())
 }
 
-runAllTests()
+AtomicARCRefTestSuite.test("acquire_initialize,load") {
+  runRaceTest(AtomicAcquiringARCRefRaceTest.self,
+    operations: 25600, timeoutInSeconds: 60)
+  expectEqual(0, dummyObjectCount.getSum())
+}
 
+runAllTests()