[Concurrency] TaskLocal.withValue closure should produce no warnings inside Actor #62239

ktoso · 2022-11-24T05:10:03Z

Without the @sendable, using a task-local's withValue results with unexpected warnings which scared developers into thinking they might be misusing the API.

This PR adds the missing annotation; I believe this is ABI compatible, but would like to make sure.

Resolves #62220
rdar://102638772

ktoso · 2022-11-24T05:26:00Z

resolves rdar://102638772

ktoso · 2022-11-24T05:26:05Z

@swift-ci please smoke test

stdlib/public/BackDeployConcurrency/TaskLocal.swift

ktoso · 2022-11-24T07:52:34Z

@swift-ci please smoke test

ktoso · 2022-11-28T05:51:01Z

@swift-ci please smoke test

rjmccall · 2022-11-28T18:18:08Z

I think the right fix here is that this ought to inherit the caller's executor.

kavon · 2022-11-28T20:07:47Z

I believe this is ABI compatible, but would like to make sure.

Seems it is not ABI compatible to add @Sendable to a parameter. For example:

> swift demangle s4what8whatever2fnS3iXE_tYaF
$s4what8whatever2fnS3iXE_tYaF ---> what.whatever(fn: (Swift.Int) -> Swift.Int) async -> Swift.Int
> swift demangle s4what8whatever2fnS3iYbXE_tYaF
$s4what8whatever2fnS3iYbXE_tYaF ---> what.whatever(fn: @Sendable (Swift.Int) -> Swift.Int) async -> Swift.Int

ktoso · 2022-11-28T22:38:59Z

Ah the unsafe executor inheritance... I'll give that a shot.

[edit] Yup that'll work as expected. Thanks for chiming in @rjmccall, @kavon!

ktoso · 2022-11-28T23:02:55Z

@swift-ci please smoke test

ktoso · 2022-11-29T02:23:49Z

@swift-ci please smoke test

rjmccall · 2022-11-29T18:01:05Z

I think it's correct in the abstract to make this inherit the executor. However, marking the function as inheriting the executor makes a semantic guarantee that the function won't switch, and unfortunately the existing implementations of this function (ever since 5.6 / SE-0338) do switch (to the generic executor) (unless that switch has been optimized out in the release builds). I think it's okay for existing compilations to call a function that doesn't switch (the function doesn't rely on being on the generic executor, and you can think of turning off the switches as manually optimizing them away, which is permitted). However, it's not okay for new compilations that will rely on the absence of switching to potentially call a function that does switch. So we need to have a way to back-deploy this fix for it to be correct.

It looks like everything these functions call is back-deployable — they're either already @usableFromInline or they're runtime functions already exported to which we can safely add the attribute. So we need new compilations to call the new version of the function, but also emit a stable version of the function for existing users. @DougGregor points out that @backDeployed is exactly what we want for this — it's a lie only in that the function does already exist, but since we don't want to actually use the existing function, it's a pretty consequence-free lie. We can make the function @inlinable as well for performance.

You may have to wait until @backDeployed (SE-0376) lands in order to land this fix.

ktoso · 2022-11-30T02:38:37Z

Thank you for the analysis and guidance, John!

We'll need to wait for _backDeploy to be able to be combined with inlinable, but that's coming soon. I'll update the PR and wait for it :)

ktoso · 2022-11-30T02:54:39Z

Depends on #62309

ktoso · 2022-12-02T03:02:49Z

Hmmm, tried to make use of backDeploy bot getting into all kinds of crashes, I'll ask @tshortli for some assistance.

Doing

  @discardableResult
  @_unsafeInheritExecutor
  @available(SwiftStdlib 5.1, *)
  @_backDeploy(before: SwiftStdlib 5.8)
  public func withValue<R>(_ valueDuringOperation: 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)
    defer { _taskLocalValuePop() }

    return try await operation()
  }

we get:

/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskLocal.swift:149:5: error: multiple definitions of symbol '$ss9TaskLocalC9withValue_9operation4file4lineqd__x_qd__yYaKXESSSutYaKlF6$deferL_yys8SendableRzr__lF'
    defer { _taskLocalValuePop() }
    ^

and removing the defer we get another crash:

1.	Swift version 5.8-dev (LLVM ec11c492d2fb63b, Swift 9b82170c7e2af84)
2.	Compiling with the current language version
3.	While evaluating request ExecuteSILPipelineRequest(Run pipelines { Mandatory Diagnostic Passes + Enabling Optimization Passes } on SIL for _Concurrency)
4.	While running pass #8084 SILModuleTransform "SILSkippingChecker".
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  swift-frontend           0x00000001079faa14 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  swift-frontend           0x00000001079f9c08 llvm::sys::RunSignalHandlers() + 112
2  swift-frontend           0x00000001079fb054 SignalHandler(int) + 304
3  libsystem_platform.dylib 0x0000000193cb82a4 _sigtramp + 56
4  libsystem_pthread.dylib  0x0000000193c89cec pthread_kill + 288
5  libsystem_c.dylib        0x0000000193bc32c8 abort + 180
6  swift-frontend           0x0000000107cf1430 (anonymous namespace)::SILSkippingChecker::run() (.cold.4) + 0
7  swift-frontend           0x000000010376be14 (anonymous namespace)::SILSkippingChecker::run() + 432
8  swift-frontend           0x0000000103626e90 swift::SILPassManager::runModulePass(unsigned int) + 968
9  swift-frontend           0x000000010362cc04 swift::SILPassManager::execute() + 624
10 swift-frontend           0x0000000103623d2c swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 68
11 swift-frontend           0x0000000103623ccc swift::ExecuteSILPipelineRequest::evaluate(swift::Evaluator&, swift::SILPipelineExecutionDescriptor) const + 52
12 swift-frontend           0x00000001036524ec swift::SimpleRequest<swift::ExecuteSILPipelineRequest, std::__1::tuple<> (swift::SILPipelineExecutionDescriptor), (swift::RequestFlags)1>::evaluateRequest(swift::ExecuteSILPipelineRequest const&, swift::Evaluator&) + 28
13 swift-frontend           0x0000000103635d28 llvm::Expected<swift::ExecuteSILPipelineRequest::OutputType> swift::Evaluator::getResultUncached<swift::ExecuteSILPipelineRequest>(swift::ExecuteSILPipelineRequest const&) + 252
14 swift-frontend           0x0000000103623f10 swift::executePassPipelinePlan(swift::SILModule*, swift::SILPassPipelinePlan const&, bool, swift::irgen::IRGenModule*) + 68
15 swift-frontend           0x000000010363a174 swift::runSILDiagnosticPasses(swift::SILModule&) + 92
16 swift-frontend           0x0000000102fddbfc swift::CompilerInstance::performSILProcessing(swift::SILModule*) + 68
17 swift-frontend           0x0000000102e28728 performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*) + 672
18 swift-frontend           0x0000000102e28100 swift::performCompileStepsPostSema(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 1100
19 swift-frontend           0x0000000102e36908 withSemanticAnalysis(swift::CompilerInstance&, swift::FrontendObserver*, llvm::function_ref<bool (swift::CompilerInstance&)>, bool) + 160
20 swift-frontend           0x0000000102e29a54 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 2876
21 swift-frontend           0x0000000102df8354 swift::mainEntry(int, char const**) + 3440
22 dyld                     0x000000019395fe50 start + 2544

error: compile command failed due to signal 6 (use -v to see invocation)
Function serialized that should have been skipped!
/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskLocal.swift:143:15
// TaskLocal.withValue<A>(_:operation:file:line:)
sil [available 9999] [ossa] @$ss9TaskLocalC9withValue_9operation4file4lineqd__x_qd__yYaKXESSSutYaKlF : $@convention(method) @async <Value where Value : Sendable><R> (@in_guaranteed Value, @noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <R>, @guaranteed String, UInt, @guaranteed TaskLocal<Value>) -> (@out R, @error any Error) {
// %0 "$return_value"                             // user: %26
// %1 "valueDuringOperation"                      // users: %17, %6
// %2 "operation"                                 // users: %22, %7
// %3 "file"                                      // users: %13, %8
// %4 "line"                                      // users: %13, %9
// %5 "self"                                      // users: %15, %10
bb0(%0 : $*R, %1 : $*Value, %2 : $@noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <R>, %3 : @guaranteed $String, %4 : $UInt, %5 : @guaranteed $TaskLocal<Value>):
  debug_value %1 : $*Value, let, name "valueDuringOperation", argno 1, expr op_deref // id: %6
  debug_value %2 : $@noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <R>, let, name "operation", argno 2 // id: %7
  debug_value %3 : $String, let, name "file", argno 3 // id: %8
  debug_value %4 : $UInt, let, name "line", argno 4 // id: %9
  debug_value %5 : $TaskLocal<Value>, let, name "self", argno 5, implicit // id: %10
  debug_value undef : $any Error, var, name "$error", argno 6 // id: %11
  // function_ref _checkIllegalTaskLocalBindingWithinWithTaskGroup(file:line:)
  %12 = function_ref @$ss039_checkIllegalTaskLocalBindingWithinWithC5Group4file4lineySS_SutF : $@convention(thin) (@guaranteed String, UInt) -> () // user: %13
  %13 = apply %12(%3, %4) : $@convention(thin) (@guaranteed String, UInt) -> ()
  // function_ref TaskLocal.key.getter
  %14 = function_ref @$ss9TaskLocalC3keyBpvg : $@convention(method) <τ_0_0 where τ_0_0 : Sendable> (@guaranteed TaskLocal<τ_0_0>) -> Builtin.RawPointer // user: %15
  %15 = apply %14<Value>(%5) : $@convention(method) <τ_0_0 where τ_0_0 : Sendable> (@guaranteed TaskLocal<τ_0_0>) -> Builtin.RawPointer // user: %19
  %16 = alloc_stack $Value                        // users: %20, %19, %17
  copy_addr %1 to [init] %16 : $*Value            // id: %17
  // function_ref swift_task_localValuePush
  %18 = function_ref @swift_task_localValuePush : $@convention(thin) <τ_0_0> (Builtin.RawPointer, @in τ_0_0) -> () // user: %19
  %19 = apply %18<Value>(%15, %16) : $@convention(thin) <τ_0_0> (Builtin.RawPointer, @in τ_0_0) -> ()
  dealloc_stack %16 : $*Value                     // id: %20
  %21 = alloc_stack [lexical] $R, let, name "value" // users: %28, %27, %26, %22, %32
  try_apply %2(%21) : $@noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <R>, normal bb1, error bb2 // id: %22

bb1(%23 : $()):                                   // Preds: bb0
  // function_ref swift_task_localValuePop
  %24 = function_ref @swift_task_localValuePop : $@convention(thin) () -> () // user: %25
  %25 = apply %24() : $@convention(thin) () -> ()
  copy_addr %21 to [init] %0 : $*R                // id: %26
  destroy_addr %21 : $*R                          // id: %27
  dealloc_stack %21 : $*R                         // id: %28
  %29 = tuple ()                                  // user: %30
  return %29 : $()                                // id: %30

// %31                                            // users: %33, %42
bb2(%31 : @owned $any Error):                     // Preds: bb0
  dealloc_stack %21 : $*R                         // id: %32
  %33 = copy_value %31 : $any Error               // users: %41, %34
  %34 = begin_borrow [lexical] %33 : $any Error   // users: %40, %38, %35
  debug_value %34 : $any Error, let, name "error", implicit // id: %35
  // function_ref swift_task_localValuePop
  %36 = function_ref @swift_task_localValuePop : $@convention(thin) () -> () // user: %37
  %37 = apply %36() : $@convention(thin) () -> ()
  %38 = copy_value %34 : $any Error               // users: %43, %39
  %39 = builtin "willThrow"(%38 : $any Error) : $()
  end_borrow %34 : $any Error                     // id: %40
  destroy_value %33 : $any Error                  // id: %41
  destroy_value %31 : $any Error                  // id: %42
  throw %38 : $any Error                          // id: %43
} // end sil function '$ss9TaskLocalC9withValue_9operation4file4lineqd__x_qd__yYaKXESSSutYaKlF'

so, not quite sure what to do here; If I'm holding it wrong or we're still missing edge-cases etc.

tshortli · 2022-12-07T20:57:26Z

Adopting @_backDeploy here should be unblocked by a combination of #62444 and #62427.

ktoso · 2022-12-07T23:24:12Z

Great, thank you!

ktoso · 2022-12-08T09:39:59Z

Ok those got merged, so I'll try to revive this 👍

ktoso · 2022-12-09T09:13:49Z

@swift-ci please smoke test

DougGregor · 2022-12-09T20:49:33Z

stdlib/public/Concurrency/TaskLocal.swift

@@ -102,6 +102,7 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
    self.defaultValue = defaultValue
  }

+  @usableFromInline


@usableFromInline adds a public symbol that won't be available on older platforms. Can you make this @alwaysEmitIntoClient?

Ok will do, I hope backdeploy will understand that hmm.

Okey cool it does :)

DougGregor · 2022-12-09T20:50:55Z

stdlib/public/Concurrency/TaskLocal.swift

@@ -135,6 +136,9 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
  /// If the value is a reference type, it will be retained for the duration of
  /// the operation closure.
  @discardableResult
+  @_unsafeInheritExecutor
+  @available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method
+  @_backDeploy(before: macOS 9999, iOS 9999, watchOS 9999, tvOS 9999) // SwiftStdlib 5.8 but it doesn't work with _backDeploy


How sad. @tshortli can @backDeploy support SwiftStdlib 5.8 here?

@_backDeploy supports availability macros in general (they're heavily used in some of the tests). If there's an edge case exposed here, though, let me know @ktoso

~~Yeah it didn’t work with the SwiftStdlib and I tried looking into it but didn’t quite see how those get applied. Help very welcome on this @tshortli !~~

Okey, I was too tired last evening and did a dumb "before: 5.8", confirmed that before: SwiftStdlib 5.8 works fine, thanks @tshortli

stdlib/public/Concurrency/TaskLocal.swift

stdlib/public/BackDeployConcurrency/TaskLocal.swift

stdlib/public/Concurrency/TaskLocal.swift

ktoso · 2022-12-10T02:04:04Z

@swift-ci please smoke test

ktoso · 2022-12-12T03:03:12Z

@swift-ci please smoke test

DougGregor

Looks great now!

stdlib/public/Concurrency/TaskLocal.swift

stdlib/public/BackDeployConcurrency/TaskLocal.swift

stdlib/public/Concurrency/TaskLocal.swift

stdlib/public/BackDeployConcurrency/TaskLocal.swift

Co-authored-by: Doug Gregor <[email protected]>

ktoso · 2022-12-13T01:46:19Z

@swift-ci please smoke test and merge

ktoso · 2022-12-13T01:46:38Z

Thank you, Doug! I'll fix a few similar issues while at it 👍

ktoso requested review from DougGregor and kavon November 24, 2022 05:10

ktoso mentioned this pull request Nov 24, 2022

Actor boundary warning for @TaskLocal values #62220

Closed

pyrtsa reviewed Nov 24, 2022

View reviewed changes

stdlib/public/BackDeployConcurrency/TaskLocal.swift Outdated Show resolved Hide resolved

ktoso added the concurrency Feature: umbrella label for concurrency language features label Nov 25, 2022

ktoso force-pushed the wip-withLocal-warnings branch from 07709fd to a64598a Compare November 28, 2022 23:02

ktoso force-pushed the wip-withLocal-warnings branch from 336f364 to 9b82170 Compare November 29, 2022 02:22

ktoso requested a review from rjmccall November 29, 2022 02:24

ktoso changed the title ~~[Concurrency] TaskLocal.withValue closure should be @Sendable; no warnings inside Actor~~ [Concurrency] TaskLocal.withValue closure should produce no warnings inside Actor Nov 29, 2022

ktoso added 3 commits December 9, 2022 10:22

[Concurrency] tasklocal withValue inside actor is safe

2178d98

Better fix, using inheriting executor

b3c3357

add _backDeploy and availability necessary for it

d122fd6

ktoso force-pushed the wip-withLocal-warnings branch from 9b82170 to d122fd6 Compare December 9, 2022 09:07

DougGregor reviewed Dec 9, 2022

View reviewed changes

ktoso force-pushed the wip-withLocal-warnings branch from 3ce1ec3 to 224a400 Compare December 10, 2022 00:57

ktoso commented Dec 10, 2022

View reviewed changes

stdlib/public/BackDeployConcurrency/TaskLocal.swift Show resolved Hide resolved

ktoso commented Dec 10, 2022

View reviewed changes

stdlib/public/Concurrency/TaskLocal.swift Show resolved Hide resolved

cleaning up attributes

342ce7e

ktoso force-pushed the wip-withLocal-warnings branch from 224a400 to 342ce7e Compare December 12, 2022 03:02

ktoso requested a review from DougGregor December 12, 2022 03:03

DougGregor approved these changes Dec 13, 2022

View reviewed changes

ktoso commented Dec 13, 2022

View reviewed changes

stdlib/public/BackDeployConcurrency/TaskLocal.swift Outdated Show resolved Hide resolved

Apply suggestions from code review

f9c2bc0

Co-authored-by: Doug Gregor <[email protected]>

ktoso mentioned this pull request Dec 13, 2022

[Concurrency] Fix missing inherit executor on withTaskCancellationHandler #62546

Merged

swift-ci merged commit d32c027 into swiftlang:main Dec 13, 2022

ktoso deleted the wip-withLocal-warnings branch December 13, 2022 06:49

[Concurrency] TaskLocal.withValue closure should produce no warnings inside Actor #62239

[Concurrency] TaskLocal.withValue closure should produce no warnings inside Actor #62239

Uh oh!

Conversation

ktoso commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Nov 24, 2022

Uh oh!

ktoso commented Nov 24, 2022

Uh oh!

Uh oh!

ktoso commented Nov 24, 2022

Uh oh!

ktoso commented Nov 28, 2022

Uh oh!

rjmccall commented Nov 28, 2022

Uh oh!

kavon commented Nov 28, 2022

Uh oh!

ktoso commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Nov 28, 2022

Uh oh!

ktoso commented Nov 29, 2022

Uh oh!

rjmccall commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Nov 30, 2022

Uh oh!

ktoso commented Nov 30, 2022

Uh oh!

ktoso commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tshortli commented Dec 7, 2022

Uh oh!

ktoso commented Dec 7, 2022

Uh oh!

ktoso commented Dec 8, 2022

Uh oh!

ktoso commented Dec 9, 2022

Uh oh!

DougGregor Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

ktoso Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

ktoso Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

DougGregor Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

tshortli Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

ktoso Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktoso commented Dec 10, 2022

Uh oh!

ktoso commented Dec 12, 2022

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ktoso commented Nov 24, 2022 •

edited

Loading

ktoso commented Nov 28, 2022 •

edited

Loading

rjmccall commented Nov 29, 2022 •

edited

Loading

ktoso commented Dec 2, 2022 •

edited

Loading

ktoso Dec 9, 2022 •

edited

Loading