Skip to content

Commit

Permalink
[Concurrency] Nest stack traffic in withValue.
Browse files Browse the repository at this point in the history
Because `_taskLocalValuePush` and `_taskLocalValuePop` can result in calls
to `swift_task_alloc` and `swift_task_dealloc` respectively, and because
the compiler hasn't been taught about that (e.g.
`SILInstruction::isAllocatingStack`,
`SILInstruction::isDeallocatingStack`, etc), calling them (push and pop)
from a function which makes use the stack for dynamically sized
allocations can result in violations of stack discipline of the form

```
swift_task_alloc // allocates %ptr_1
copy_value_witness // copies into %ptr_1
swift_task_localValuePush // calls swift_task_alloc and allocates %ptr_2
swift_task_dealloc // deallocates %ptr_1
swift_task_localValuePop // calls swift_task_dealloc and deallocates %ptr_2
```

Avoid the problem by not allocating dynamically sized stack space in the
function which calls `_taskLocalValuePush` and `_taskLocalValuePop`.
Split the calls to those functions into `withValueImpl` function which
takes its argument `__owned`.  Call that function from `withValue`,
ensuring that the necessary copy (to account for the fact that withValue
takes its argument `__guaranteed` but `_taskLocalValuePush` takes its
`__owned`) and associated stack traffic occur in `withValue`.

Still, allow `withValueImpl` to be inlined.  The stack nesting will be
preserved across it.

rdar://107275872
  • Loading branch information
nate-chandler committed Apr 19, 2023
1 parent 4e14288 commit 4fe988b
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 18 deletions.
41 changes: 32 additions & 9 deletions stdlib/public/BackDeployConcurrency/TaskLocal.swift
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,41 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
@_backDeploy(before: SwiftStdlib 5.8)
public func withValue<R>(_ valueDuringOperation: Value, operation: () async throws -> R,
file: String = #file, line: UInt = #line) async rethrows -> R {
return try await withValueImpl(valueDuringOperation, operation: operation, file: file, line: line)
}

/// Implementation for withValue that consumes valueDuringOperation.
///
/// Because _taskLocalValuePush and _taskLocalValuePop involve calls to
/// swift_task_alloc/swift_task_dealloc respectively unbeknownst to the
/// compiler, compiler-emitted calls to swift_task_de/alloc must be avoided
/// in a function that calls them.
///
/// A copy of valueDuringOperation is required because withValue borrows its
/// argument but _taskLocalValuePush consumes its. Because
/// valueDuringOperation is of generic type, its size is not generally known,
/// so such a copy entails a stack allocation and a copy to that allocation.
/// That stack traffic gets lowered to calls to
/// swift_task_alloc/swift_task_deallloc.
///
/// Split the calls _taskLocalValuePush/Pop from the compiler-emitted calls
/// to swift_task_de/alloc for the copy as follows:
/// - withValue contains the compiler-emitted calls swift_task_de/alloc.
/// - withValueImpl contains the calls to _taskLocalValuePush/Pop
@inlinable
@discardableResult
@_unsafeInheritExecutor
@available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method
@_backDeploy(before: SwiftStdlib 5.9)
internal func withValueImpl<R>(_ valueDuringOperation: __owned Value, operation: () async throws -> R,
file: String = #fileID, line: UInt = #line) async rethrows -> R {
// check if we're not trying to bind a value from an illegal context; this may crash
_checkIllegalTaskLocalBindingWithinWithTaskGroup(file: file, line: line)

_taskLocalValuePush(key: key, value: valueDuringOperation)
do {
let result = try await operation()
_taskLocalValuePop()
return result
} catch {
_taskLocalValuePop()
throw error
}
_taskLocalValuePush(key: key, value: consume valueDuringOperation)
defer { _taskLocalValuePop() }

return try await operation()
}

/// Binds the task-local to the specific value for the duration of the
Expand Down
41 changes: 32 additions & 9 deletions stdlib/public/Concurrency/TaskLocal.swift
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,41 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
@_backDeploy(before: SwiftStdlib 5.8)
public func withValue<R>(_ valueDuringOperation: Value, operation: () async throws -> R,
file: String = #fileID, line: UInt = #line) async rethrows -> R {
return try await withValueImpl(valueDuringOperation, operation: operation, file: file, line: line)
}

/// Implementation for withValue that consumes valueDuringOperation.
///
/// Because _taskLocalValuePush and _taskLocalValuePop involve calls to
/// swift_task_alloc/swift_task_dealloc respectively unbeknownst to the
/// compiler, compiler-emitted calls to swift_task_de/alloc must be avoided
/// in a function that calls them.
///
/// A copy of valueDuringOperation is required because withValue borrows its
/// argument but _taskLocalValuePush consumes its. Because
/// valueDuringOperation is of generic type, its size is not generally known,
/// so such a copy entails a stack allocation and a copy to that allocation.
/// That stack traffic gets lowered to calls to
/// swift_task_alloc/swift_task_deallloc.
///
/// Split the calls _taskLocalValuePush/Pop from the compiler-emitted calls
/// to swift_task_de/alloc for the copy as follows:
/// - withValue contains the compiler-emitted calls swift_task_de/alloc.
/// - withValueImpl contains the calls to _taskLocalValuePush/Pop
@inlinable
@discardableResult
@_unsafeInheritExecutor
@available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method
@_backDeploy(before: SwiftStdlib 5.9)
internal func withValueImpl<R>(_ valueDuringOperation: __owned Value, operation: () async throws -> R,
file: String = #fileID, line: UInt = #line) async rethrows -> R {
// check if we're not trying to bind a value from an illegal context; this may crash
_checkIllegalTaskLocalBindingWithinWithTaskGroup(file: file, line: line)

_taskLocalValuePush(key: key, value: valueDuringOperation)
do {
let result = try await operation()
_taskLocalValuePop()
return result
} catch {
_taskLocalValuePop()
throw error
}
_taskLocalValuePush(key: key, value: consume valueDuringOperation)
defer { _taskLocalValuePop() }

return try await operation()
}

/// Binds the task-local to the specific value for the duration of the
Expand Down
28 changes: 28 additions & 0 deletions validation-test/Concurrency/rdar107275872.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift -O -Xfrontend -disable-availability-checking %s -parse-as-library -module-name main -o %t/main
// RUN: %target-codesign %t/main
// RUN: %target-run %t/main | %FileCheck %s

// REQUIRES: objc_interop
// REQUIRES: concurrency
// REQUIRES: executable_test
// REQUIRES: concurrency_runtime

import Foundation

@main struct M {
@TaskLocal static var v: UUID = UUID()
static func test(_ t: UUID) async {
await Self.$v.withValue(t) {
await Task.sleep(1)
print(Self.$v.get())
}
}
static func main() async {
// CHECK: before
print("before")
await test(UUID())
// CHECK: after
print("after")
}
}

0 comments on commit 4fe988b

Please sign in to comment.