Merge pull request #31483 from atrick/5.3-fix-exclusivity-coroutine

atrick · web-flow · commit a2ff89b8bab2 · 2020-05-02T00:30:19.000-07:00
[5.3] Diagnose exclusivity in the presence of coroutines.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,46 @@ CHANGELOG
 Swift 5.3
 ----------
 
+* [SR-11700][]:
+
+  Exclusivity violations within code that computes the `default`
+  argument during Dictionary access are now diagnosed.
+
+  ```swift
+  struct Container {
+     static let defaultKey = 0
+
+     var dictionary = [defaultKey:0]
+
+     mutating func incrementValue(at key: Int) {
+       dictionary[key, default: dictionary[Container.defaultKey]!] += 1
+     }
+  }
+  // error: overlapping accesses to 'self.dictionary', but modification requires exclusive access; consider copying to a local variable
+  //     dictionary[key, default: dictionary[Container.defaultKey]!] += 1
+  //     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // note: conflicting access is here
+  //     dictionary[key, default: dictionary[Container.defaultKey]!] += 1
+  //                              ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
+  ```
+
+  The exclusivity violation can be avoided by precomputing the `default`
+  argument using a local variable.
+
+  ```swift
+  struct Container {
+    static let defaultKey = 0
+
+    var dictionary = [defaultKey:0]
+
+    mutating func incrementValue(at key: Int) {
+      let defaultValue = dictionary[Container.defaultKey]!
+      dictionary[key, default: defaultValue] += 1
+    }
+  }
+  // No error.
+  ```
+
 * [SR-7083][]:
 
   Property observers such as `willSet` and `didSet` are now supported on `lazy` properties:
@@ -8068,4 +8108,5 @@ Swift 1.0
 [SR-9827]: <https://bugs.swift.org/browse/SR-9827>
 [SR-11298]: <https://bugs.swift.org/browse/SR-11298>
 [SR-11429]: <https://bugs.swift.org/browse/SR-11429>
+[SR-11700]: <https://bugs.swift.org/browse/SR-11700>
 [SR-11841]: <https://bugs.swift.org/browse/SR-11841>
diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp
@@ -562,6 +562,12 @@ struct ImmutableAddressUseVerifier {
         if (isConsumingOrMutatingYieldUse(use))
           return true;
         break;
+      case SILInstructionKind::BeginAccessInst:
+        if (cast<BeginAccessInst>(inst)->getAccessKind() != SILAccessKind::Read)
+          return true;
+        break;
+      case SILInstructionKind::EndAccessInst:
+        break;
       case SILInstructionKind::CopyAddrInst:
         if (isConsumingOrMutatingCopyAddrUse(use))
           return true;
diff --git a/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp b/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp
@@ -65,6 +65,29 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
     Operand *operand = worklist.pop_back_val();
     SILInstruction *user = operand->getUser();
 
+    // Handle all types of full applies without switching over them.
+    // Ultimately, this analysis only considers calls with @inout_aliasable
+    // arguments because other argument conventions require an access on the
+    // caller side.
+    if (auto apply = FullApplySite::isa(user)) {
+      SILFunction *callee = apply.getCalleeFunction();
+      // We can't apply a summary for function whose body we can't see.  Since
+      // user-provided closures are always in the same module as their callee
+      // This likely indicates a missing begin_access before an open-coded
+      // call.
+      if (!callee || callee->empty()) {
+        summary.mergeWith(SILAccessKind::Modify, apply.getLoc(),
+                          getSubPathTrieRoot());
+        continue;
+      }
+      unsigned operandNumber = operand->getOperandNumber();
+      assert(operandNumber > 0 && "Summarizing apply for non-argument?");
+
+      unsigned calleeArgumentIndex = operandNumber - 1;
+      processCall(info, argumentIndex, callee, calleeArgumentIndex, order);
+      continue;
+    }
+
     switch (user->getKind()) {
     case SILInstructionKind::BeginAccessInst: {
       auto *BAI = cast<BeginAccessInst>(user);
@@ -101,14 +124,6 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
       processPartialApply(info, argumentIndex, cast<PartialApplyInst>(user),
                           operand, order);
       break;
-    case SILInstructionKind::ApplyInst:
-      processFullApply(info, argumentIndex, cast<ApplyInst>(user), operand,
-                       order);
-      break;
-    case SILInstructionKind::TryApplyInst:
-      processFullApply(info, argumentIndex, cast<TryApplyInst>(user), operand,
-                       order);
-      break;
     default:
       // FIXME: These likely represent scenarios in which we're not generating
       // begin access markers. Ignore these for now. But we really should
@@ -124,9 +139,9 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
 }
 
 #ifndef NDEBUG
-/// Sanity check to make sure that a noescape partial apply is
-/// only ultimately used by an apply, a try_apply or as an argument (but not
-/// the called function) in a partial_apply.
+/// Sanity check to make sure that a noescape partial apply is only ultimately
+/// used by directly calling it or passing it as argument, but not using it as a
+/// partial_apply callee.
 ///
 /// FIXME: This needs to be checked in the SILVerifier.
 static bool hasExpectedUsesOfNoEscapePartialApply(Operand *partialApplyUse) {
@@ -136,6 +151,9 @@ static bool hasExpectedUsesOfNoEscapePartialApply(Operand *partialApplyUse) {
   switch (user->getKind()) {
   case SILInstructionKind::ApplyInst:
   case SILInstructionKind::TryApplyInst:
+  case SILInstructionKind::BeginApplyInst:
+    // The partial_apply must be passed to a @noescape argument type, but that
+    // is already checked by the SIL verifier.
     return true;
   // partial_apply [stack] is terminated by a dealloc_stack.
   case SILInstructionKind::DeallocStackInst:
@@ -150,7 +168,10 @@ static bool hasExpectedUsesOfNoEscapePartialApply(Operand *partialApplyUse) {
                         hasExpectedUsesOfNoEscapePartialApply);
 
   case SILInstructionKind::PartialApplyInst:
-    return partialApplyUse->get() != cast<PartialApplyInst>(user)->getCallee();
+    if (partialApplyUse->get() == cast<PartialApplyInst>(user)->getCallee())
+      return false;
+    return llvm::all_of(cast<PartialApplyInst>(user)->getUses(),
+                        hasExpectedUsesOfNoEscapePartialApply);
 
   // Look through begin_borrow.
   case SILInstructionKind::BeginBorrowInst:
@@ -245,27 +266,6 @@ void AccessSummaryAnalysis::processPartialApply(FunctionInfo *callerInfo,
               calleeArgumentIndex, order);
 }
 
-void AccessSummaryAnalysis::processFullApply(FunctionInfo *callerInfo,
-                                             unsigned callerArgumentIndex,
-                                             FullApplySite apply,
-                                             Operand *argumentOperand,
-                                             FunctionOrder &order) {
-  unsigned operandNumber = argumentOperand->getOperandNumber();
-  assert(operandNumber > 0 && "Summarizing apply for non-argument?");
-
-  unsigned calleeArgumentIndex = operandNumber - 1;
-  SILFunction *callee = apply.getCalleeFunction();
-  // We can't apply a summary for function whose body we can't see.
-  // Since user-provided closures are always in the same module as their callee
-  // This likely indicates a missing begin_access before an open-coded
-  // call.
-  if (!callee || callee->empty())
-    return;
-
-  processCall(callerInfo, callerArgumentIndex, callee, calleeArgumentIndex,
-              order);
-}
-
 void AccessSummaryAnalysis::processCall(FunctionInfo *callerInfo,
                                         unsigned callerArgumentIndex,
                                         SILFunction *callee,
diff --git a/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp b/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp
@@ -886,17 +886,15 @@ static void checkForViolationsAtInstruction(SILInstruction &I,
       });
   }
 
-  if (auto *AI = dyn_cast<ApplyInst>(&I)) {
-    // Record calls to swap() for potential Fix-Its.
-    if (isCallToStandardLibrarySwap(AI, I.getFunction()->getASTContext()))
-      State.CallsToSwap.push_back(AI);
-    else
-      checkForViolationAtApply(AI, State);
-    return;
-  }
-
-  if (auto *TAI = dyn_cast<TryApplyInst>(&I)) {
-    checkForViolationAtApply(TAI, State);
+  if (auto apply = FullApplySite::isa(&I)) {
+    if (auto *AI = dyn_cast<ApplyInst>(&I)) {
+      // Record calls to swap() for potential Fix-Its.
+      if (isCallToStandardLibrarySwap(AI, I.getFunction()->getASTContext())) {
+        State.CallsToSwap.push_back(AI);
+        return;
+      }
+    }
+    checkForViolationAtApply(apply, State);
     return;
   }
 
diff --git a/test/SILOptimizer/access_summary_analysis.sil b/test/SILOptimizer/access_summary_analysis.sil
@@ -226,7 +226,7 @@ bb0(%0 : $*(Int, Int)):
 sil @closureWithMissingBody : $@convention(thin) (@inout_aliasable Int, Int) -> ()
 
 // CHECK-LABEL: @callClosureWithMissingBody
-// CHECK-NEXT: ([], [])
+// CHECK-NEXT: ([modify], [])
 sil private [ossa] @callClosureWithMissingBody : $@convention(thin) (@inout_aliasable Int, Int) -> () {
 bb0(%0 : $*Int, %1 : $Int):
   %2 = function_ref @closureWithMissingBody : $@convention(thin) (@inout_aliasable Int, Int) -> ()
diff --git a/test/SILOptimizer/exclusivity_static_diagnostics.sil b/test/SILOptimizer/exclusivity_static_diagnostics.sil
@@ -1436,6 +1436,7 @@ struct TestDynamic {
 
 @_hasStorage @_hasInitialValue var s: S { get set }
 
+// CHECK-LABEL: sil hidden [ossa] @testCallDynamic : $@convention(method) (@inout TestDynamic) -> () {
 sil hidden [ossa] @testCallDynamic : $@convention(method) (@inout TestDynamic) -> () {
 bb0(%0 : $*TestDynamic):
   %access = begin_access [modify] [unknown] %0 : $*TestDynamic // expected-error {{overlapping accesses, but modification requires exclusive access; consider copying to a local variable}}
@@ -1462,3 +1463,137 @@ bb0(%0 : $*Builtin.Int64, %1 : $*TestDynamic):
   %19 = tuple ()
   return %19 : $()
 }
+
+// Dictionary.subscript.modify
+sil [serialized] [always_inline] @$sSD_7defaultq_x_q_yXKtciM : $@yield_once @convention(method) <τ_0_0, τ_0_1 where τ_0_0 : Hashable> (@in_guaranteed τ_0_0, @noescape @callee_guaranteed () -> @out τ_0_1, @inout Dictionary<τ_0_0, τ_0_1>) -> @yields @inout τ_0_1
+
+// -----------------------------------------------------------------------------
+// testBeginApplyOfClosure: DiagnoseStaticExclusivity must consider
+// begin_apply as a user of accessed variables.
+//
+// Test a static exclusivity conflict between an outgoing coroutine
+// argument and the closure passed to the coroutine.
+//   %access = begin_access [modify] %0
+//   begin_apply %coroutine(%access, %closure) -- where the closure captures %0
+
+// CHECK-LABEL: sil private [ossa] @closureForBeginApplyOfClosure : $@convention(thin) (@inout_aliasable Int) -> @out Int {
+sil private [ossa] @closureForBeginApplyOfClosure : $@convention(thin) (@inout_aliasable Int) -> @out Int {
+bb0(%0 : $*Int, %1 : $*Int):
+  %access = begin_access [read] [static] %1 : $*Int // expected-note {{conflicting access is here}}
+  %val = load [trivial] %access : $*Int
+  end_access %access : $*Int
+  store %val to [trivial] %0 : $*Int
+  %v = tuple ()
+  return %v : $()
+}
+
+// CHECK-LABEL: sil [ossa] @testBeginApplyOfClosure : $@convention(thin) (@inout Int, @inout Dictionary<Int, Int>) -> () {
+sil [ossa] @testBeginApplyOfClosure : $@convention(thin) (@inout Int, @inout Dictionary<Int, Int>) -> () {
+bb0(%0 : $*Int, %1 : $*Dictionary<Int, Int>):
+  %f = function_ref @closureForBeginApplyOfClosure : $@convention(thin) (@inout_aliasable Int) -> @out Int
+  %pa = partial_apply [callee_guaranteed] %f(%0) : $@convention(thin) (@inout_aliasable Int) -> @out Int
+  %cvt = convert_escape_to_noescape [not_guaranteed] %pa : $@callee_guaranteed () -> @out Int to $@noescape @callee_guaranteed () -> @out Int
+  // function_ref Dictionary.subscript.modify
+  %mod = function_ref @$sSD_7defaultq_x_q_yXKtciM : $@yield_once @convention(method) <τ_0_0, τ_0_1 where τ_0_0 : Hashable> (@in_guaranteed τ_0_0, @noescape @callee_guaranteed () -> @out τ_0_1, @inout Dictionary<τ_0_0, τ_0_1>) -> @yields @inout τ_0_1
+  %access = begin_access [modify] [static] %0 : $*Int // expected-error {{overlapping accesses, but modification requires exclusive access; consider copying to a local variable}}
+  (%yield, %token) = begin_apply %mod<Int, Int>(%access, %cvt, %1) : $@yield_once @convention(method) <τ_0_0, τ_0_1 where τ_0_0 : Hashable> (@in_guaranteed τ_0_0, @noescape @callee_guaranteed () -> @out τ_0_1, @inout Dictionary<τ_0_0, τ_0_1>) -> @yields @inout τ_0_1
+  end_apply %token
+  end_access %access : $*Int
+  destroy_value %pa : $@callee_guaranteed () -> @out Int
+  %v = tuple ()
+  return %v : $()
+}
+
+// -----------------------------------------------------------------------------
+// testCoroutineWithClosureArg: AccessedSummaryAnalysis must consider
+// begin_apply a valid user of partial_apply.
+//
+// Test that this does not assert in hasExpectedUsesOfNoEscapePartialApply.
+//
+// This test needs two closures, one to capture the variable, another
+// to recapture the variable, so AccessSummary is forced to process
+// the closure.
+// CHECK-LABEL: sil hidden [ossa] @testCoroutineWithClosureArg : $@convention(thin) (Int, @inout Int, @inout Dictionary<Int, Int>) -> () {
+sil hidden [ossa] @testCoroutineWithClosureArg : $@convention(thin) (Int, @inout Int, @inout Dictionary<Int, Int>) -> () {
+bb0(%0 : $Int, %1 : $*Int, %2 : $*Dictionary<Int, Int>):
+  %6 = function_ref @closureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Dictionary<Int, Int>, Int, @inout_aliasable Int) -> ()
+  %7 = apply %6(%2, %0, %1) : $@convention(thin) (@inout_aliasable Dictionary<Int, Int>, Int, @inout_aliasable Int) -> ()
+  %8 = tuple ()
+  return %8 : $()
+}
+
+// thunk for @callee_guaranteed () -> (@unowned Int)
+sil shared [transparent] [serializable] [reabstraction_thunk] [ossa] @$sSiIgd_SiIegr_TR : $@convention(thin) (@noescape @callee_guaranteed () -> Int) -> @out Int {
+bb0(%0 : $*Int, %1 : $@noescape @callee_guaranteed () -> Int):
+  %2 = apply %1() : $@noescape @callee_guaranteed () -> Int
+  store %2 to [trivial] %0 : $*Int
+  %4 = tuple ()
+  return %4 : $()
+}
+
+// CHECK-LABEL: sil private [ossa] @closureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Dictionary<Int, Int>, Int, @inout_aliasable Int) -> () {
+sil private [ossa] @closureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Dictionary<Int, Int>, Int, @inout_aliasable Int) -> () {
+bb0(%0 : $*Dictionary<Int, Int>, %1 : $Int, %2 : $*Int):
+  %6 = function_ref @implicitClosureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> Int
+  %7 = partial_apply [callee_guaranteed] %6(%2) : $@convention(thin) (@inout_aliasable Int) -> Int
+  %8 = convert_escape_to_noescape [not_guaranteed] %7 : $@callee_guaranteed () -> Int to $@noescape @callee_guaranteed () -> Int
+  %13 = begin_access [modify] [unknown] %0 : $*Dictionary<Int, Int>
+  %14 = alloc_stack $Int
+  store %1 to [trivial] %14 : $*Int
+  // thunk for @callee_guaranteed () -> (@unowned Int)
+  %16 = function_ref @$sSiIgd_SiIegr_TR : $@convention(thin) (@noescape @callee_guaranteed () -> Int) -> @out Int
+  %17 = partial_apply [callee_guaranteed] %16(%8) : $@convention(thin) (@noescape @callee_guaranteed () -> Int) -> @out Int
+  %18 = convert_escape_to_noescape [not_guaranteed] %17 : $@callee_guaranteed () -> @out Int to $@noescape @callee_guaranteed () -> @out Int
+  destroy_value %17 : $@callee_guaranteed () -> @out Int
+  // Dictionary.subscript.modify
+  %20 = function_ref @$sSD_7defaultq_x_q_yXKtciM : $@yield_once @convention(method) <τ_0_0, τ_0_1 where τ_0_0 : Hashable> (@in_guaranteed τ_0_0, @noescape @callee_guaranteed () -> @out τ_0_1, @inout Dictionary<τ_0_0, τ_0_1>) -> @yields @inout τ_0_1
+  (%21, %22) = begin_apply %20<Int, Int>(%14, %18, %13) : $@yield_once @convention(method) <τ_0_0, τ_0_1 where τ_0_0 : Hashable> (@in_guaranteed τ_0_0, @noescape @callee_guaranteed () -> @out τ_0_1, @inout Dictionary<τ_0_0, τ_0_1>) -> @yields @inout τ_0_1
+  assign %1 to %21 : $*Int
+  end_apply %22
+  end_access %13 : $*Dictionary<Int, Int>
+  dealloc_stack %14 : $*Int
+  destroy_value %7 : $@callee_guaranteed () -> Int
+  %28 = tuple ()
+  return %28 : $()
+}
+
+// CHECK-LABEL: sil private [transparent] [ossa] @implicitClosureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> Int {
+sil private [transparent] [ossa] @implicitClosureForTestCoroutineWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> Int {
+bb0(%0 : $*Int):
+  %2 = begin_access [read] [unknown] %0 : $*Int
+  %3 = load [trivial] %2 : $*Int
+  end_access %2 : $*Int
+  return %3 : $Int
+}
+
+// -----------------------------------------------------------------------------
+// testExternalWithClosureArg: handle inout arguments to resilient functions
+// conservatively.
+
+sil [ossa] @externalWithInout : $@convention(thin) (@inout Int) -> ()
+
+// CHECK-LABEL: sil private [ossa] @closureForTestExternalWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> () {
+sil private [ossa] @closureForTestExternalWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> () {
+bb0(%0 : $*Int):
+  %f = function_ref @externalWithInout : $@convention(thin) (@inout Int) -> ()
+  %call = apply %f(%0) : $@convention(thin) (@inout Int) -> () // expected-note {{conflicting access is here}}
+  %v = tuple ()
+  return %v : $()
+}
+
+sil [ossa] @calleeForTestExternalWithClosureArg : $@convention(thin) (@inout Int, @noescape @callee_guaranteed () -> ()) -> ()
+
+// CHECK-LABEL: sil hidden [ossa] @testExternalWithClosureArg : $@convention(thin) (@inout Int) -> () {
+sil hidden [ossa] @testExternalWithClosureArg : $@convention(thin) (@inout Int) -> () {
+bb0(%0 : $*Int):
+  %2 = function_ref @closureForTestExternalWithClosureArg : $@convention(thin) (@inout_aliasable Int) -> ()
+  %3 = partial_apply [callee_guaranteed] %2(%0) : $@convention(thin) (@inout_aliasable Int) -> ()
+  %4 = convert_escape_to_noescape [not_guaranteed] %3 : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> ()
+  %5 = begin_access [modify] [unknown] %0 : $*Int // expected-error {{overlapping accesses, but modification requires exclusive access; consider copying to a local variable}}
+  %6 = function_ref @calleeForTestExternalWithClosureArg : $@convention(thin) (@inout Int, @noescape @callee_guaranteed () -> ()) -> ()
+  %7 = apply %6(%5, %4) : $@convention(thin) (@inout Int, @noescape @callee_guaranteed () -> ()) -> ()
+  end_access %5 : $*Int
+  destroy_value %3 : $@callee_guaranteed () -> ()
+  %10 = tuple ()
+  return %10 : $()
+}
diff --git a/test/SILOptimizer/exclusivity_static_diagnostics.swift b/test/SILOptimizer/exclusivity_static_diagnostics.swift
@@ -649,3 +649,31 @@ struct DisjointLet {
     }
   }
 }
+
+// -----------------------------------------------------------------------------
+// coroutineWithClosureArg: AccessedSummaryAnalysis must consider
+// begin_apply a valid user of partial_apply.
+//
+// Test that this does not assert in hasExpectedUsesOfNoEscapePartialApply.
+//
+// This test needs two closures, one to capture the variable, another
+// to recapture the variable, so AccessSummary is forced to process
+// the closure.
+func coroutineWithClosureArg(i: Int, x: inout Int, d: inout Dictionary<Int, Int>) {
+  { d[i, default: x] = 0 }()
+}
+
+// -----------------------------------------------------------------------------
+//
+struct TestConflictInCoroutineClosureArg {
+  static let defaultKey = 0
+
+  var dictionary = [defaultKey:0]
+
+  mutating func incrementValue(at key: Int) {
+    dictionary[key, default:
+      dictionary[TestConflictInCoroutineClosureArg.defaultKey]!] += 1
+    // expected-error@-2 {{overlapping accesses to 'self.dictionary', but modification requires exclusive access; consider copying to a local variable}}
+    // expected-note@-2 {{conflicting access is here}}
+  }
+}
