Merge pull request #61534 from rjmccall/task-group-detach-race

Synchronize with cancellation when removing a task from a task group.

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