Merge pull request #84879 from gottesmm/pr-9c51dc994a742cf2eaf0f068c64f5e3ed57a22f9

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

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
 //===----------------------------------------------------------------------===//