fix bugs that present themselves when handling multiarg and varargs functions, including adding better debug dump methods

Author: jturcotti

include/swift/AST/DiagnosticsSIL.def

@@ -865,9 +865,9 @@ NOTE(sil_referencebinding_inout_binding_here, none,
 // Warnings arising from the flow-sensitive checking of Sendability of
 // non-Sendable values
 WARNING(arg_region_consumed, none,
-        "this application could pass `self` or a Non-Sendable argument of this function to another thread, potentially yielding a race with the caller", ())
+        "call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller", ())
 WARNING(consumption_yields_race, none,
-        "non-sendable value sent across isolation domains here, but could be accessed later in this function (%0 access site%select{|s}1 displayed%select{|, %3 more hidden}2)",
+        "non-sendable value sent across isolation domains that could be concurrently accessed later in this function (%0 access site%select{|s}1 displayed%select{|, %3 more hidden}2)",
         (unsigned, bool, bool, unsigned))
 WARNING(call_site_consumption_yields_race, none,
         "passing argument of non-sendable type %0 from %1 context to %2 context at this call site could yield a race with accesses later in this function (%3 access site%select{|s}4 displayed%select{|, %6 more hidden}5)",

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -127,12 +127,16 @@ class PartitionOp {
   }
 
   bool operator==(const PartitionOp &other) const {
-      return OpKind == other.OpKind && sourceInst == other.sourceInst;
+      return OpKind == other.OpKind
+             && OpArgs == other.OpArgs
+                && sourceInst == other.sourceInst;
   };
   // implemented for insertion into std::map
   bool operator<(const PartitionOp &other) const {
     if (OpKind != other.OpKind)
       return OpKind < other.OpKind;
+    if (OpArgs != other.OpArgs)
+      return OpArgs < other.OpArgs;
     return sourceInst < other.sourceInst;
   }
 
@@ -444,25 +448,26 @@ class Partition {
       assert(labels.count(op.OpArgs[0]) &&
              "Consume PartitionOp's argument should already be tracked");
 
-      // if attempting to consume a consumed region, handle the failure
-      if (isConsumed(op.OpArgs[0]))
-        handleFailure(op, op.OpArgs[0]);
-
-      // mark region as consumed
-      horizontalUpdate(labels, op.OpArgs[0], Region::consumed());
-
-      // check if any nonconsumables were consumed, and handle the failure if so
+      // check if any nonconsumables are consumed here, and handle the failure if so
       for (Element nonconsumable : nonconsumables) {
         assert(labels.count(nonconsumable) &&
                "nonconsumables should be function args and self, and therefore"
                "always present in the label map because of initialization at "
                "entry");
-        if (isConsumed(nonconsumable)) {
+        if (!isConsumed(nonconsumable) &&
+            labels.at(nonconsumable) == labels.at(op.OpArgs[0])) {
           handleConsumeNonConsumable(op, nonconsumable);
           break;
         }
       }
 
+      // ensure region is consumed
+      if (!isConsumed(op.OpArgs[0]))
+        // mark region as consumed
+        horizontalUpdate(labels, op.OpArgs[0], Region::consumed());
+
+
+
       break;
     case PartitionOpKind::Merge:
       assert(op.OpArgs.size() == 2 &&
@@ -527,7 +532,7 @@ class Partition {
     llvm::dbgs() << "}\n";
   }
 
-  void dump() LLVM_ATTRIBUTE_USED {
+  void dump() const LLVM_ATTRIBUTE_USED {
     std::map<Region, std::vector<Element>> buckets;
 
     for (auto [i, label] : labels)

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -232,14 +232,17 @@ class PartitionOpTranslator {
   // raw pointers to ensure it is treated as non-Sendable and strict checking
   // is applied to it
   bool isNonSendableType(SILType type) const {
-    if (type.getASTType()->getKind() == TypeKind::BuiltinNativeObject) {
+    switch (type.getASTType()->getKind()) {
+    case TypeKind::BuiltinNativeObject:
+    case TypeKind::BuiltinRawPointer:
       // these are very unsafe... definitely not Sendable
       return true;
+    default:
+      // consider caching this if it's a performance bottleneck
+      return TypeChecker::conformsToProtocol(
+                 type.getASTType(), sendableProtocol, function->getParentModule())
+          .hasMissingConformance(function->getParentModule());
     }
-    // consider caching this if it's a bottleneck
-    return TypeChecker::conformsToProtocol(
-        type.getASTType(), sendableProtocol, function->getParentModule())
-        .hasMissingConformance(function->getParentModule());
   }
 
   // used to statefully track the instruction currently being translated,
@@ -403,16 +406,29 @@ class PartitionOpTranslator {
            applyInst->getOperandValues().end()}
       );
 
+    return translateIsolationCrossingSILApply(applyInst);
+  }
+
+  // handles the semantics for SIL applies that cross isolation
+  // in particular, all arguments are consumed
+  std::vector<PartitionOp> translateIsolationCrossingSILApply(
+      const SILInstruction *applyInst) {
     ApplyExpr *sourceApply = applyInst->getLoc().getAsASTNode<ApplyExpr>();
     assert(sourceApply && "only ApplyExpr's should cross isolation domains");
 
     std::vector<PartitionOp> translated;
+
+    // require all operands
+    for (auto op : applyInst->getOperandValues())
+      if (auto trackOp = trackIfNonSendable(op))
+        translated.push_back(Require(trackOp.value()).front());
+
     auto getSourceArg = [&](unsigned i) {
-        if (i < sourceApply->getArgs()->size())
-          return sourceApply->getArgs()->getExpr(i);
+      if (i < sourceApply->getArgs()->size())
+        return sourceApply->getArgs()->getExpr(i);
       assert(false && "SIL instruction has too many arguments for"
                       " corresponding AST node");
-        return (Expr *)nullptr;
+      return (Expr *)nullptr;
     };
 
     auto getSourceSelf = [&]() {
@@ -441,11 +457,11 @@ class PartitionOpTranslator {
     if (auto applyInstCast = dyn_cast<ApplyInst>(applyInst)) {
       handleSILOperands(applyInstCast->getArgumentsWithoutSelf());
       if (applyInstCast->hasSelfArgument())
-        handleSILSelf(applyInstCast->getSelfArgument());
+          handleSILSelf(applyInstCast->getSelfArgument());
     } else if (auto applyInstCase = dyn_cast<TryApplyInst>(applyInst)) {
       handleSILOperands(applyInstCast->getArgumentsWithoutSelf());
       if (applyInstCast->hasSelfArgument())
-        handleSILSelf(applyInstCast->getSelfArgument());
+          handleSILSelf(applyInstCast->getSelfArgument());
     } else {
       llvm_unreachable("this instruction crossing isolation is not handled yet");
     }
@@ -501,15 +517,15 @@ class PartitionOpTranslator {
 
   // used to index the translations of SILInstructions performed
   // for refrence and debugging
-  int translationIndex = 0;
+  static inline int translationIndex = 0;
 
   // Some SILInstructions contribute to the partition of non-Sendable values
   // being analyzed. translateSILInstruction translate a SILInstruction
   // to its effect on the non-Sendable partition, if it has one.
   //
   // The current pattern of
   std::vector<PartitionOp> translateSILInstruction(SILInstruction *instruction) {
-    translationIndex++;
+    LLVM_DEBUG(translationIndex++;);
     currentInstruction = instruction;
 
     // The following instructions are treated as assigning their result to a
@@ -910,11 +926,11 @@ class ConsumeRequireAccumulator {
   // that access ("require") the region consumed. Sorting is by lowest distance
   // first, then arbitrarily. This is used for final diagnostic output.
   void forEachConsumeRequire(
-      unsigned numRequiresPerConsume,
       llvm::function_ref<void(const PartitionOp& consumeOp, unsigned numProcessed, unsigned numSkipped)>
           processConsumeOp,
       llvm::function_ref<void(const PartitionOp& requireOp)>
-          processRequireOp) const {
+          processRequireOp,
+      unsigned numRequiresPerConsume = UINT_MAX) const {
     for (auto [consumeOp, requireOps] : requirementsForConsumptions) {
       unsigned numProcessed = std::min({(unsigned) requireOps.size(),
                                         (unsigned) numRequiresPerConsume});
@@ -927,6 +943,19 @@ class ConsumeRequireAccumulator {
       }
     }
   }
+
+  void dump() const {
+    forEachConsumeRequire(
+        [](const PartitionOp& consumeOp, unsigned numProcessed, unsigned numSkipped) {
+          llvm::dbgs() << " ┌──╼ CONSUME: ";
+          consumeOp.dump();
+        },
+        [](const PartitionOp& requireOp) {
+          llvm::dbgs() << " ├╼ REQUIRE: ";
+          requireOp.dump();
+        }
+    );
+  }
 };
 
 // A RaceTracer is used to accumulate the facts that the main phase of
@@ -1075,8 +1104,9 @@ class RaceTracer {
     if (workingPartition.isConsumed(consumedVal))
       workingPartition.apply(PartitionOp::AssignFresh(consumedVal));
 
+    int i = 0;
     block.forEachPartitionOp([&](const PartitionOp& partitionOp) {
-      if (targetOp && targetOp == partitionOp)
+      if (targetOp == partitionOp)
         return false; //break
       workingPartition.apply(partitionOp);
       if (workingPartition.isConsumed(consumedVal) && !consumedReason) {
@@ -1093,6 +1123,7 @@ class RaceTracer {
         consumedReason = llvm::None;
 
       // continue walking block
+      i++;
       return true;
     });
 
@@ -1102,7 +1133,10 @@ class RaceTracer {
       consumedReason = LocalConsumedReason::NonLocal();
 
     // if consumedReason is none, then consumedVal was not actually consumed
-    assert(consumedReason);
+    assert(consumedReason
+           || dumpBlockSearch(SILBlock, consumedVal)
+           && " no consumption was found"
+    );
 
     // if this is a query for consumption reason at block exit, update the cache
     if (!targetOp)
@@ -1112,6 +1146,31 @@ class RaceTracer {
     return consumedReason.value();
   }
 
+  bool dumpBlockSearch(SILBasicBlock * SILBlock, TrackableValueID consumedVal) {
+    LLVM_DEBUG(
+        unsigned i = 0;
+        const BlockPartitionState &block = blockStates[SILBlock];
+        Partition working = block.getEntryPartition();
+        llvm::dbgs() << "┌──────────╼\n│ ";
+        working.dump();
+        block.forEachPartitionOp([&](const PartitionOp &op) {
+          llvm::dbgs() << "├[" << i++ << "] ";
+          op.dump();
+          working.apply(op);
+          llvm::dbgs() << "│ ";
+          if (working.isConsumed(consumedVal)) {
+            llvm::errs().changeColor(llvm::raw_ostream::RED, true);
+            llvm::errs() << "(" << consumedVal << " CONSUMED) ";
+            llvm::errs().resetColor();
+          }
+          working.dump();
+          return true;
+        });
+        llvm::dbgs() << "└──────────╼\n";
+    );
+    return false;
+  }
+
 public:
   RaceTracer(const BasicBlockData<BlockPartitionState>& blockStates)
       : blockStates(blockStates) {}
@@ -1126,7 +1185,6 @@ class RaceTracer {
   }
 };
 
-
 // Instances of PartitionAnalysis perform the region-based Sendable checking.
 // Internally, a PartitionOpTranslator is stored to perform the translation from
 // SILInstructions to PartitionOps, then a fixed point iteration is run to
@@ -1258,10 +1316,14 @@ class PartitionAnalysis {
   void diagnose() {
     assert(solved && "diagnose should not be called before solve");
 
-    //llvm::dbgs() << function->getName() << "\n";
+    LLVM_DEBUG(
+        llvm::dbgs() << "Emitting diagnostics for function "
+                     << function->getName() << "\n");
     RaceTracer tracer = blockStates;
 
     for (auto [_, blockState] : blockStates) {
+      // populate the raceTracer with all requires of consumed valued found
+      // throughout the CFG
       blockState.diagnoseFailures(
           /*handleFailure=*/
           [&](const PartitionOp& partitionOp, TrackableValueID consumedVal) {
@@ -1282,16 +1344,23 @@ class PartitionAnalysis {
           });
     }
 
+    LLVM_DEBUG(
+        llvm::dbgs() << "Accumulator Complete:\n";
+        raceTracer.getAccumulator().dump();
+    );
+
+    // ask the raceTracer to report diagnostics at the consumption sites
+    // for all the racy requirement sites entered into it above
     raceTracer.getAccumulator().forEachConsumeRequire(
-        NUM_REQUIREMENTS_TO_DIAGNOSE,
         /*diagnoseConsume=*/
         [&](const PartitionOp& consumeOp,
             unsigned numDisplayed, unsigned numHidden) {
 
           if (tryDiagnoseAsCallSite(consumeOp, numDisplayed, numHidden))
             return;
 
-          assert(false);
+          assert(false && "no consumptions besides callsites implemented yet");
+
           // default to more generic diagnostic
           auto expr = getExprForPartitionOp(consumeOp);
           auto diag = function->getASTContext().Diags.diagnose(
@@ -1307,7 +1376,8 @@ class PartitionAnalysis {
           function->getASTContext().Diags.diagnose(
               expr->getLoc(), diag::possible_racy_access_site)
               .highlight(expr->getSourceRange());
-        });
+        },
+        NUM_REQUIREMENTS_TO_DIAGNOSE);
   }
 
   // try to interpret this consumeOp as a source-level callsite (ApplyExpr),
@@ -1331,7 +1401,7 @@ class PartitionAnalysis {
       assert(false && "sourceExpr should be populated for ApplyExpr consumptions");
 
     function->getASTContext().Diags.diagnose(
-        apply->getLoc(), diag::call_site_consumption_yields_race,
+        argExpr->getLoc(), diag::call_site_consumption_yields_race,
         findOriginalValueType(argExpr),
         isolationCrossing.value().getCallerIsolation(),
         isolationCrossing.value().getCalleeIsolation(),