Merge pull request swiftlang#77308 from rjmccall/optional-isolation-check

Fix a crash when emitting isolation checks in a function with optional isolation

Author: rjmccall

include/swift/SIL/SILBuilder.h

@@ -2711,6 +2711,17 @@ class SILBuilder {
         CaseCounts, DefaultCount, forwardingOwnershipKind));
   }
 
+  /// A convenience function to decompose a scalar optional value with a
+  /// switch_enum.  Returns the object value, which will only be valid
+  /// in `someBB`.  Don't forget to switch insertion blocks after
+  /// calling this.
+  SILPhiArgument *createSwitchOptional(
+      SILLocation loc, SILValue operand,
+      SILBasicBlock *someBB, SILBasicBlock *noneBB,
+      ValueOwnershipKind forwardingOwnershipKind,
+      ProfileCounter someCount = ProfileCounter(),
+      ProfileCounter noneCount = ProfileCounter());
+
   SwitchEnumAddrInst *createSwitchEnumAddr(
       SILLocation Loc, SILValue Operand, SILBasicBlock *DefaultBB,
       ArrayRef<std::pair<EnumElementDecl *, SILBasicBlock *>> CaseBBs,

lib/SIL/IR/SILBuilder.cpp

@@ -681,6 +681,25 @@ void SILBuilder::emitScopedBorrowOperation(SILLocation loc, SILValue original,
     createEndBorrow(loc, value);
 }
 
+SILPhiArgument *SILBuilder::createSwitchOptional(
+                                SILLocation loc, SILValue operand,
+                                SILBasicBlock *someBB, SILBasicBlock *noneBB,
+                                ValueOwnershipKind forwardingOwnershipKind,
+                                ProfileCounter someCount,
+                                ProfileCounter noneCount) {
+  ProfileCounter counts[] = {someCount, noneCount};
+  std::optional<ArrayRef<ProfileCounter>> countsArg = std::nullopt;
+  if (someCount || noneCount) countsArg = counts;
+
+  auto &ctx = getASTContext();
+  auto sei = createSwitchEnum(loc, operand, /*default*/ nullptr,
+                              {{ctx.getOptionalSomeDecl(), someBB},
+                               {ctx.getOptionalNoneDecl(), noneBB}},
+                              countsArg, /*default*/ProfileCounter(),
+                              forwardingOwnershipKind);
+  return sei->createResult(someBB, operand->getType().unwrapOptionalType());
+}
+
 /// Attempt to propagate ownership from \p operand to the returned forwarding
 /// ownership where the forwarded value has type \p targetType. If this fails,
 /// return Owned forwarding ownership instead.

lib/SIL/Verifier/SILVerifier.cpp

@@ -1069,6 +1069,24 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
             what + " must be Optional<Builtin.Executor>");
   }
 
+  /// Require the operand to be an object of some type that conforms to
+  /// Actor or DistributedActor.
+  void requireAnyActorType(SILValue value, bool allowOptional,
+                           bool allowExecutor, const Twine &what) {
+    auto type = value->getType();
+    require(type.isObject(), what + " must be an object type");
+
+    auto actorType = type.getASTType();
+    if (allowOptional) {
+      if (auto objectType = actorType.getOptionalObjectType())
+        actorType = objectType;
+    }
+    if (allowExecutor && isa<BuiltinExecutorType>(actorType))
+      return;
+    require(actorType->isAnyActorType(),
+            what + " must be some kind of actor type");
+  }
+
   /// Assert that two types are equal.
   void requireSameType(Type type1, Type type2, const Twine &complaint) {
     _require(type1->isEqual(type2), complaint,
@@ -5806,10 +5824,21 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     if (HI->getModule().getStage() == SILStage::Lowered) {
       requireOptionalExecutorType(executor,
                                   "hop_to_executor operand in lowered SIL");
+    } else {
+      requireAnyActorType(executor,
+                          /*allow optional*/ true,
+                          /*allow executor*/ true,
+                          "hop_to_executor operand");
     }
   }
 
   void checkExtractExecutorInst(ExtractExecutorInst *EEI) {
+    requireObjectType(BuiltinExecutorType, EEI,
+                      "extract_executor result");
+    requireAnyActorType(EEI->getExpectedExecutor(),
+                        /*allow optional*/ false,
+                        /*allow executor*/ false,
+                        "extract_executor operand");
     if (EEI->getModule().getStage() == SILStage::Lowered) {
       require(false,
               "extract_executor instruction should have been lowered away");

lib/SILGen/SILGenConcurrency.cpp

@@ -736,6 +736,23 @@ void SILGenFunction::emitPreconditionCheckExpectedExecutor(
   // We don't want the debugger to step into these.
   loc.markAutoGenerated();
 
+  // If the function is isolated to an optional actor reference,
+  // check dynamically whether it's non-null.  We don't need to
+  // do an assertion if the expected expected is nil.
+  SILBasicBlock *noneBB = nullptr;
+  bool isOptional = (bool) executorOrActor->getType().getOptionalObjectType();
+  if (isOptional) {
+    // Start by emiting the .some path.
+    noneBB = createBasicBlock();
+    auto someBB = createBasicBlockBefore(noneBB);
+
+    executorOrActor =
+      B.createSwitchOptional(loc, executorOrActor, someBB, noneBB,
+                             executorOrActor->getOwnershipKind());
+
+    B.emitBlock(someBB);
+  }
+
   // Get the executor.
   SILValue executor = B.createExtractExecutor(loc, executorOrActor);
 
@@ -747,6 +764,20 @@ void SILGenFunction::emitPreconditionCheckExpectedExecutor(
       {args.filenameStartPointer, args.filenameLength, args.filenameIsAscii,
        args.line, ManagedValue::forObjectRValueWithoutOwnership(executor)},
       SGFContext());
+
+  // Complete the optional control flow if we had an optional value.
+  if (isOptional) {
+    assert(noneBB);
+    // Finish the .some path by branching to the continuation block.
+    auto contBB = createBasicBlockAfter(noneBB);
+    B.createBranch(loc, contBB);
+
+    // The .none path is trivial.
+    B.emitBlock(noneBB);
+    B.createBranch(loc, contBB);
+
+    B.emitBlock(contBB);
+  }
 }
 
 bool SILGenFunction::unsafelyInheritsExecutor() {

lib/SILOptimizer/Mandatory/LowerHopToActor.cpp

@@ -112,21 +112,19 @@ static bool isOptionalBuiltinExecutor(SILType type) {
 void LowerHopToActor::recordDominatingInstFor(SILInstruction *inst) {
   SILValue actor;
   if (auto *hop = dyn_cast<HopToExecutorInst>(inst)) {
+    // hop_to_executor can take optional and non-optional Builtin.Executor
+    // values directly.  If we see Optional<Builtin.Executor>, there's
+    // nothing to do.
     actor = hop->getTargetExecutor();
-    // If hop_to_executor was emitted with an optional executor operand,
-    // there's nothing to derive.
-    if (isOptionalBuiltinExecutor(actor->getType())) {
+    if (isOptionalBuiltinExecutor(actor->getType()))
       return;
-    }
   } else if (auto *extract = dyn_cast<ExtractExecutorInst>(inst)) {
+    // extract_executor can only take non-optional actor values.
     actor = extract->getExpectedExecutor();
   } else {
     return;
   }
 
-  if (isOptionalBuiltinExecutor(actor->getType()))
-    return;
-
   auto *dominatingInst = ExecutorDerivationForActor.lookup(actor);
   if (dominatingInst) {
     DominatingActorHops.insert(dominatingInst, inst);

test/SILGen/dynamic_isolation_enforcement.swift

@@ -0,0 +1,55 @@
+// RUN: %empty-directory(%t/src)
+// RUN: split-file %s %t/src
+
+/// Build the library A
+// RUN: %target-swift-frontend -emit-module %t/src/Preconcurrency.swift \
+// RUN:   -module-name Preconcurrency -swift-version 5 -enable-library-evolution \
+// RUN:   -emit-module-path %t/Preconcurrency.swiftmodule
+
+// RUN: %target-swift-emit-silgen -swift-version 6 -disable-availability-checking -I %t %t/src/test.swift -o - -verify | %FileCheck %s
+
+// REQUIRES: concurrency
+
+//--- Preconcurrency.swift
+
+public func takeNonSendableClosure_preconcurrency(_ fn: @escaping () -> Int) {}
+public func takeNonSendableClosure_preconcurrency_generic<T>(_ fn: @escaping () -> T) {}
+
+//--- test.swift
+
+import Preconcurrency
+
+func takeNonSendableClosure_strict(_ fn: @escaping () -> Int) { }
+@preconcurrency
+
+actor MyActor {
+  var counter = 0
+}
+func forceIsolation(isolation: isolated MyActor?) {}
+
+// rdar://132478429
+//
+// We were trying to emit dynamic isolation checks in functions that are
+// isolated to optional actor references by just passing that reference
+// directly to extract_executor, which is incorrect --- we need to skip
+// the check when the reference is nil.
+
+// CHECK-LABEL: sil private [ossa] @$s4test0A25OptionalIsolation_checked9isolationyAA7MyActorCSgYi_tFSiycfU_ :
+// CHECK:         [[T0:%.*]] = copy_value %0 : $Optional<MyActor>
+// CHECK-NEXT:    [[BORROW:%.*]] = begin_borrow [[T0]] :
+// CHECK-NEXT:    switch_enum [[BORROW]] : $Optional<MyActor>, case #Optional.some!enumelt: bb1, case #Optional.none!enumelt: bb2
+// CHECK:       bb1([[T0:%.*]] : @guaranteed $MyActor):
+// CHECK-NEXT:    extract_executor [[T0]] : $MyActor
+// CHECK:         // function_ref _checkExpectedExecutor
+// CHECK:         br bb3
+// CHECK:       bb2:
+// CHECK-NEXT:    br bb3
+func testOptionalIsolation_checked(isolation: isolated MyActor?) {
+  takeNonSendableClosure_preconcurrency {
+    // This closure inherits isolation because it's non-Sendable, and
+    // it requires a dynamic check because we're passing it to a
+    // preconcurrency function.
+    forceIsolation(isolation: isolation)
+    return 0
+  }
+}

test/SILOptimizer/consume_operator_kills_addresses_dbginfo.sil

@@ -19,11 +19,6 @@ public protocol P {
 sil @forceSplit : $@convention(thin) @async () -> ()
 sil @genericUse : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
 
-enum Optional<T> {
-case some(T)
-case none
-}
-
 ///////////
 // Tests //
 ///////////