Merge pull request #74919 from gottesmm/pr-d8b24a45ff257893c8172491f11a617fc00d5589

[region-isolation] Implement support for 'inout sending' diagnostics.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -943,6 +943,9 @@ NOTE(sil_referencebinding_inout_binding_here, none,
 
 NOTE(regionbasedisolation_maybe_race, none,
      "access can happen concurrently", ())
+NOTE(regionbasedisolation_inout_sending_must_be_reinitialized, none,
+     "'inout sending' parameter must be reinitialized before function exit with a non-actor isolated value",
+     ())
 ERROR(regionbasedisolation_unknown_pattern, none,
       "pattern that the region based isolation checker does not understand how to check. Please file a bug",
       ())
@@ -972,6 +975,9 @@ ERROR(regionbasedisolation_arg_passed_to_strongly_transferred_param, none,
 NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
      "value is %0 since it is in the same region as %1",
      (StringRef, DeclBaseName))
+NOTE(regionbasedisolation_isolated_since_in_same_region_string, none,
+     "%0 is %1since it is in the same region as %2",
+     (Identifier, StringRef, Identifier))
 
 //===---
 // New Transfer Non Sendable Diagnostics
@@ -981,6 +987,13 @@ ERROR(regionbasedisolation_named_transfer_yields_race, none,
       "sending %0 risks causing data races",
       (Identifier))
 
+ERROR(regionbasedisolation_inout_sending_cannot_be_actor_isolated, none,
+      "'inout sending' parameter %0 cannot be %1at end of function",
+      (Identifier, StringRef))
+NOTE(regionbasedisolation_inout_sending_cannot_be_actor_isolated_note, none,
+      "%1%0 risks causing races in between %1uses and caller uses since caller assumes value is not actor isolated",
+      (Identifier, StringRef))
+
 NOTE(regionbasedisolation_named_info_transfer_yields_race, none,
      "sending %1%0 to %2 callee risks causing data races between %2 and local %3 uses",
      (Identifier, StringRef, ActorIsolation, ActorIsolation))

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -410,6 +410,15 @@ enum class PartitionOpKind : uint8_t {
   ///
   /// This is used if we need to reject the program and do not want to assert.
   UnknownPatternError,
+
+  /// Require that a 'inout sending' parameter's region is not transferred and
+  /// disconnected at a specific function exiting term inst.
+  ///
+  /// This ensures that if users transfer away an inout sending parameter, the
+  /// parameter is reinitialized with a disconnected value.
+  ///
+  /// Takes one parameter, the inout parameter that we need to check.
+  RequireInOutSendingAtFunctionExit,
 };
 
 /// PartitionOp represents a primitive operation that can be performed on
@@ -494,6 +503,12 @@ class PartitionOp {
     return PartitionOp(PartitionOpKind::UnknownPatternError, elt, sourceInst);
   }
 
+  static PartitionOp
+  RequireInOutSendingAtFunctionExit(Element elt, SILInstruction *sourceInst) {
+    return PartitionOp(PartitionOpKind::RequireInOutSendingAtFunctionExit, elt,
+                       sourceInst);
+  }
+
   bool operator==(const PartitionOp &other) const {
     return opKind == other.opKind && opArgs == other.opArgs &&
            source == other.source;
@@ -925,6 +940,21 @@ struct PartitionOpEvaluator {
                                                    isolationRegionInfo);
   }
 
+  /// Call our CRTP subclass.
+  void handleInOutSendingNotInitializedAtExitError(
+      const PartitionOp &op, Element elt, Operand *transferringOp) const {
+    return asImpl().handleInOutSendingNotInitializedAtExitError(op, elt,
+                                                                transferringOp);
+  }
+
+  /// Call our CRTP subclass.
+  void handleInOutSendingNotDisconnectedAtExitError(
+      const PartitionOp &op, Element elt,
+      SILDynamicMergedIsolationInfo isolation) const {
+    return asImpl().handleInOutSendingNotDisconnectedAtExitError(op, elt,
+                                                                 isolation);
+  }
+
   /// Call isActorDerived on our CRTP subclass.
   bool isActorDerived(Element elt) const {
     return asImpl().isActorDerived(elt);
@@ -959,8 +989,10 @@ struct PartitionOpEvaluator {
   }
 
   /// Overload of \p getIsolationRegionInfo without an Operand.
-  SILIsolationInfo getIsolationRegionInfo(Region region) const {
-    return getIsolationRegionInfo(region, nullptr).first;
+  SILDynamicMergedIsolationInfo getIsolationRegionInfo(Region region) const {
+    if (auto opt = getIsolationRegionInfo(region, nullptr))
+      return opt->first;
+    return SILDynamicMergedIsolationInfo();
   }
 
   bool isTaskIsolatedDerived(Element elt) const {
@@ -1128,6 +1160,41 @@ struct PartitionOpEvaluator {
         }
       }
       return;
+    case PartitionOpKind::RequireInOutSendingAtFunctionExit: {
+      assert(op.getOpArgs().size() == 1 &&
+             "Require PartitionOp should be passed 1 argument");
+      assert(p.isTrackingElement(op.getOpArgs()[0]) &&
+             "Require PartitionOp's argument should already be tracked");
+
+      // First check if the region of our 'inout sending' element has been
+      // transferred. In that case, we emit a special use after free error.
+      if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[0])) {
+        for (auto transferredOperand : transferredOperandSet->data()) {
+          handleInOutSendingNotInitializedAtExitError(op, op.getOpArgs()[0],
+                                                      transferredOperand);
+        }
+        return;
+      }
+
+      // If we were not transferred, check if our region is actor isolated. If
+      // so, error since we need a disconnected value in the inout parameter.
+      Region inoutSendingRegion = p.getRegion(op.getOpArgs()[0]);
+      auto dynamicRegionIsolation = getIsolationRegionInfo(inoutSendingRegion);
+
+      // If we failed to merge emit an unknown pattern error so we fail.
+      if (!dynamicRegionIsolation) {
+        handleUnknownCodePattern(op);
+        return;
+      }
+
+      // Otherwise, emit the error if the dynamic region isolation is not
+      // disconnected.
+      if (!dynamicRegionIsolation.isDisconnected()) {
+        handleInOutSendingNotDisconnectedAtExitError(op, op.getOpArgs()[0],
+                                                     dynamicRegionIsolation);
+      }
+      return;
+    }
     case PartitionOpKind::UnknownPatternError:
       // Begin tracking the specified element in case we have a later use.
       p.trackNewElement(op.getOpArgs()[0]);
@@ -1315,6 +1382,16 @@ struct PartitionOpEvaluatorBaseImpl : PartitionOpEvaluator<Subclass> {
   /// to our user so that we can emit that error as we process.
   void handleUnknownCodePattern(const PartitionOp &op) const {}
 
+  /// Called if we find an 'inout sending' parameter that is not live at exit.
+  void handleInOutSendingNotInitializedAtExitError(
+      const PartitionOp &op, Element elt, Operand *transferringOp) const {}
+
+  /// Called if we find an 'inout sending' parameter that is live at excit but
+  /// is actor isolated instead of disconnected.
+  void handleInOutSendingNotDisconnectedAtExitError(
+      const PartitionOp &op, Element elt,
+      SILDynamicMergedIsolationInfo actorIsolation) const {}
+
   /// This is used to determine if an element is actor derived. If we determine
   /// that a region containing such an element is transferred, we emit an error
   /// since actor regions cannot be transferred.

include/swift/SILOptimizer/Utils/SILIsolationInfo.h

@@ -81,6 +81,12 @@ class ActorInstance {
     return value.getPointer();
   }
 
+  SILValue maybeGetValue() const {
+    if (getKind() != Kind::Value)
+      return SILValue();
+    return getValue();
+  }
+
   bool isValue() const { return getKind() == Kind::Value; }
 
   bool isAccessorInit() const { return getKind() == Kind::ActorAccessorInit; }
@@ -389,7 +395,7 @@ class SILDynamicMergedIsolationInfo {
 
 public:
   SILDynamicMergedIsolationInfo() : innerInfo() {}
-  SILDynamicMergedIsolationInfo(SILIsolationInfo innerInfo)
+  explicit SILDynamicMergedIsolationInfo(SILIsolationInfo innerInfo)
       : innerInfo(innerInfo) {}
 
   /// Returns nullptr only if both this isolation info and \p other are actor
@@ -404,6 +410,8 @@ class SILDynamicMergedIsolationInfo {
 
   operator bool() const { return bool(innerInfo); }
 
+  SILIsolationInfo *operator->() { return &innerInfo; }
+
   SILIsolationInfo getIsolationInfo() const { return innerInfo; }
 
   bool isDisconnected() const { return innerInfo.isDisconnected(); }
@@ -412,6 +420,12 @@ class SILDynamicMergedIsolationInfo {
     return innerInfo.hasSameIsolation(other);
   }
 
+  static SILDynamicMergedIsolationInfo
+  getDisconnected(bool isUnsafeNonIsolated) {
+    return SILDynamicMergedIsolationInfo(
+        SILIsolationInfo::getDisconnected(isUnsafeNonIsolated));
+  }
+
   SWIFT_DEBUG_DUMP { innerInfo.dump(); }
 
   void printForDiagnostics(llvm::raw_ostream &os) const {

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1205,6 +1205,14 @@ struct PartitionOpBuilder {
         PartitionOp::Require(lookupValueID(value), currentInst));
   }
 
+  void addRequireInOutSendingAtFunctionExit(SILValue value) {
+    assert(valueHasID(value, /*dumpIfHasNoID=*/true) &&
+           "required value should already have been encountered");
+    currentInstPartitionOps.emplace_back(
+        PartitionOp::RequireInOutSendingAtFunctionExit(lookupValueID(value),
+                                                       currentInst));
+  }
+
   void addUnknownPatternError(SILValue value) {
     if (shouldAbortOnUnknownPatternMatchError()) {
       llvm::report_fatal_error(
@@ -1604,9 +1612,9 @@ class PartitionOpTranslator {
     // compiler exits successfully, actor merge errors could not have happened.
     std::optional<SILDynamicMergedIsolationInfo> mergedInfo;
     if (resultIsolationInfoOverride) {
-      mergedInfo = resultIsolationInfoOverride;
+      mergedInfo = SILDynamicMergedIsolationInfo(resultIsolationInfoOverride);
     } else {
-      mergedInfo = SILIsolationInfo::getDisconnected(false);
+      mergedInfo = SILDynamicMergedIsolationInfo::getDisconnected(false);
     }
 
     for (SILValue src : sourceValues) {
@@ -2321,6 +2329,22 @@ class PartitionOpTranslator {
 #define INST(INST, PARENT) TranslationSemantics visit##INST(INST *inst);
 #include "swift/SIL/SILNodes.def"
 
+  /// Adds requires for all sending inout parameters to make sure that they are
+  /// properly updated before the end of the function.
+  void addRequiresForInOutParameters(TermInst *inst) {
+    assert(inst->isFunctionExiting() && "Must be function exiting term inst?!");
+    for (auto *arg : inst->getFunction()->getArguments()) {
+      auto *fArg = cast<SILFunctionArgument>(arg);
+      if (fArg->getArgumentConvention().isInoutConvention() &&
+          fArg->getKnownParameterInfo().hasOption(SILParameterInfo::Sending)) {
+        if (auto ns = tryToTrackValue(arg)) {
+          auto rep = ns->getRepresentative().getValue();
+          builder.addRequireInOutSendingAtFunctionExit(rep);
+        }
+      }
+    }
+  }
+
   /// Top level switch that translates SIL instructions.
   void translateSILInstruction(SILInstruction *inst) {
     builder.reset(inst);
@@ -2710,12 +2734,8 @@ CONSTANT_TRANSLATION(CondFailInst, Ignored)
 // function_ref/class_method which are considered sendable.
 CONSTANT_TRANSLATION(SwitchValueInst, Ignored)
 CONSTANT_TRANSLATION(UnreachableInst, Ignored)
-CONSTANT_TRANSLATION(UnwindInst, Ignored)
-// Doesn't take a parameter.
-CONSTANT_TRANSLATION(ThrowAddrInst, Ignored)
 
 // Terminators that only need require.
-CONSTANT_TRANSLATION(ThrowInst, Require)
 CONSTANT_TRANSLATION(SwitchEnumAddrInst, Require)
 CONSTANT_TRANSLATION(YieldInst, Require)
 
@@ -2725,6 +2745,33 @@ CONSTANT_TRANSLATION(CondBranchInst, TerminatorPhi)
 CONSTANT_TRANSLATION(CheckedCastBranchInst, TerminatorPhi)
 CONSTANT_TRANSLATION(DynamicMethodBranchInst, TerminatorPhi)
 
+// Function exiting terminators.
+//
+// We handle these especially since we want to make sure that inout parameters
+// that are transferred are forced to be reinitialized.
+//
+// There is an assert in TermInst::isFunctionExiting that makes sure we do this
+// correctly.
+//
+// NOTE: We purposely do not require reinitialization along paths that end in
+// unreachable.
+#ifdef FUNCTION_EXITING_TERMINATOR_TRANSLATION
+#error "FUNCTION_EXITING_TERMINATOR_TRANSLATION already defined?!"
+#endif
+
+#define FUNCTION_EXITING_TERMINATOR_CONSTANT(INST, Kind)                       \
+  TranslationSemantics PartitionOpTranslator::visit##INST(INST *inst) {        \
+    assert(inst->isFunctionExiting() && "Must be function exiting?!");         \
+    addRequiresForInOutParameters(inst);                                       \
+    return TranslationSemantics::Kind;                                         \
+  }
+
+FUNCTION_EXITING_TERMINATOR_CONSTANT(UnwindInst, Ignored)
+FUNCTION_EXITING_TERMINATOR_CONSTANT(ThrowAddrInst, Ignored)
+FUNCTION_EXITING_TERMINATOR_CONSTANT(ThrowInst, Require)
+
+#undef FUNCTION_EXITING_TERMINATOR_CONSTANT
+
 // Today, await_async_continuation just takes Sendable values
 // (UnsafeContinuation and UnsafeThrowingContinuation).
 CONSTANT_TRANSLATION(AwaitAsyncContinuationInst, AssertingIfNonSendable)
@@ -2961,6 +3008,7 @@ PartitionOpTranslator::visitLoadBorrowInst(LoadBorrowInst *lbi) {
 }
 
 TranslationSemantics PartitionOpTranslator::visitReturnInst(ReturnInst *ri) {
+  addRequiresForInOutParameters(ri);
   if (ri->getFunction()->getLoweredFunctionType()->hasSendingResult()) {
     return TranslationSemantics::TransferringNoResult;
   }