Heap allocate our atomics

ahoppen · ahoppen · commit 204dd332c55b · 2024-06-05T21:30:27.000-07:00
We used C atomics but these were allocated as Swift variables. Even thought they were atomic, concurrent accesses to them could violate Swift’s exclusivity laws, raising thread sanitizer errors.

Allocate the C atomics using malloc to fix this problem.

rdar://129170128
diff --git a/Package.swift b/Package.swift
@@ -108,7 +108,6 @@ let package = Package(
     .target(
       name: "InProcessClient",
       dependencies: [
-        "CAtomics",
         "LanguageServerProtocol",
         "LSPLogging",
         "SKCore",
@@ -193,7 +192,6 @@ let package = Package(
     .target(
       name: "SemanticIndex",
       dependencies: [
-        "CAtomics",
         "LanguageServerProtocol",
         "LSPLogging",
         "SKCore",
@@ -218,7 +216,6 @@ let package = Package(
       name: "SKCore",
       dependencies: [
         "BuildServerProtocol",
-        "CAtomics",
         "LanguageServerProtocol",
         "LanguageServerProtocolJSONRPC",
         "LSPLogging",
@@ -247,10 +244,11 @@ let package = Package(
     .target(
       name: "SKSupport",
       dependencies: [
-        .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
+        "CAtomics",
         "LanguageServerProtocol",
         "LSPLogging",
         "SwiftExtensions",
+        .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"),
       ],
       exclude: ["CMakeLists.txt"],
       swiftSettings: [.enableExperimentalFeature("StrictConcurrency")]
@@ -350,7 +348,6 @@ let package = Package(
       name: "SourceKitLSP",
       dependencies: [
         "BuildServerProtocol",
-        "CAtomics",
         "LanguageServerProtocol",
         "LanguageServerProtocolJSONRPC",
         "LSPLogging",
@@ -378,7 +375,6 @@ let package = Package(
       name: "SourceKitLSPTests",
       dependencies: [
         "BuildServerProtocol",
-        "CAtomics",
         "LSPLogging",
         "LSPTestSupport",
         "LanguageServerProtocol",
diff --git a/Sources/CAtomics/include/CAtomics.h b/Sources/CAtomics/include/CAtomics.h
@@ -16,79 +16,32 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <sys/types.h>
-
-// MARK: - AtomicBool
-
-typedef struct {
-  _Atomic(bool) value;
-} AtomicBool;
-
-__attribute__((swift_name("AtomicBool.init(initialValue:)")))
-static inline AtomicBool atomic_bool_create(bool initialValue) {
-  AtomicBool atomic;
-  atomic.value = initialValue;
-  return atomic;
-}
-
-__attribute__((swift_name("getter:AtomicBool.value(self:)")))
-static inline bool atomic_bool_get(AtomicBool *atomic) {
-  return atomic->value;
-}
-
-__attribute__((swift_name("setter:AtomicBool.value(self:_:)")))
-static inline void atomic_bool_set(AtomicBool *atomic, bool newValue) {
-  atomic->value = newValue;
-}
-
-// MARK: - AtomicUInt8
+#include <stdlib.h>
 
 typedef struct {
-  _Atomic(uint8_t) value;
-} AtomicUInt8;
+  _Atomic(uint32_t) value;
+} CAtomicUInt32;
 
-__attribute__((swift_name("AtomicUInt8.init(initialValue:)")))
-static inline AtomicUInt8 atomic_uint8_create(uint8_t initialValue) {
-  AtomicUInt8 atomic;
-  atomic.value = initialValue;
+static inline CAtomicUInt32 *_Nonnull atomic_uint32_create(uint32_t initialValue) {
+  CAtomicUInt32 *atomic = malloc(sizeof(CAtomicUInt32));
+  atomic->value = initialValue;
   return atomic;
 }
 
-__attribute__((swift_name("getter:AtomicUInt8.value(self:)")))
-static inline uint8_t atomic_uint8_get(AtomicUInt8 *atomic) {
+static inline uint32_t atomic_uint32_get(CAtomicUInt32 *_Nonnull atomic) {
   return atomic->value;
 }
 
-__attribute__((swift_name("setter:AtomicUInt8.value(self:_:)")))
-static inline void atomic_uint8_set(AtomicUInt8 *atomic, uint8_t newValue) {
+static inline void atomic_uint32_set(CAtomicUInt32 *_Nonnull atomic, uint32_t newValue) {
   atomic->value = newValue;
 }
 
-// MARK: AtomicInt
-
-typedef struct {
-  _Atomic(int) value;
-} AtomicUInt32;
-
-__attribute__((swift_name("AtomicUInt32.init(initialValue:)")))
-static inline AtomicUInt32 atomic_int_create(uint32_t initialValue) {
-  AtomicUInt32 atomic;
-  atomic.value = initialValue;
-  return atomic;
-}
-
-__attribute__((swift_name("getter:AtomicUInt32.value(self:)")))
-static inline uint32_t atomic_int_get(AtomicUInt32 *atomic) {
-  return atomic->value;
-}
-
-__attribute__((swift_name("setter:AtomicUInt32.value(self:_:)")))
-static inline void atomic_uint32_set(AtomicUInt32 *atomic, uint32_t newValue) {
-  atomic->value = newValue;
+static inline uint32_t atomic_uint32_fetch_and_increment(CAtomicUInt32 *_Nonnull atomic) {
+  return atomic->value++;
 }
 
-__attribute__((swift_name("AtomicUInt32.fetchAndIncrement(self:)")))
-static inline uint32_t atomic_uint32_fetch_and_increment(AtomicUInt32 *atomic) {
-  return atomic->value++;
+static inline void atomic_uint32_destroy(CAtomicUInt32 *_Nonnull atomic) {
+  free(atomic);
 }
 
 #endif // SOURCEKITLSP_CATOMICS_H
diff --git a/Sources/InProcessClient/CMakeLists.txt b/Sources/InProcessClient/CMakeLists.txt
@@ -6,7 +6,6 @@ set_target_properties(InProcessClient PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 
 target_link_libraries(InProcessClient PUBLIC
-  CAtomics
   LanguageServerProtocol
   LSPLogging
   SKCore
diff --git a/Sources/InProcessClient/InProcessSourceKitLSPClient.swift b/Sources/InProcessClient/InProcessSourceKitLSPClient.swift
@@ -10,9 +10,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import LanguageServerProtocol
 import SKCore
+import SKSupport
 import SourceKitLSP
 
 /// Launches a `SourceKitLSPServer` in-process and allows sending messages to it.
diff --git a/Sources/SKCore/CMakeLists.txt b/Sources/SKCore/CMakeLists.txt
@@ -23,7 +23,6 @@ set_target_properties(SKCore PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 target_link_libraries(SKCore PUBLIC
   BuildServerProtocol
-  CAtomics
   LanguageServerProtocol
   LanguageServerProtocolJSONRPC
   LSPLogging
diff --git a/Sources/SKCore/TaskScheduler.swift b/Sources/SKCore/TaskScheduler.swift
@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import Foundation
 import LSPLogging
 import SKSupport
diff --git a/Sources/SKSupport/Atomics.swift b/Sources/SKSupport/Atomics.swift
@@ -0,0 +1,84 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import CAtomics
+
+#if compiler(>=6.2)
+#warning("We should be able to use atomics in the stdlib when we raise the deployment target to require Swift 6")
+#endif
+
+public class AtomicBool {
+  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+
+  public init(initialValue: Bool) {
+    self.atomic = atomic_uint32_create(initialValue ? 1 : 0)
+  }
+
+  deinit {
+    atomic_uint32_destroy(atomic)
+  }
+
+  public var value: Bool {
+    get {
+      atomic_uint32_get(atomic) != 0
+    }
+    set {
+      atomic_uint32_set(atomic, newValue ? 1 : 0)
+    }
+  }
+}
+
+public class AtomicUInt8 {
+  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+
+  public init(initialValue: UInt8) {
+    self.atomic = atomic_uint32_create(UInt32(initialValue))
+  }
+
+  deinit {
+    atomic_uint32_destroy(atomic)
+  }
+
+  public var value: UInt8 {
+    get {
+      UInt8(atomic_uint32_get(atomic))
+    }
+    set {
+      atomic_uint32_set(atomic, UInt32(newValue))
+    }
+  }
+}
+
+public class AtomicUInt32 {
+  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+
+  public init(initialValue: UInt32) {
+    self.atomic = atomic_uint32_create(initialValue)
+  }
+
+  public var value: UInt32 {
+    get {
+      atomic_uint32_get(atomic)
+    }
+    set {
+      atomic_uint32_set(atomic, newValue)
+    }
+  }
+
+  deinit {
+    atomic_uint32_destroy(atomic)
+  }
+
+  public func fetchAndIncrement() -> UInt32 {
+    return atomic_uint32_fetch_and_increment(atomic)
+  }
+}
diff --git a/Sources/SKSupport/CMakeLists.txt b/Sources/SKSupport/CMakeLists.txt
@@ -1,5 +1,6 @@
 
 add_library(SKSupport STATIC
+  Atomics.swift
   BuildConfiguration.swift
   ByteString.swift
   Connection+Send.swift
@@ -18,6 +19,7 @@ add_library(SKSupport STATIC
 set_target_properties(SKSupport PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 target_link_libraries(SKSupport PRIVATE
+  CAtomics
   LanguageServerProtocol
   LSPLogging
   SwiftExtensions
diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
@@ -13,7 +13,6 @@
 import Basics
 import Build
 import BuildServerProtocol
-import CAtomics
 import Dispatch
 import Foundation
 import LSPLogging
diff --git a/Sources/SKTestSupport/TestSourceKitLSPClient.swift b/Sources/SKTestSupport/TestSourceKitLSPClient.swift
@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import Foundation
 import InProcessClient
 import LSPTestSupport
diff --git a/Sources/SemanticIndex/CMakeLists.txt b/Sources/SemanticIndex/CMakeLists.txt
@@ -12,6 +12,7 @@ add_library(SemanticIndex STATIC
 set_target_properties(SemanticIndex PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY})
 target_link_libraries(SemanticIndex PRIVATE
+  LanguageServerProtocol
   LSPLogging
   SKCore
   SwiftExtensions
diff --git a/Sources/SemanticIndex/PreparationTaskDescription.swift b/Sources/SemanticIndex/PreparationTaskDescription.swift
@@ -10,11 +10,11 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import Foundation
 import LSPLogging
 import LanguageServerProtocol
 import SKCore
+import SKSupport
 
 import struct TSCBasic.AbsolutePath
 import class TSCBasic.Process
diff --git a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift
@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import Foundation
 import LSPLogging
 import LanguageServerProtocol
diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift
@@ -11,7 +11,6 @@
 //===----------------------------------------------------------------------===//
 
 import BuildServerProtocol
-import CAtomics
 import Dispatch
 import Foundation
 import IndexStoreDB
diff --git a/Tests/SourceKitDTests/SourceKitDRegistryTests.swift b/Tests/SourceKitDTests/SourceKitDRegistryTests.swift
@@ -10,8 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import LSPTestSupport
+import SKSupport
 import SourceKitD
 import TSCBasic
 import XCTest
diff --git a/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift b/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift
@@ -10,9 +10,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-import CAtomics
 import LSPTestSupport
 import LanguageServerProtocol
+import SKSupport
 import SKTestSupport
 import SourceKitLSP
 import XCTest
