Merge pull request swiftlang#857 from CodaFi/a-critique-of-pure-reason

SyntaxArena: thread safety part 2

Author: CodaFi

Sources/SwiftParser/Parser.swift

@@ -42,8 +42,10 @@ extension Parser {
     // Extended lifetime is required because `SyntaxArena` in the parser must
     // be alive until `Syntax(raw:)` retains the arena.
     return withExtendedLifetime(parser) {
-      let rawSourceFile =  parser.parseSourceFile()
-      return rawSourceFile.syntax
+      parser.arena.assumingSingleThread {
+        let rawSourceFile =  parser.parseSourceFile()
+        return rawSourceFile.syntax
+      }
     }
   }
 }
@@ -120,7 +122,8 @@ extension Parser {
 /// tokens as needed to disambiguate a parse. However, because lookahead
 /// operates on a copy of the lexical stream, no input tokens are lost..
 public struct Parser: TokenConsumer {
-  let arena: SyntaxArena
+  @_spi(RawSyntax)
+  public var arena: SyntaxArena
   /// A view of the sequence of lexemes in the input.
   var lexemes: Lexer.LexemeSequence
   /// The current token. If there was no input, this token will have a kind of `.eof`.

Sources/SwiftSyntax/CMakeLists.txt

@@ -10,6 +10,7 @@ add_library(SwiftSyntax STATIC
   AbsolutePosition.swift
   BumpPtrAllocator.swift
   IncrementalParseTransition.swift
+  PlatformMutex.swift
   SourceLength.swift
   SourceLocation.swift
   SourcePresence.swift

Sources/SwiftSyntax/PlatformMutex.swift

@@ -0,0 +1,100 @@
+//===------ PlatformMutex.swift - Platform-specific Mutual Exclusion  -----===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2022 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
+//
+//===----------------------------------------------------------------------===//
+
+#if canImport(Darwin)
+@_implementationOnly import Darwin
+#elseif canImport(Glibc)
+@_implementationOnly import Glibc
+#elseif canImport(WinSDK)
+@_implementationOnly import WinSDK
+#endif
+
+/// A protocol that platform-specific mutual exclusion primitives should conform to.
+final class PlatformMutex {
+  // FIXME: Use os_unfair_lock when we bump to macOS 12.0 on Darwin
+#if canImport(Darwin) || canImport(Glibc)
+  typealias Primitive = pthread_mutex_t
+#elseif canImport(WinSDK)
+  typealias Primitive = SRWLOCK
+#endif
+  typealias PlatformLock = UnsafeMutablePointer<Primitive>
+
+  private let lock: PlatformLock
+
+  /// Allocate memory for, and initialize, the mutex in a platform-specific fashion.
+  ///
+  /// - Parameter allocator: An allocator
+  init(allocator: BumpPtrAllocator) {
+    let storage = allocator.allocate(Primitive.self, count: 1).baseAddress!
+    storage.initialize(to: Primitive())
+    self.lock = storage
+    Self.initialize(self.lock)
+  }
+
+  /// Deinitialize the memory associated with the mutex.
+  ///
+  /// - Warning: Do not call `.deallocate()` on  any pointers allocated by the
+  ///            `BumpPtrAllocator` here.
+  func deinitialize() {
+#if canImport(Darwin) || canImport(Glibc)
+    pthread_mutex_destroy(self.lock)
+#endif
+    self.lock.deinitialize(count: 1)
+  }
+
+  /// Invoke the provided block under the protection of the mutex.
+  func withGuard<T>(body: () throws -> T) rethrows -> T {
+    Self.lock(self.lock)
+    defer { Self.unlock(self.lock) }
+    return try body()
+  }
+}
+
+extension PlatformMutex {
+  private static func initialize(_ platformLock: PlatformLock) {
+#if canImport(Darwin)
+    // HACK: On Darwin, the value of PTHREAD_MUTEX_INITIALIZER installs
+    // signature bits into the mutex that are later checked by other aspects
+    // of the mutex API. This is a completely optional POSIX-ism that most
+    // Linuxes don't implement - often so that (global) lock variables can be
+    // stuck in .bss. Swift doesn't know how to import
+    // PTHREAD_MUTEX_INITIALIZER, so we'll replicate its signature-installing
+    // magic with the bit it can import.
+    platformLock.pointee.__sig = Int(_PTHREAD_MUTEX_SIG_init)
+    let result = pthread_mutex_init(platformLock, nil)
+    precondition(result == 0)
+#elseif canImport(Glibc)
+    let result = pthread_mutex_init(platformLock, nil)
+    precondition(result == 0)
+#elseif canImport(WinSDK)
+    InitializeSRWLock(platformLock)
+#endif
+  }
+
+  private static func lock(_ platformLock: PlatformLock) {
+#if canImport(Darwin) || canImport(Glibc)
+    let result = pthread_mutex_lock(platformLock)
+    assert(result == 0)
+#elseif canImport(WinSDK)
+    AcquireSRWLockExclusive(platformLock)
+#endif
+  }
+
+  private static func unlock(_ platformLock: PlatformLock) {
+#if canImport(Darwin) || canImport(Glibc)
+    let result = pthread_mutex_unlock(platformLock)
+    assert(result == 0)
+#elseif canImport(WinSDK)
+    ReleaseSRWLockExclusive(platformLock)
+#endif
+  }
+}

Sources/SwiftSyntax/SyntaxArena.swift

@@ -28,28 +28,57 @@ public class SyntaxArena {
   private var hasParent: Bool
   private var parseTriviaFunction: ParseTriviaFunction
 
+  /// Thread safe guard.
+  private let lock: PlatformMutex
+  private var singleThreadMode: Bool
+
   @_spi(RawSyntax)
   public init(parseTriviaFunction: @escaping ParseTriviaFunction) {
-    allocator = BumpPtrAllocator()
-    children = []
-    sourceBuffer = .init(start: nil, count: 0)
-    hasParent = false
+    self.allocator = BumpPtrAllocator()
+    self.lock = PlatformMutex(allocator: self.allocator)
+    self.singleThreadMode = false
+    self.children = []
+    self.sourceBuffer = .init(start: nil, count: 0)
+    self.hasParent = false
     self.parseTriviaFunction = parseTriviaFunction
   }
 
+  deinit {
+    // Make sure we give the platform lock a chance to deinitialize any
+    // memory it used.
+    lock.deinitialize()
+  }
+
   public convenience init() {
     self.init(parseTriviaFunction: _defaultParseTriviaFunction(_:_:))
   }
 
+  private func withGuard<R>(_ body: () throws -> R) rethrows -> R {
+    if self.singleThreadMode {
+      return try body()
+    } else {
+      return try self.lock.withGuard(body: body)
+    }
+  }
+
+  public func assumingSingleThread<R>(body: () throws -> R) rethrows -> R {
+    let oldValue = self.singleThreadMode
+    defer { self.singleThreadMode = oldValue }
+    self.singleThreadMode = true
+    return try body()
+  }
+
   /// Copies a source buffer in to the memory this arena manages, and returns
   /// the interned buffer.
   ///
   /// The interned buffer is guaranteed to be null-terminated.
   /// `contains(address _:)` is faster if the address is inside the memory
   /// range this function returned.
   public func internSourceBuffer(_ buffer: UnsafeBufferPointer<UInt8>) -> UnsafeBufferPointer<UInt8> {
+    let allocated = self.withGuard {
+      allocator.allocate(UInt8.self, count: buffer.count + /* for NULL */1)
+    }
     precondition(sourceBuffer.baseAddress == nil, "SourceBuffer should only be set once.")
-    let allocated = allocator.allocate(UInt8.self, count: buffer.count + /* for NULL */1)
     _ = allocated.initialize(from: buffer)
 
     // NULL terminate.
@@ -69,20 +98,27 @@ public class SyntaxArena {
   /// Allocates a buffer of `RawSyntax?` with the given count, then returns the
   /// uninitlialized memory range as a `UnsafeMutableBufferPointer<RawSyntax?>`.
   func allocateRawSyntaxBuffer(count: Int) -> UnsafeMutableBufferPointer<RawSyntax?> {
-    return allocator.allocate(RawSyntax?.self, count: count)
+    return self.withGuard {
+      allocator.allocate(RawSyntax?.self, count: count)
+    }
   }
 
   /// Allcates a buffer of `RawTriviaPiece` with the given count, then returns
   /// the uninitialized memory range as a `UnsafeMutableBufferPointer<RawTriviaPiece>`.
   func allocateRawTriviaPieceBuffer(
-    count: Int) -> UnsafeMutableBufferPointer<RawTriviaPiece> {
-      return allocator.allocate(RawTriviaPiece.self, count: count)
+    count: Int
+  ) -> UnsafeMutableBufferPointer<RawTriviaPiece> {
+    return self.withGuard {
+      allocator.allocate(RawTriviaPiece.self, count: count)
     }
+  }
 
   /// Allcates a buffer of `UInt8` with the given count, then returns the
   /// uninitialized memory range as a `UnsafeMutableBufferPointer<UInt8>`.
   func allocateTextBuffer(count: Int) -> UnsafeMutableBufferPointer<UInt8> {
-    return allocator.allocate(UInt8.self, count: count)
+    return self.withGuard {
+      allocator.allocate(UInt8.self, count: count)
+    }
   }
 
   /// Copies the contents of a `SyntaxText` to the memory this arena manages,
@@ -114,7 +150,9 @@ public class SyntaxArena {
   /// Copies a `RawSyntaxData` to the memory this arena manages, and retuns the
   /// pointer to the destination.
   func intern(_ value: RawSyntaxData) -> UnsafePointer<RawSyntaxData> {
-    let allocated = allocator.allocate(RawSyntaxData.self, count: 1).baseAddress!
+    let allocated = self.withGuard {
+      allocator.allocate(RawSyntaxData.self, count: 1).baseAddress!
+    }
     allocated.initialize(to: value)
     return UnsafePointer(allocated)
   }
@@ -128,21 +166,26 @@ public class SyntaxArena {
   /// See also `RawSyntax.layout()`.
   func addChild(_ arenaRef: SyntaxArenaRef) {
     if SyntaxArenaRef(self) == arenaRef { return }
-
     let other = arenaRef.value
 
-    precondition(
-      !self.hasParent,
-      "an arena can't have a new child once it's owned by other arenas")
-    
-    other.hasParent = true
-    children.insert(other)
+    other.withGuard {
+      self.withGuard {
+        precondition(
+          !self.hasParent,
+          "an arena can't have a new child once it's owned by other arenas")
+
+        other.hasParent = true
+        children.insert(other)
+      }
+    }
   }
 
   /// Recursively checks if this arena contains given `arena` as a descendant.
   func contains(arena: SyntaxArena) -> Bool {
-    return children.contains { child in
-      child === arena  || child.contains(arena: arena)
+    self.withGuard {
+      children.contains { child in
+        child === arena  || child.contains(arena: arena)
+      }
     }
   }
 
@@ -154,7 +197,7 @@ public class SyntaxArena {
   public func contains(text: SyntaxText) -> Bool {
     return (text.isEmpty ||
             sourceBufferContains(text.baseAddress!) ||
-            allocator.contains(address: text.baseAddress!))
+            self.withGuard({allocator.contains(address: text.baseAddress!)}))
   }
 
   @_spi(RawSyntax)

Sources/SwiftSyntax/SyntaxText.swift

@@ -11,9 +11,9 @@
 //===----------------------------------------------------------------------===//
 
 #if canImport(Darwin)
-import Darwin
+@_implementationOnly import Darwin
 #elseif canImport(Glibc)
-import Glibc
+@_implementationOnly import Glibc
 #endif
 
 /// Represent a string.

Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift

@@ -1,6 +1,5 @@
-@_spi(RawSyntax)
-import SwiftSyntax
-import SwiftParser
+@_spi(RawSyntax) import SwiftSyntax
+@_spi(RawSyntax) import SwiftParser
 
 /// An individual interpolated syntax node.
 struct InterpolatedSyntaxNode {
@@ -101,7 +100,9 @@ extension SyntaxExpressibleByStringInterpolation {
       var parser = Parser(buffer)
       // FIXME: When the parser supports incremental parsing, put the
       // interpolatedSyntaxNodes in so we don't have to parse them again.
-      return Self.parse(from: &parser)
+      return parser.arena.assumingSingleThread {
+        return Self.parse(from: &parser)
+      }
     }
   }
 

Tests/SwiftParserTest/Linkage.swift

@@ -32,7 +32,6 @@ final class LinkageTest: XCTestCase {
       .library("-lswiftCompatibility56", condition: .mayBeAbsent("Starting in Xcode 14 this library is not always autolinked")),
       .library("-lswiftCompatibilityConcurrency"),
       .library("-lswiftCore"),
-      .library("-lswiftDarwin"),
       .library("-lswiftSwiftOnoneSupport", condition: .when(configuration: .debug)),
       .library("-lswift_Concurrency"),
       .library("-lswift_StringProcessing", condition: .mayBeAbsent("Starting in Xcode 14 this library is not always autolinked")),

Tests/SwiftSyntaxTest/MultithreadingTests.swift

@@ -1,5 +1,6 @@
 import XCTest
-import SwiftSyntax
+@_spi(RawSyntax) import SwiftSyntax
+
 
 public class MultithreadingTests: XCTestCase {
 
@@ -15,6 +16,29 @@ public class MultithreadingTests: XCTestCase {
     }
   }
 
+  public func testConcurrentArena() {
+    let arena = SyntaxArena()
+
+    DispatchQueue.concurrentPerform(iterations: 100) { i in
+      var identStr = " ident\(i) "
+      let tokenRaw = identStr.withSyntaxText { text in
+        RawTokenSyntax(
+          kind: .identifier,
+          wholeText: arena.intern(text),
+          textRange: 1..<(text.count-1),
+          presence: .present,
+          arena: arena)
+      }
+      let identifierExprRaw = RawIdentifierExprSyntax(
+        identifier: tokenRaw,
+        declNameArguments: nil,
+        arena: arena)
+
+      let expr = Syntax(raw: RawSyntax(identifierExprRaw)).as(IdentifierExprSyntax.self)!
+      XCTAssertEqual(expr.identifier.text, "ident\(i)")
+    }
+  }
+
   public func testTwoAccesses() {
     let tuple = TupleTypeSyntax(
       leftParen: .leftParenToken(),

utils/group.json

@@ -56,5 +56,6 @@
     "SyntaxArena.swift",
     "SyntaxVerifier.swift",
     "BumpPtrAllocator.swift",
+    "PlatformMutex.swift",
   ]
 }