Merge pull request #2440 from ahoppen/ahoppen/has-parent-atomic

Make `SyntaxArena.hasParent` an atomic boolean

Author: ahoppen

CMakeLists.txt

@@ -88,5 +88,9 @@ if (SWIFTSYNTAX_ENABLE_ASSERTIONS)
   )
 endif()
 
+add_compile_definitions(
+  $<$<COMPILE_LANGUAGE:Swift>:SWIFT_SYNTAX_BUILD_USING_CMAKE>
+)
+
 add_subdirectory(Sources)
 add_subdirectory(cmake/modules)

Package.swift

@@ -31,6 +31,10 @@ let package = Package(
   targets: [
     // MARK: - Internal helper targets
 
+    .target(
+      name: "_AtomicBool"
+    ),
+
     .target(
       name: "_InstructionCounter"
     ),
@@ -116,7 +120,7 @@ let package = Package(
 
     .target(
       name: "SwiftSyntax",
-      dependencies: ["SwiftSyntax509", "SwiftSyntax510", "SwiftSyntax511"],
+      dependencies: ["_AtomicBool", "SwiftSyntax509", "SwiftSyntax510", "SwiftSyntax511"],
       exclude: ["CMakeLists.txt"],
       swiftSettings: swiftSyntaxSwiftSettings
     ),

Sources/SwiftSyntax/SyntaxArena.swift

@@ -10,6 +10,23 @@
 //
 //===----------------------------------------------------------------------===//
 
+#if SWIFT_SYNTAX_BUILD_USING_CMAKE
+// The CMake bulid of swift-syntax does not build the _AtomicBool module because swift-syntax's CMake build is
+// Swift-only. Fake an `AtomicBool` type that is not actually atomic. This should be acceptable for the following
+// reasons:
+//  - `AtomicBool` is only used for the `hasParent` assertion, so release compilers don't rely on it
+//  - The compiler is single-threaded so it it is safe from race conditions on `AtomicBool`.
+fileprivate struct AtomicBool {
+  var value: Bool
+
+  init(initialValue: Bool) {
+    self.value = initialValue
+  }
+}
+#else
+import _AtomicBool
+#endif
+
 /// A syntax arena owns the memory for all syntax nodes within it.
 ///
 /// The following is only relevant if you are accessing the raw syntax tree using
@@ -52,7 +69,7 @@ public class SyntaxArena {
   ///
   /// - Important: This is only intended to be used for assertions to catch
   ///   retain cycles in syntax arenas.
-  var hasParent: Bool
+  fileprivate var hasParent: AtomicBool
   #endif
 
   /// Construct a new ``SyntaxArena`` in which syntax nodes can be allocated.
@@ -64,7 +81,7 @@ public class SyntaxArena {
     self.allocator = BumpPtrAllocator(initialSlabSize: slabSize)
     self.childRefs = []
     #if DEBUG || SWIFTSYNTAX_ENABLE_ASSERTIONS
-    self.hasParent = false
+    self.hasParent = AtomicBool(initialValue: false)
     #endif
   }
 
@@ -141,7 +158,7 @@ public class SyntaxArena {
 
     #if DEBUG || SWIFTSYNTAX_ENABLE_ASSERTIONS
     precondition(
-      !self.hasParent,
+      !self.hasParent.value,
       "an arena can't have a new child once it's owned by other arenas"
     )
     #endif
@@ -285,16 +302,12 @@ struct SyntaxArenaRef: Hashable, @unchecked Sendable {
   #if DEBUG || SWIFTSYNTAX_ENABLE_ASSERTIONS
   /// Accessor for ther underlying's `SyntaxArena.hasParent`
   var hasParent: Bool {
-    // FIXME: This accesses mutable state across actor boundaries and is thus not concurrency-safe.
-    // We should use an atomic for `hasParent` to make it concurrency safe.
-    value.hasParent
+    value.hasParent.value
   }
 
   /// Sets the `SyntaxArena.hasParent` on the referenced arena.
   func setHasParent(_ newValue: Bool) {
-    // FIXME: This modifies mutable state across actor boundaries and is thus not concurrency-safe.
-    // We should use an atomic for `hasParent` to make it concurrency safe.
-    value.hasParent = newValue
+    value.hasParent.value = newValue
   }
   #endif
 

Sources/_AtomicBool/include/AtomicBool.h

@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 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
+//
+//===----------------------------------------------------------------------===//
+
+#include <stdbool.h>
+
+typedef struct {
+  _Atomic(bool) value;
+} AtomicBool;
+
+__attribute__((swift_name("AtomicBool.init(initialValue:)")))
+AtomicBool atomic_bool_create(bool initialValue);
+
+__attribute__((swift_name("getter:AtomicBool.value(self:)")))
+bool atomic_bool_get(AtomicBool *atomic);
+
+__attribute__((swift_name("setter:AtomicBool.value(self:_:)")))
+void atomic_bool_set(AtomicBool *atomic, bool newValue);

Sources/_AtomicBool/src/AtomicBool.c

@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "AtomicBool.h"
+
+AtomicBool atomic_bool_create(bool initialValue) {
+  AtomicBool atomic;
+  atomic.value = initialValue;
+  return atomic;
+}
+
+bool atomic_bool_get(AtomicBool *atomic) {
+  return atomic->value;
+}
+
+void atomic_bool_set(AtomicBool *atomic, bool newValue) {
+  atomic->value = newValue;
+}