minor cleanups and renames

Author: ktoso

include/swift/ABI/TaskLocal.h

@@ -130,8 +130,6 @@ class TaskLocal {
     }
 
     void relinkNext(Item* nextOverride) {
-      fprintf(stderr, "[%s:%d](%s) try relink item:%p\n", __FILE_NAME__, __LINE__, __FUNCTION__, this);
-      fprintf(stderr, "[%s:%d](%s) try relink to target:%p\n", __FILE_NAME__, __LINE__, __FUNCTION__, nextOverride);
       assert(!getNext() &&
                "Can only relink task local item that was not pointing at anything yet");
       assert(nextOverride->isNextLinkPointer() ||

include/swift/Runtime/Concurrency.h

@@ -668,9 +668,17 @@ void swift_task_localValuePop();
 SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift)
 void swift_task_localsCopyTo(AsyncTask* target);
 
-// FIXME no need for runtime API
+/// Some task local bindings must be copied defensively when a child task is
+/// created in a task group. See task creation (swift_task_create_common) for
+/// a detailed discussion how and when this is used.
+///
+/// Its Swift signature is
+///
+/// \code
+/// func swift_task_localsCopyToTaskGroupChildTaskDefensively<Key>(AsyncTask* task)
+/// \endcode
 SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift)
-void swift_task_localsOnlyCurrentCopyTo(AsyncTask* target);
+void swift_task_localsCopyToTaskGroupChildTaskDefensively(AsyncTask* target);
 
 /// Switch the current task to a new executor if we aren't already
 /// running on a compatible executor.

stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def

@@ -378,7 +378,7 @@ OVERRIDE_TASK_LOCAL(task_localsCopyTo, void,
                     (AsyncTask *target),
                     (target))
 
-OVERRIDE_TASK_LOCAL(task_localsOnlyCurrentCopyTo, void,
+OVERRIDE_TASK_LOCAL(task_localsCopyToTaskGroupChildTaskDefensively, void,
                     SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
                     swift::,
                     (AsyncTask *target),

stdlib/public/Concurrency/Task.cpp

@@ -735,22 +735,6 @@ swift_task_create_commonImpl(size_t rawTaskCreateFlags,
   /// FIXME: this is the fail path
   if (group) {
     assert(parent && "a task created in a group must be a child task");
-//
-//    // Prevent task-local misuse;
-//    // We must not allow an addTask {} wrapped immediately with a withValue {}
-//    auto ParentLocal = parent->_private().Local;
-//    if (auto taskLocalHeadLinkType = ParentLocal.peekHeadLinkType()) {
-//      if (taskLocalHeadLinkType ==
-//          swift::TaskLocal::NextLinkType::IsNextCreatedInTaskGroupBody) {
-//#if !SWIFT_CONCURRENCY_EMBEDDED
-//        swift_task_reportIllegalTaskLocalBindingWithinWithTaskGroup(
-//            nullptr, 0, true, 0);
-//#endif
-//        // TODO(ktoso): If we were to keep this crash mode; offer a better failure for embedded swift
-//        abort();
-//      }
-//    }
-//
     // Add to the task group, if requested.
     if (taskCreateFlags.addPendingGroupTaskUnconditionally()) {
       assert(group && "Missing group");
@@ -1024,10 +1008,17 @@ swift_task_create_commonImpl(size_t rawTaskCreateFlags,
       // their structural lifetime guarantee is upheld by the group scope
       // out-living any addTask created tasks.
       auto ParentLocal = parent->_private().Local;
+      // If we were going to copy ALL values anyway, we don't need to
+      // perform this defensive partial copying. In practice, we currently
+      // do not have child tasks which force copying, but we could.
+      assert(!taskCreateFlags.copyTaskLocals() &&
+             "Currently we don't have child tasks which force copying task "
+             "locals; unexpected attempt to combine the two!");
+
       if (auto taskLocalHeadLinkType = ParentLocal.peekHeadLinkType()) {
         if (taskLocalHeadLinkType ==
             swift::TaskLocal::NextLinkType::IsNextCreatedInTaskGroupBody) {
-          swift_task_localsOnlyCurrentCopyTo(task);
+          swift_task_localsCopyToTaskGroupChildTaskDefensively(task);
           taskLocalStorageInitialized = true;
         }
       }

stdlib/public/Concurrency/TaskLocal.cpp

@@ -164,7 +164,7 @@ static void swift_task_localsCopyToImpl(AsyncTask *target) {
 }
 
 SWIFT_CC(swift)
-static void swift_task_localsOnlyCurrentCopyToImpl(AsyncTask *target) {
+static void swift_task_localsCopyToTaskGroupChildTaskDefensivelyImpl(AsyncTask *target) {
   TaskLocal::Storage *Local = nullptr;
 
   if (AsyncTask *task = swift_task_getCurrent()) {
@@ -289,6 +289,9 @@ TaskLocal::Item::copyTo(AsyncTask *target) {
 // =============================================================================
 // ==== checks -----------------------------------------------------------------
 
+/// UNUSED: This is effectively not used anymore by new runtimes because we will
+/// defensively copy in this situation since Swift 6, rather than crash as a
+/// means of defence.
 SWIFT_CC(swift)
 static void swift_task_reportIllegalTaskLocalBindingWithinWithTaskGroupImpl(
     const unsigned char *_unused_file, uintptr_t _unused_fileLength,
@@ -521,7 +524,6 @@ void TaskLocal::Storage::copyToOnlyOnlyFromCurrent(AsyncTask *target) {
     if (copied.emplace(item->key).second) {
 
       if (!item->isNextLinkPointerCreatedInTaskGroupBody() && copiedHead) {
-        SWIFT_TASK_LOCAL_DEBUG_LOG(item->key, "break out, next item is not within body, item:%p", item);
         // The next item is not the "risky one" so we can directly link to it,
         // as we would have within normal child task relationships. E.g. this is
         // a parent or next pointer to a "safe" (withValue { withTaskGroup { ... } })
@@ -532,18 +534,12 @@ void TaskLocal::Storage::copyToOnlyOnlyFromCurrent(AsyncTask *target) {
 
       auto copy = item->copyTo(target);
       if (!copiedHead) {
-        SWIFT_TASK_LOCAL_DEBUG_LOG(item->key, "store copied head item:%p",
-                                   copiedHead);
         copiedHead = copy;
       }
 
-      SWIFT_TASK_LOCAL_DEBUG_LOG(item->key, "copy value [%p] to target:%p, item:%p, copied:%p",
-                                 item->getStoragePtr(), target, item, copiedHead);
-
       // If we didn't copy an item, e.g. because it was a pointer to parent,
       // break out of the loop and keep pointing at parent still.
       if (!copy) {
-        SWIFT_TASK_LOCAL_DEBUG_LOG(item->key, "break out, next is %p", 0);
         break;
       }
     } else {

stdlib/public/Concurrency/TaskLocal.swift

@@ -362,8 +362,9 @@ func _taskLocalsCopy(
 
 // ==== Checks -----------------------------------------------------------------
 
-@available(SwiftStdlib 5.1, *)
 @usableFromInline
+@available(SwiftStdlib 5.1, *)
+@available(*, deprecated, message: "The situation diagnosed by this is not handled gracefully rather than by crashing")
 func _checkIllegalTaskLocalBindingWithinWithTaskGroup(file: String, line: UInt) {
   if _taskHasTaskGroupStatusRecord() {
     file.withCString { _fileStart in
@@ -373,8 +374,9 @@ func _checkIllegalTaskLocalBindingWithinWithTaskGroup(file: String, line: UInt)
   }
 }
 
-@available(SwiftStdlib 5.1, *)
 @usableFromInline
+@available(SwiftStdlib 5.1, *)
+@available(*, deprecated, message: "The situation diagnosed by this is not handled gracefully rather than by crashing")
 @_silgen_name("swift_task_reportIllegalTaskLocalBindingWithinWithTaskGroup")
 func _reportIllegalTaskLocalBindingWithinWithTaskGroup(
   _ _filenameStart: UnsafePointer<Int8>,

test/Concurrency/Runtime/async_task_locals_in_task_group_may_need_to_copy.swift

@@ -18,6 +18,19 @@ enum TL {
 // ==== ------------------------------------------------------------------------
 
 func test() async {
+  // no outer task locals
+  await withTaskGroup(of: Void.self) { group in
+    TL.$two.withValue(2222) {
+      // should not have any effect on reads below
+    }
+    await TL.$two.withValue(22) {
+      group.addTask { // will have to copy the `22`
+        print("Survived, one: \(TL.one) @ \(#fileID):\(#line)") // CHECK: Survived, one: 1
+        print("Survived, two: \(TL.two) @ \(#fileID):\(#line)") // CHECK: Survived, two: 22
+      }
+    }
+  }
+
   await TL.$one.withValue(11) {
     await TL.$one.withValue(1111) {
       await withTaskGroup(of: Void.self) { group in

test/abi/macOS/arm64/concurrency.swift

@@ -284,6 +284,9 @@ Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixy
 // async function pointer to Swift.withTaskExecutorPreference<A, B where B: Swift.Error>(_: Swift.TaskExecutor?, isolation: isolated Swift.Actor?, operation: () async throws(B) -> A) async throws(B) -> A
 Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixyYaq_YKXEtYaq_YKs5ErrorR_r0_lFTu
 
+// === Task groups can now handle (copy) task locals set directly around an addTask
+Added: _swift_task_localsCopyToTaskGroupChildTaskDefensively
+
 // === Add #isolation to next() and waitForAll() in task groups
 // Swift.TaskGroup.awaitAllRemainingTasks(isolation: isolated Swift.Actor?) async -> ()
 Added: _$sScG22awaitAllRemainingTasks9isolationyScA_pSgYi_tYaF
@@ -318,4 +321,5 @@ Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__y
 Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
 // Swift.TaskLocal.withValue<A>(_: A, operation: () async throws -> A1, isolation: isolated Swift.Actor?, file: Swift.String, line: Swift.UInt) async throws -> A1
 Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlF
-Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
+Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
+

test/abi/macOS/x86_64/concurrency.swift

@@ -284,6 +284,9 @@ Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixy
 // async function pointer to Swift.withTaskExecutorPreference<A, B where B: Swift.Error>(_: Swift.TaskExecutor?, isolation: isolated Swift.Actor?, operation: () async throws(B) -> A) async throws(B) -> A
 Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixyYaq_YKXEtYaq_YKs5ErrorR_r0_lFTu
 
+// === Task groups can now handle (copy) task locals set directly around an addTask
+Added: _swift_task_localsCopyToTaskGroupChildTaskDefensively
+
 // === Add #isolation to next() and waitForAll() in task groups
 // Swift.TaskGroup.awaitAllRemainingTasks(isolation: isolated Swift.Actor?) async -> ()
 Added: _$sScG22awaitAllRemainingTasks9isolationyScA_pSgYi_tYaF