[Concurrency] Implement supressible crash/nocrash mode

Author: ktoso

include/swift/Runtime/Bincompat.h

@@ -51,12 +51,33 @@ bool useLegacySwiftObjCHashing();
 /// only log a warning. This changes in future releases and this function
 /// will fatal error always.
 ///
+/// Similarly, the internal runtime function
+/// `swift_task_isCurrentExecutor(expected)` was previously allowed to return
+/// `false`. In future releases it will call into `checkIsolated`, and CRASH
+/// when previously it would have returned false.
+///
+/// Because some applications were running with "isolation warnings" and
+/// those call into the `isCurrentExecutor` API and expected warnings to be
+/// logged, but they ignored those warnings we cannot make them crashing,
+/// and must check if the app was built against a new.
+///
 /// Old behavior:
+/// - `swift_task_isCurrentExecutorImpl` cannot crash and does NOT invoke
+///     `SerialExecutor.checkIsolated`
+/// - `swift_task_isCurrentExecutorImpl` does not invoke `checkIsolated`
 /// - logging a warning on concurrency violation is allowed
+///
 /// New behavior:
 /// - always fatal error in `swift_task_reportUnexpectedExecutor`
+/// - `swift_task_isCurrentExecutorImpl` will crash when it would have returned
+///     false
+/// - `swift_task_isCurrentExecutorImpl` does invoke `checkIsolated` when other
+///     checks failed
+///
+/// This can be overridden by using `SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=1`
+/// or `SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=crash|nocrash`
 SWIFT_RUNTIME_STDLIB_SPI
-bool swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor();
+bool swift_bincompat_useLegacyNonCrashingExecutorChecks();
 
 } // namespace bincompat
 

stdlib/public/Concurrency/Actor.cpp

@@ -316,10 +316,54 @@ bool _task_serialExecutor_isSameExclusiveExecutionContext(
     const Metadata *selfType,
     const SerialExecutorWitnessTable *wtable);
 
+// We currently still support "legacy mode" in which isCurrentExecutor is NOT
+// allowed to crash, because it is used to power "log warnings" data race
+// detector. This mode is going away in Swift 6, but until then we allow this.
+// This override exists primarily to be able to test both code-paths.
+enum IsCurrentExecutorCheckMode: unsigned {
+  /// The default mode when an app was compiled against "new" enough SDK.
+  /// It allows crashing in isCurrentExecutor, and calls into `checkIsolated`.
+  Default_UseCheckIsolated_AllowCrash,
+  /// Legacy mode; Primarily to support old applications which used data race
+  /// detector with "warning" mode, which is no longer supported. When such app
+  /// is re-compiled against a new SDK, it will see crashes in what was
+  /// previously warnings; however, until until recompiled, warnings will be
+  /// used, and `checkIsolated` cannot be invoked.
+  Legacy_NoCheckIsolated_NonCrashing,
+};
+static IsCurrentExecutorCheckMode isCurrentExecutorMode =
+    Default_UseCheckIsolated_AllowCrash;
+
+// Check override of executor checking mode.
+static void checkIsCurrentExecutorMode(void *context) {
+  auto useLegacyMode =
+      swift_bincompat_useLegacyNonCrashingExecutorChecks();
+
+  // Potentially, override the platform detected mode, primarily used in tests.
+#if SWIFT_STDLIB_HAS_ENVIRON
+  const char *modeStr = getenv("SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE");
+  if (!modeStr)
+    return;
+  
+  if (strcmp(modeStr, "nocrash") == 0) {
+    useLegacyMode = Legacy_NoCheckIsolated_NonCrashing;
+  } else if (strcmp(modeStr, "crash") == 0)  {
+    useLegacyMode = Default_UseCheckIsolated_AllowCrash;
+  } // else, just use the platform detected mode
+#endif // SWIFT_STDLIB_HAS_ENVIRON
+  isCurrentExecutorMode = useLegacyMode ? Legacy_NoCheckIsolated_NonCrashing
+                                        : Default_UseCheckIsolated_AllowCrash;
+}
+
 SWIFT_CC(swift)
 static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor) {
   auto current = ExecutorTrackingInfo::current();
 
+  // To support old applications on apple platforms which assumed this call
+  // does not crash, try to use a more compatible mode for those apps.
+  static swift::once_t checkModeToken;
+  swift::once(checkModeToken, checkIsCurrentExecutorMode, nullptr);
+
   if (!current) {
     // We have no current executor, i.e. we are running "outside" of Swift
     // Concurrency. We could still be running on a thread/queue owned by
@@ -328,16 +372,24 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
 
     // Are we expecting the main executor and are using the main thread?
     if (expectedExecutor.isMainExecutor() && isExecutingOnMainThread()) {
-      // TODO(concurrency): consider removing this special case, as checkIsolated will compare against mainQueue already
+      // Due to compatibility with pre-checkIsolated code, we cannot remove
+      // this special handling. CheckIsolated can handle this if the expected
+      // executor is the main queue / main executor, however, if we cannot call
+      // checkIsolated we cannot rely on it to handle this.
+      // TODO: consider removing this branch when `useCrashingCheckIsolated=true`
       return true;
     }
 
     // Otherwise, as last resort, let the expected executor check using
     // external means, as it may "know" this thread is managed by it etc.
-    swift_task_checkIsolated(expectedExecutor);
+    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+      swift_task_checkIsolated(expectedExecutor);
+      // checkIsolated did not crash, so we are on the right executor, after all!
+      return true;
+    }
 
-    // checkIsolated did not crash, so we are on the right executor, after all!
-    return true;
+    assert(isCurrentExecutorMode == Legacy_NoCheckIsolated_NonCrashing);
+    return false;
   }
 
   SerialExecutorRef currentExecutor = current->getActiveExecutor();
@@ -416,10 +468,16 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
   // Note that this only works because the closure in assumeIsolated is
   // synchronous, and will not cause suspensions, as that would require the
   // presence of a Task.
-  swift_task_checkIsolated(expectedExecutor);
+  // compat_invoke_swift_task_checkIsolated(expectedExecutor);
+  if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+    swift_task_checkIsolated(expectedExecutor);
+    // The checkIsolated call did not crash, so we are on the right executor.
+    return true;
+  }
 
-  // The checkIsolated call did not crash, so we are on the right executor.
-  return true;
+  // Using legacy mode, if no explicit executor match worked, we assume `false`
+  assert(isCurrentExecutorMode == Legacy_NoCheckIsolated_NonCrashing);
+  return false;
 }
 
 /// Logging level for unexpected executors:
@@ -431,7 +489,7 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
 /// an application was linked to. Since Swift 6 the default is to crash,
 /// and the logging behavior is no longer available.
 static unsigned unexpectedExecutorLogLevel =
-    swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor()
+    swift_bincompat_useLegacyNonCrashingExecutorChecks()
         ? 1 // legacy apps default to the logging mode, and cannot use `checkIsolated`
         : 2; // new apps will only crash upon concurrency violations, and will call into `checkIsolated`
 

stdlib/public/runtime/Bincompat.cpp

@@ -256,7 +256,7 @@ bool useLegacySwiftObjCHashing() {
 }
 
 // FIXME(concurrency): Once the release is announced, adjust the logic detecting the SDKs
-bool swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor() {
+bool swift_bincompat_useLegacyNonCrashingExecutorChecks() {
 #if BINARY_COMPATIBILITY_APPLE
   return true; // For now, legacy behavior on Apple OSes
 #elif SWIFT_TARGET_OS_DARWIN
@@ -266,7 +266,6 @@ bool swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor() {
 #endif
 }
 
-
 } // namespace bincompat
 
 } // namespace runtime

test/Concurrency/Runtime/data_race_detection_default.swift renamed to test/Concurrency/Runtime/data_race_detection_crash.swift

@@ -1,7 +1,10 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-build-swift %import-libdispatch -Xfrontend -disable-availability-checking -enable-actor-data-race-checks -parse-as-library %s -o %t/a.out -module-name main
 // RUN: %target-codesign %t/a.out
-// RUN: env %env-SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/a.out
+
+// NOTE: This test specifically tests the crashing behavior of `checkIsolated`,
+// because this behavior is currently disabled
+// RUN: env %env-SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=crash %target-run %t/a.out
 
 // REQUIRES: executable_test
 // REQUIRES: concurrency

test/Concurrency/Runtime/data_race_detection_legacy_warning.swift

@@ -1,7 +1,12 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-build-swift %import-libdispatch -Xfrontend -disable-availability-checking -enable-actor-data-race-checks -parse-as-library %s -o %t/a.out -module-name main
 // RUN: %target-codesign %t/a.out
-// RUN: env %env-SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=1 %target-run %t/a.out 2>&1 | %FileCheck %s
+
+// We specifically test for legacy behavior here, apps compiled against old SDKs
+// will be able to have this behavior, however new apps will not. We use the
+// overrides to test the logic for legacy code remains functional.
+//
+// RUN: env %env-SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=1 %env-SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=nocrash %target-run %t/a.out 2>&1 | %FileCheck %s
 
 // REQUIRES: executable_test
 // REQUIRES: concurrency
@@ -61,14 +66,14 @@ actor MyActor {
 struct Runner {
   static func main() async {
     print("Launching a main-actor task")
-    // CHECK: warning: data race detected: @MainActor function at main/data_race_detection_legacy_warning.swift:25 was not called on the main thread
+    // CHECK: warning: data race detected: @MainActor function at main/data_race_detection_legacy_warning.swift:30 was not called on the main thread
     launchFromMainThread()
     sleep(1)
 
     let actor = MyActor()
     let actorFn = await actor.getTaskOnMyActor()
     print("Launching an actor-instance task")
-    // CHECK: warning: data race detected: actor-isolated function at main/data_race_detection_legacy_warning.swift:54 was not called on the same actor
+    // CHECK: warning: data race detected: actor-isolated function at main/data_race_detection_legacy_warning.swift:59 was not called on the same actor
     launchTask(actorFn)
 
     sleep(1)

test/abi/macOS/arm64/stdlib.swift

@@ -257,4 +257,4 @@ Added: __swift_willThrowTypedImpl
 Added: __swift_enableSwizzlingOfAllocationAndRefCountingFunctions_forInstrumentsOnly
 
 // Runtime bincompat functions for Concurrency runtime to detect legacy mode
-Added: _swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor
+Added: _swift_bincompat_useLegacyNonCrashingExecutorChecks

test/abi/macOS/x86_64/stdlib.swift

@@ -257,4 +257,4 @@ Added: __swift_willThrowTypedImpl
 Added: __swift_enableSwizzlingOfAllocationAndRefCountingFunctions_forInstrumentsOnly
 
 // Runtime bincompat functions for Concurrency runtime to detect legacy mode
-Added: _swift_bincompat_useLegacyWarningModeReportUnexpectedExecutor
+Added: _swift_bincompat_useLegacyNonCrashingExecutorChecks