Synchronize with cancellation when removing a task from a task group

We were detaching the child by just modifying the list, but the cancellation path was assuming that that would not be done without holding the task status lock.

This patch just fixes the current runtime; the back-deployment side is complicated.

Fixes rdar://88398824

Author: rjmccall

include/swift/ABI/TaskGroup.h

@@ -44,10 +44,14 @@ class alignas(Alignment_TaskGroup) TaskGroup {
   /// Checks the cancellation status of the group.
   bool isCancelled();
 
-  // Add a child task to the group. Always called with the status record lock of
-  // the parent task held
+  // Add a child task to the task group. Always called while holding the
+  // status record lock of the task group's owning task.
   void addChildTask(AsyncTask *task);
 
+  // Remove a child task from the task group. Always called while holding
+  // the status record lock of the task group's owning task.
+  void removeChildTask(AsyncTask *task);
+
   // Provide accessor for task group's status record
   TaskGroupTaskStatusRecord *getTaskRecord();
 };

include/swift/ABI/TaskStatus.h

@@ -150,15 +150,24 @@ class ChildTaskStatusRecord : public TaskStatusRecord {
 ///
 /// A record always is a specific `TaskGroupImpl`.
 ///
+/// This record holds references to all the non-completed children of
+/// the task group.  It may also hold references to completed children
+/// which have not yet been found by `next()`.
+///
 /// The child tasks are stored as an invasive single-linked list, starting
 /// from `FirstChild` and continuing through the `NextChild` pointers of all
 /// the linked children.
 ///
-/// All children of the specific `Group` are stored "by" this record,
-/// so that they may be cancelled when this task becomes cancelled.
+/// This list structure should only ever be modified:
+/// - while holding the status record lock of the owning task, so that
+///   asynchronous operations such as cancellation can walk the structure
+///   without having to acquire a secondary lock, and
+/// - synchronously with the owning task, so that the owning task doesn't
+///   have to acquire the status record lock just to walk the structure
+///   itself.
 ///
 /// When the group exits, it may simply remove this single record from the task
-/// running it. As it has guaranteed that the tasks have already completed.
+/// running it, as it has guaranteed that the tasks have already completed.
 ///
 /// Group child tasks DO NOT have their own `ChildTaskStatusRecord` entries,
 /// and are only tracked by their respective `TaskGroupTaskStatusRecord`.

stdlib/public/BackDeployConcurrency/CompatibilityOverrideConcurrency.def

@@ -34,11 +34,11 @@
 /// entries, or define one or more of the more specific OVERRIDE_* variants to
 /// get only those entries.
 
-// NOTE: this file is used to build the definition of OverrideSection in
-// CompatibilityOverride.cpp, which is part of the ABI. Moving or removing
-// entries in this file will break the ABI. Additional entries can be added to
-// the end. ABI breaks or version-specific changes can be accommodated by
-// changing the name of the override section in that file.
+// NOTE: the entries in this file are used to build the struct layout for
+// OverrideSection in CompatibilityOverride.cpp, which is built into the
+// concurrency runtime.  The back-deployment concurrency runtime targets
+// the same compatibility overrides as the OS-deployed runtime in Swift 5.6.
+// This file should not be edited.
 
 #ifdef OVERRIDE
 #  define OVERRIDE_ACTOR OVERRIDE

stdlib/public/CompatibilityOverride/CompatibilityOverride.h

@@ -1,4 +1,4 @@
-//===--- CompatibilityOverride.h - Back-deploying compatibility fixes --*- C++ -*-===//
+//===--- CompatibilityOverride.h - Back-deployment patches ------*- C++ -*-===//
 //
 // This source file is part of the Swift.org open source project
 //
@@ -10,7 +10,71 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// Support back-deploying compatibility fixes for newer apps running on older runtimes.
+// The compatibility override system supports the back-deployment of
+// bug fixes and ABI additions to existing Swift systems.  This is
+// primarily of interest on Apple platforms, since other platforms don't
+// ship with a stable Swift runtime that cannot simply be replaced.
+//
+// The compatibility override system works as follows:
+//
+// 1. Certain runtime functions are "hooked" so that they can be replaced
+//    by the program at launch time.  The function at the public symbol
+//    (we will use `swift_task_cancel` as a consistent example) is a
+//    thunk that either calls the standard implementation or its dynamic
+//    replacement.  If a dynamic replacement is called, it is passed
+//    the standard implementation as an extra argument.
+//
+// 2. The public thunks are defined in different .cpp files throughout
+//    the runtime by the COMPATIBILITY_OVERRIDE macro below, triggered
+//    by an #include at the bottom of the file.  The list of definitions
+//    to expand in a particular file is defined by the appropriate
+//    CompatibilityOverride*.def file for the current runtime component
+//    (i.e. either the main runtime or the concurrency runtime).
+//    The standard implementation must be defined in that file as a
+//    function with the suffix `Impl`, e.g. `swift_task_cancelImpl`.
+//    Usually the standard implementation should be static, and
+//    everywhere else in the runtime should call the public symbol.
+//
+// 3. The public thunks determine their replacement by calling an
+//    override accessor for the symbol the first time they are called.
+//    These accessors are named e.g. `swift::getOverride_swift_task_cancel`
+//    and are defined by CompatibilityOverride.cpp, which is built
+//    separately for each runtime component using different build
+//    settings.
+//
+// 4. The override accessors check for a Mach-O section with a specific
+//    name, and if it exists, they interpret the section as a struct
+//    with one field per replaceable function.  The order of fields is
+//    determined by the appropriate CompatibilityOverride*.def file.
+//    The section name, the struct layout, and the function signatures of
+//    the replacement functions are the only parts of this which are ABI;
+//    everything else is an internal detail of the runtime which can be
+//    changed if useful.
+//
+// 5. The name of the Mach-O section is specific to both the current
+//    runtime component and the current version of the Swift runtime.
+//    Therefore, a compatibility override library always targets a
+//    specific runtime version and implicitly does nothing on other
+//    versions.
+//
+// 6. Compatibility override libraries define a Mach-O section with the
+//    appropriate name and layout for their target component and version
+//    and initialize the appropriate fields within it to the replacement
+//    functions.  This occurs in the Overrides.cpp file in the library.
+//    Compatibility override libraries are linked against later override
+//    libraries for the same component, so if a patch needs to be applied
+//    to multiple versions, the last version can define public symbols
+//    that the other versions can use (assuming that any internal runtime
+//    structures are roughly compatible).
+//
+// 7. Compatibility override libraries are rebuilt with every Swift
+//    release in case that release requires new patches to the target
+//    runtime.  They are therefore live code, unlike e.g. the
+//    back-deployment concurrency runtime.
+//
+// 8. The back-deployment concurrency runtime looks for the same section
+//    name as the OS-installed 5.6 runtime and therefore will be patched
+//    by the 5.6 compatibility override library.
 //
 //===----------------------------------------------------------------------===//
 

stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def

@@ -11,7 +11,8 @@
 //===----------------------------------------------------------------------===//
 //
 // This file defines x-macros used for metaprogramming with the set of
-// compatibility override functions.
+// compatibility override functions.  See CompatibilityOverride.h for
+// a detailed explanation of how this system works.
 //
 //===----------------------------------------------------------------------===//
 
@@ -32,13 +33,42 @@
 ///
 /// The entries are organized by group. A user may define OVERRIDE to get all
 /// entries, or define one or more of the more specific OVERRIDE_* variants to
-/// get only those entries.
-
-// NOTE: this file is used to build the definition of OverrideSection in
-// CompatibilityOverride.cpp, which is part of the ABI. Moving or removing
-// entries in this file will break the ABI. Additional entries can be added to
-// the end. ABI breaks or version-specific changes can be accommodated by
-// changing the name of the override section in that file.
+/// get only those entries.  The more specific OVERRIDE_* variants group
+/// entries into the functions that are emitted in the specified file;
+/// for example, OVERRIDE_ACTOR identifies the functions that are defined
+/// in Actor.cpp.
+
+// NOTE: the entries in this file are used to build the struct layout for
+// the OverrideSection in the CompatibilityOverride.cpp that is built into
+// the concurrency runtime.  A matching file must be used to build the
+// ConcurrencyOverrideSection in Overrides.cpp for future compatibility
+// override libraries that target this release.
+//
+// Because compatibility override libraries target a specific release of
+// Swift, there is no inherent reason the entries in this file cannot be
+// arbitrarily rearranged between release cycles, as long as a matching
+// file is used to build any future compatibility override library
+// targeting this release.  However, the targeting of compatibility
+// override libraries is precise only to a specific major+minor release
+// number (e.g. 5.6).  Therefore, care must be taken to avoid ABI breaks
+// in this file between patch releases, or else it will become impossible
+// to create a compatibility override library for this release:
+//
+// - Moving or removing entries in this file will break the ABI.
+//
+// - Changing an entry to use a different implementation file is allowed,
+//   but do not move the entry to be grouped with the other entries for
+//   the implementation file, as this will break the ABI.
+//
+// - New entries can be added to the end without breaking the ABI.  This
+//   is possible even if there have already been patch releases for this
+//   major+minor release, since older patch releases of the runtime will
+//   simply not read the new fields.  It is not possible if a compatibility
+//   override library has already been released for this major+minor
+//   release, but that is unlikely for releases under active development.
+//
+// When creating a new compatibility override library, always clone the
+// last .def files from the appropriate release branch and edit this comment.
 
 #ifdef OVERRIDE
 #  define OVERRIDE_ACTOR OVERRIDE
@@ -319,9 +349,9 @@ OVERRIDE_TASK_STATUS(task_hasTaskGroupStatusRecord, bool,
 OVERRIDE_TASK_STATUS(task_cancel, void, SWIFT_EXPORT_FROM(swift_Concurrency),
                      SWIFT_CC(swift), swift::, (AsyncTask *task), (task))
 
-OVERRIDE_TASK_STATUS(task_cancel_group_child_tasks, void,
-                     SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
-                     swift::, (TaskGroup *group), (group))
+OVERRIDE_TASK_GROUP(task_cancel_group_child_tasks, void,
+                    SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
+                    swift::, (TaskGroup *group), (group))
 
 OVERRIDE_TASK_STATUS(task_escalate, JobPriority,
                      SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),

stdlib/public/CompatibilityOverride/CompatibilityOverrideRuntime.def

@@ -11,7 +11,8 @@
 //===----------------------------------------------------------------------===//
 //
 // This file defines x-macros used for metaprogramming with the set of
-// compatibility override functions.
+// compatibility override functions.  See CompatibilityOverride.h for
+// a detailed explanation of how this system works.
 //
 //===----------------------------------------------------------------------===//
 
@@ -31,15 +32,43 @@
 ///     parentheses
 ///
 /// The entries are organized by group. A user may define OVERRIDE to get all
-/// entries, or define one or more of OVERRIDE_METADATALOOKUP, OVERRIDE_CASTING,
-/// OVERRIDE_OBJC, OVERRIDE_FOREIGN, OVERRIDE_PROTOCOLCONFORMANCE,
-/// and OVERRIDE_KEYPATH to get only those entries.
-
-// NOTE: this file is used to build the definition of OverrideSection in
-// CompatibilityOverride.cpp, which is part of the ABI. Moving or removing
-// entries in this file will break the ABI. Additional entries can be added to
-// the end. ABI breaks or version-specific changes can be accommodated by
-// changing the name of the override section in that file.
+/// entries, or define one or more of the more specific OVERRIDE_* variants to
+/// get only those entries.  The more specific OVERRIDE_* variants group
+/// entries into the functions that are emitted in the specified file;
+/// for example, OVERRIDE_CASTING identifies the functions that are defined
+/// in Casting.cpp.
+
+// NOTE: the entries in this file are used to build the struct layout for
+// the OverrideSection in the CompatibilityOverride.cpp that is built into
+// the primary runtime.  A matching file must be used to build the
+// RuntimeOverrideSection in Overrides.cpp for future compatibility
+// override libraries that target this release.
+//
+// Because compatibility override libraries target a specific release of
+// Swift, there is no inherent reason the entries in this file cannot be
+// arbitrarily rearranged between release cycles, as long as a matching
+// file is used to build any future compatibility override library
+// targeting this release.  However, the targeting of compatibility
+// override libraries is precise only to a specific major+minor release
+// number (e.g. 5.6).  Therefore, care must be taken to avoid ABI breaks
+// in this file between patch releases, or else it will become impossible
+// to create a compatibility override library for this release:
+//
+// - Moving or removing entries in this file will break the ABI.
+//
+// - Changing an entry to use a different implementation file is allowed,
+//   but do not move the entry to be grouped with the other entries for
+//   the implementation file, as this will break the ABI.
+//
+// - New entries can be added to the end without breaking the ABI.  This
+//   is possible even if there have already been patch releases for this
+//   major+minor release, since older patch releases of the runtime will
+//   simply not read the new fields.  It is not possible if a compatibility
+//   override library has already been released for this major+minor
+//   release, but that is unlikely for releases under active development.
+//
+// When creating a new compatibility override library, always clone the
+// last .def files from the appropriate release branch and edit this comment.
 
 #ifdef OVERRIDE
 #  define OVERRIDE_METADATALOOKUP OVERRIDE