[rbi] Ensure that we error when two 'inout sending' parameters are in the same region at end of function.

The caller is allowed to assume that the 'inout sending' parameters are not in
the same region on return so can be sent to different isolation domains safely.
To enforce that we have to ensure on return that the two are /actually/ not in
the same region.

rdar://138519484

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -1201,6 +1201,12 @@ NOTE(rbi_add_generic_parameter_sendable_conformance,none,
      "consider making generic parameter %0 conform to the 'Sendable' protocol",
      (Type))
 
+ERROR(regionbasedisolation_inout_sending_in_same_region, none,
+      "'inout sending' parameters %0 and %1 can be potentially accessed from each other at function return risking data races in caller",
+      (Identifier, Identifier))
+NOTE(regionbasedisolation_inout_sending_in_same_region_note, none,
+     "caller function assumes on return that %0 and %1 cannot be used to access each other implying sending them to different isolation domains does not risk a data race", (Identifier, Identifier))
+
 // Concurrency related diagnostics
 ERROR(cannot_find_executor_factory_type, none,
       "the DefaultExecutorFactory type could not be found", ())

include/swift/SIL/SILArgument.h

@@ -426,6 +426,9 @@ class SILFunctionArgument : public SILArgument {
 
   bool isSending() const;
 
+  /// Returns true if this SILFunctionArgument is an 'inout sending' parameter.
+  bool isInOutSending() const;
+
   Lifetime getLifetime() const {
     return getType()
         .getLifetime(*getFunction())

include/swift/SILOptimizer/Utils/PartitionOpError.def

@@ -66,4 +66,7 @@ PARTITION_OP_ERROR(UnknownCodePattern)
 /// non-Sendable value.
 PARTITION_OP_ERROR(NonSendableIsolationCrossingResult)
 
+/// Used to signify that two 'inout sending' values are in the same region.
+PARTITION_OP_ERROR(InOutSendingParametersInSameRegion)
+
 #undef PARTITION_OP_ERROR

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -1191,6 +1191,29 @@ class PartitionOpError {
     }
   };
 
+  struct InOutSendingParametersInSameRegionError {
+    const PartitionOp *op;
+    Element firstInoutSendingParam;
+    SmallVector<Element, 1> otherInOutSendingParams;
+
+    InOutSendingParametersInSameRegionError(
+        const PartitionOp &op, Element firstInoutSendingParam,
+        SmallVector<Element, 1> &&otherInOutSendingParams)
+        : op(&op), firstInoutSendingParam(firstInoutSendingParam),
+          otherInOutSendingParams(std::move(otherInOutSendingParams)) {}
+
+    InOutSendingParametersInSameRegionError(
+        InOutSendingParametersInSameRegionError &&other) = default;
+    InOutSendingParametersInSameRegionError &
+    operator=(InOutSendingParametersInSameRegionError &&other) = default;
+
+    void print(llvm::raw_ostream &os, RegionAnalysisValueMap &valueMap) const;
+
+    SWIFT_DEBUG_DUMPER(dump(RegionAnalysisValueMap &valueMap)) {
+      print(llvm::dbgs(), valueMap);
+    }
+  };
+
   struct NonSendableIsolationCrossingResultError {
     const PartitionOp *op;
 
@@ -1408,6 +1431,42 @@ struct PartitionOpEvaluator {
     return SILValue();
   }
 
+  /// Given the region \p regionOfInOutSendingParam containing the 'inout
+  /// sending' parameter \p firstInOutSendingParam, see if that region contains
+  /// any other 'inout sending' parameters with greater element numbers than \p
+  /// firstInOutSendingParam. If so, place all of the found params into \p
+  /// foundInOutSendingParams and return true. Otherwise return false.
+  ///
+  /// DISCUSSION: The reason why we return all of the found 'inout sending'
+  /// params is at this point we do not know if any of the params have
+  /// diagnostic behavior reduction. It would not be correct if we returned the
+  /// first one and that had that its diagnostic should be ignored but later
+  /// ones did not.
+  ///
+  /// DISCUSSION: The reason why we only place in the elements that have element
+  /// numbers greater than \p firstInOutsendingParam is to ensure that we do not
+  /// emit diagnostics multiple times for the same variables, one for $VAR1,
+  /// $VAR2 and the second for $VAR2, $VAR1.
+  bool findOtherInOutSendingParameters(
+      Region regionOfInOutSendingParam, Element firstInOutSendingParam,
+      SmallVectorImpl<Element> &foundInOutSendingParams) const {
+    for (const auto &pair : p.range()) {
+      // Skip values that are not in the region or if the value is our
+      // firstInOutSendingParam.
+      if (pair.second != regionOfInOutSendingParam ||
+          pair.first <= firstInOutSendingParam)
+        continue;
+
+      auto *value = dyn_cast_or_null<SILFunctionArgument>(
+          getRepresentativeValue(pair.first).maybeGetValue());
+      if (!value || !value->isInOutSending())
+        continue;
+      foundInOutSendingParams.push_back(pair.first);
+    }
+
+    return foundInOutSendingParams.size();
+  }
+
   bool isTaskIsolatedDerived(Element elt) const {
     return asImpl().isTaskIsolatedDerived(elt);
   }
@@ -1722,13 +1781,23 @@ struct PartitionOpEvaluator {
         return;
       }
 
-      // Ok, we have a disconnected value. We could still be returning a
-      // disconnected value in the same region as an 'inout sending'
-      // parameter. We cannot allow that since the caller considers 'inout
-      // sending' values to be in their own region on function return. So it
-      // would assume that it could potentially send the value in the 'inout
-      // sending' parameter and the return value to different isolation
-      // domains... allowing for races.
+      // Ok, we have a disconnected value. First check if we have two 'inout
+      // sending' values in the same region. We want the two values to still be
+      // in different regions in the caller.
+      SmallVector<Element, 1> foundInOutSendingElts;
+      if (findOtherInOutSendingParameters(inoutSendingRegion, op.getOpArg1(),
+                                          foundInOutSendingElts)) {
+        handleError(InOutSendingParametersInSameRegionError(
+            op, op.getOpArg1(), std::move(foundInOutSendingElts)));
+        return;
+      }
+
+      // Finally We could still be returning a disconnected value in the same
+      // region as an 'inout sending' parameter. We cannot allow that since the
+      // caller considers 'inout sending' values to be in their own region on
+      // function return. So it would assume that it could potentially send the
+      // value in the 'inout sending' parameter and the return value to
+      // different isolation domains... allowing for races.
 
       // Even though we are disconnected, see if our SILFunction has an actor
       // isolation. In that case, we want to use that since the direct return

lib/SIL/IR/SILArgument.cpp

@@ -442,3 +442,10 @@ bool SILFunctionArgument::isSending() const {
     return getFunction()->getLoweredFunctionType()->hasSendingResult();
   return getKnownParameterInfo().hasOption(SILParameterInfo::Sending);
 }
+
+bool SILFunctionArgument::isInOutSending() const {
+  // Make sure that we are sending, not an indirect result (since indirect
+  // results can be sending) and have an inout convention.
+  return isSending() && !isIndirectResult() &&
+         getArgumentConvention().isInoutConvention();
+}

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -723,6 +723,7 @@ struct DiagnosticEvaluator final
     case PartitionOpError::SentNeverSendable:
     case PartitionOpError::AssignNeverSendableIntoSendingResult:
     case PartitionOpError::NonSendableIsolationCrossingResult:
+    case PartitionOpError::InOutSendingParametersInSameRegion:
       // We are going to process these later... but dump so we can see that we
       // handled an error here. The rest of the explicit handlers will dump as
       // appropriate if they want to emit an error here (some will squelch the
@@ -3501,6 +3502,138 @@ void NonSendableIsolationCrossingResultDiagnosticEmitter::emit() {
   }
 }
 
+//===----------------------------------------------------------------------===//
+//               MARK: InOutSendingParametersInSameRegionError
+//===----------------------------------------------------------------------===//
+
+namespace {
+
+class InOutSendingParametersInSameRegionDiagnosticEmitter {
+  /// The function exiting inst where the 'inout sending' parameter was actor
+  /// isolated.
+  TermInst *functionExitingInst;
+
+  /// The first 'inout sending' param in the region.
+  SILValue firstInOutSendingParam;
+
+  /// The second 'inout sending' param in the region.
+  SILValue secondInOutSendingParam;
+
+  bool emittedErrorDiagnostic = false;
+
+public:
+  InOutSendingParametersInSameRegionDiagnosticEmitter(
+      TermInst *functionExitingInst, SILValue firstInOutSendingParam,
+      SILValue secondInOutSendingParam)
+      : functionExitingInst(functionExitingInst),
+        firstInOutSendingParam(firstInOutSendingParam),
+        secondInOutSendingParam(secondInOutSendingParam) {}
+
+  ~InOutSendingParametersInSameRegionDiagnosticEmitter() {
+    // If we were supposed to emit a diagnostic and didn't emit an unknown
+    // pattern error.
+    if (!emittedErrorDiagnostic)
+      emitUnknownPatternError();
+  }
+
+  SILFunction *getFunction() const {
+    return functionExitingInst->getFunction();
+  }
+
+  std::optional<DiagnosticBehavior> getBehaviorLimit() const {
+    auto first =
+        firstInOutSendingParam->getType().getConcurrencyDiagnosticBehavior(
+            getFunction());
+    auto second =
+        secondInOutSendingParam->getType().getConcurrencyDiagnosticBehavior(
+            getFunction());
+    if (!first)
+      return second;
+    if (!second)
+      return first;
+    return first->merge(*second);
+  }
+
+  void emitUnknownPatternError() {
+    if (shouldAbortOnUnknownPatternMatchError()) {
+      llvm::report_fatal_error(
+          "RegionIsolation: Aborting on unknown pattern match error");
+    }
+
+    diagnoseError(functionExitingInst,
+                  diag::regionbasedisolation_unknown_pattern)
+        .limitBehaviorIf(getBehaviorLimit());
+  }
+
+  void emit();
+
+  ASTContext &getASTContext() const {
+    return functionExitingInst->getFunction()->getASTContext();
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseError(SourceLoc loc, Diag<T...> diag,
+                                   U &&...args) {
+    emittedErrorDiagnostic = true;
+    return std::move(getASTContext()
+                         .Diags.diagnose(loc, diag, std::forward<U>(args)...)
+                         .warnUntilSwiftVersion(6));
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseError(SILLocation loc, Diag<T...> diag,
+                                   U &&...args) {
+    return diagnoseError(loc.getSourceLoc(), diag, std::forward<U>(args)...);
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseError(SILInstruction *inst, Diag<T...> diag,
+                                   U &&...args) {
+    return diagnoseError(inst->getLoc(), diag, std::forward<U>(args)...);
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseNote(SourceLoc loc, Diag<T...> diag, U &&...args) {
+    return getASTContext().Diags.diagnose(loc, diag, std::forward<U>(args)...);
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseNote(SILLocation loc, Diag<T...> diag,
+                                  U &&...args) {
+    return diagnoseNote(loc.getSourceLoc(), diag, std::forward<U>(args)...);
+  }
+
+  template <typename... T, typename... U>
+  InFlightDiagnostic diagnoseNote(SILInstruction *inst, Diag<T...> diag,
+                                  U &&...args) {
+    return diagnoseNote(inst->getLoc(), diag, std::forward<U>(args)...);
+  }
+};
+
+} // namespace
+
+void InOutSendingParametersInSameRegionDiagnosticEmitter::emit() {
+  // We should always be able to find a name for an inout sending param. If we
+  // do not, emit an unknown pattern error.
+  auto firstName = inferNameHelper(firstInOutSendingParam);
+  if (!firstName) {
+    return emitUnknownPatternError();
+  }
+  auto secondName = inferNameHelper(secondInOutSendingParam);
+  if (!secondName) {
+    return emitUnknownPatternError();
+  }
+
+  diagnoseError(functionExitingInst,
+                diag::regionbasedisolation_inout_sending_in_same_region,
+                *firstName, *secondName)
+      .limitBehaviorIf(getBehaviorLimit());
+
+  diagnoseNote(functionExitingInst,
+               diag::regionbasedisolation_inout_sending_in_same_region_note,
+               *firstName, *secondName);
+}
+
 //===----------------------------------------------------------------------===//
 //                         MARK: Top Level Entrypoint
 //===----------------------------------------------------------------------===//
@@ -3569,6 +3702,30 @@ void SendNonSendableImpl::emitVerbatimErrors() {
       diagnosticInferrer.emit();
       continue;
     }
+    case PartitionOpError::InOutSendingParametersInSameRegion: {
+      auto e =
+          std::move(erasedError).getInOutSendingParametersInSameRegionError();
+      REGIONBASEDISOLATION_LOG(e.print(llvm::dbgs(), info->getValueMap()));
+      auto firstParam =
+          info->getValueMap().getRepresentativeValue(e.firstInoutSendingParam);
+      // Walk through our secondInOutSendingParams and use one that is not
+      // ignore.
+      for (auto paramElt : e.otherInOutSendingParams) {
+        auto paramValue =
+            info->getValueMap().getRepresentativeValue(paramElt).getValue();
+        if (auto behavior =
+                paramValue->getType().getConcurrencyDiagnosticBehavior(
+                    info->getFunction());
+            behavior && *behavior == DiagnosticBehavior::Ignore) {
+          continue;
+        }
+        InOutSendingParametersInSameRegionDiagnosticEmitter diagnosticEmitter(
+            cast<TermInst>(e.op->getSourceInst()), firstParam.getValue(),
+            paramValue);
+        diagnosticEmitter.emit();
+      }
+      continue;
+    }
     }
     llvm_unreachable("Covered switch isn't covered?!");
   }

lib/SILOptimizer/Utils/PartitionUtils.cpp

@@ -111,6 +111,18 @@ void PartitionOpError::InOutSendingReturnedError::print(
      << "        Rep: " << valueMap.getRepresentativeValue(returnedValue);
 }
 
+void PartitionOpError::InOutSendingParametersInSameRegionError::print(
+    llvm::raw_ostream &os, RegionAnalysisValueMap &valueMap) const {
+  os << "    Emitting Error. Kind: InOutSendingParametersInSameRegion!\n"
+     << "        First ID:  %%" << firstInoutSendingParam
+     << "        First Rep: "
+     << valueMap.getRepresentativeValue(firstInoutSendingParam);
+  for (auto other : otherInOutSendingParams) {
+    os << "        Other ID:  %%" << other
+       << "        Other Rep: " << valueMap.getRepresentativeValue(other);
+  }
+}
+
 //===----------------------------------------------------------------------===//
 //                             MARK: PartitionOp
 //===----------------------------------------------------------------------===//