Merge pull request #10518 from atrick/swift-4.0-branch

AnnaZaks · web-flow · commit 29bd6c3c5d3e · 2017-06-23T09:55:18.000-07:00
[4.0] Static exclusivity diagnostics handling of non-escaping closures passed as blocks.
diff --git a/include/swift/SILOptimizer/Analysis/ClosureScope.h b/include/swift/SILOptimizer/Analysis/ClosureScope.h
@@ -95,7 +95,7 @@ class ClosureScopeAnalysis : public SILAnalysis {
     IndexLookupFunc(const std::vector<SILFunction *> &indexedScopes)
         : indexedScopes(indexedScopes) {}
 
-    Optional<SILFunction *> operator()(int &idx) const {
+    Optional<SILFunction *> operator()(int idx) const {
       if (auto funcPtr = indexedScopes[idx]) {
         return funcPtr;
       }
diff --git a/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp b/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp
@@ -106,6 +106,8 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
     case ValueKind::ExistentialMetatypeInst:
     case ValueKind::ValueMetatypeInst:
     case ValueKind::LoadInst:
+    case ValueKind::LoadBorrowInst:
+    case ValueKind::EndBorrowInst:
     case ValueKind::OpenExistentialAddrInst:
     case ValueKind::ProjectBlockStorageInst:
       // These likely represent scenarios in which we're not generating
@@ -129,24 +131,46 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
 /// only ultimately used by an apply, a try_apply or as an argument (but not
 /// the called function) in a partial_apply.
 /// TODO: This really should be checked in the SILVerifier.
-static bool isExpectedUseOfNoEscapePartialApply(SILInstruction *user) {
-  if (!user)
-    return true;
+static bool hasExpectedUsesOfNoEscapePartialApply(Operand *partialApplyUse) {
+  SILInstruction *user = partialApplyUse->getUser();
 
   // It is fine to call the partial apply
-  if (isa<ApplyInst>(user) || isa<TryApplyInst>(user)) {
+  switch (user->getKind()) {
+  case ValueKind::ApplyInst:
+  case ValueKind::TryApplyInst:
     return true;
-  }
-
-  if (isa<ConvertFunctionInst>(user)) {
-    return isExpectedUseOfNoEscapePartialApply(user->getSingleUse()->getUser());
-  }
 
-  if (auto *PAI = dyn_cast<PartialApplyInst>(user)) {
-    return user != PAI->getCallee();
+  case ValueKind::ConvertFunctionInst:
+    return llvm::all_of(user->getUses(),
+                        hasExpectedUsesOfNoEscapePartialApply);
+
+  case ValueKind::PartialApplyInst:
+    return partialApplyUse->get() != cast<PartialApplyInst>(user)->getCallee();
+
+  case ValueKind::StoreInst:
+  case ValueKind::DestroyValueInst:
+    // @block_storage is passed by storing it to the stack. We know this is
+    // still nonescaping simply because our original argument convention is
+    // @inout_aliasable. In this SIL, both store and destroy_value are users
+    // of %closure:
+    //
+    // %closure = partial_apply %f1(%arg)
+    //   : $@convention(thin) (@inout_aliasable T) -> ()
+    // %storage = alloc_stack $@block_storage @callee_owned () -> ()
+    // %block_addr = project_block_storage %storage
+    //   : $*@block_storage @callee_owned () -> ()
+    // store %closure to [init] %block_addr : $*@callee_owned () -> ()
+    // %block = init_block_storage_header %storage
+    //     : $*@block_storage @callee_owned () -> (),
+    //   invoke %f2 : $@convention(c)
+    //     (@inout_aliasable @block_storage @callee_owned () -> ()) -> (),
+    //   type $@convention(block) () -> ()
+    // %copy = copy_block %block : $@convention(block) () -> ()
+    // destroy_value %storage : $@callee_owned () -> ()
+    return true;
+  default:
+    return false;
   }
-
-  return false;
 }
 #endif
 
@@ -164,15 +188,9 @@ void AccessSummaryAnalysis::processPartialApply(FunctionInfo *callerInfo,
   assert(isa<FunctionRefInst>(apply->getCallee()) &&
          "Noescape partial apply of non-functionref?");
 
-  SILInstruction *user = apply->getSingleUse()->getUser();
-  assert(isExpectedUseOfNoEscapePartialApply(user) &&
+  assert(llvm::all_of(apply->getUses(),
+                      hasExpectedUsesOfNoEscapePartialApply) &&
          "noescape partial_apply has unexpected use!");
-  (void)user;
-
-  // The arguments to partial_apply are a suffix of the arguments to the
-  // the actually-called function. Translate the index of the argument to
-  // the partial_apply into to the corresponding index into the arguments of
-  // the called function.
 
   // The argument index in the called function.
   ApplySite site(apply);
diff --git a/lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp b/lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp
@@ -57,6 +57,11 @@ static void setDynamicEnforcement(BeginAccessInst *access) {
 namespace {
 // Information about an address-type closure capture.
 // This is only valid for inout_aliasable parameters.
+//
+// TODO: Verify somewhere that we properly handle any non-inout_aliasable
+// partial apply captures or that they never happen. Eventually @inout_aliasable
+// should be simply replaced by @in or @out, once we don't have special aliasing
+// rules.
 struct AddressCapture {
   ApplySite site;
   unsigned calleeArgIdx;
@@ -466,7 +471,7 @@ void SelectEnforcement::updateCapture(AddressCapture capture) {
 namespace {
 
 // Model the kind of access needed based on analyzing the access's source.
-// This is either determined to be static or dynamic, or requries further
+// This is either determined to be static or dynamic, or requires further
 // analysis of a boxed variable.
 struct SourceAccess {
   enum { StaticAccess, DynamicAccess, BoxAccess } kind;
@@ -534,8 +539,10 @@ void AccessEnforcementSelection::processFunction(SILFunction *F) {
 
       if (auto access = dyn_cast<BeginAccessInst>(inst))
         handleAccess(access);
+
       else if (auto access = dyn_cast<BeginUnpairedAccessInst>(inst))
         assert(access->getEnforcement() == SILAccessEnforcement::Dynamic);
+
       else if(auto pa = dyn_cast<PartialApplyInst>(inst))
         handlePartialApply(pa);
     }
diff --git a/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp b/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp
@@ -985,6 +985,11 @@ static PartialApplyInst *lookThroughForPartialApply(SILValue V) {
 /// if any of the @inout_aliasable captures passed to those closures have
 /// in-progress accesses that would conflict with any access the summary
 /// says the closure would perform.
+//
+/// TODO: We currently fail to statically diagnose non-escaping closures pased
+/// via @block_storage convention. To enforce this case, we should statically
+/// recognize when the apply takes a block argument that has been initialized to
+/// a non-escaping closure.
 static void checkForViolationsInNoEscapeClosures(
     const StorageMap &Accesses, FullApplySite FAS, AccessSummaryAnalysis *ASA,
     llvm::SmallVectorImpl<ConflictingAccess> &ConflictingAccesses) {
diff --git a/test/SILOptimizer/access_enforcement_noescape.swift b/test/SILOptimizer/access_enforcement_noescape.swift
@@ -6,11 +6,9 @@
 // (Some static/dynamic enforcement selection is done in SILGen, and some is
 // deferred. That may change over time but we want the outcome to be the same).
 //
-// Each FIXME line is a case that the current implementation misses.
-// The model is currently being refined, so this isn't set in stone.
-//
-// TODO: Ensure that each dynamic case is covered by
-// Interpreter/enforce_exclusive_access.swift.
+// These tests attempt to fully cover the possibilities of reads and
+// modifications to captures along with `inout` arguments on both the caller and
+// callee side.
 
 // Helper
 func doOne(_ f: () -> ()) {
@@ -24,10 +22,8 @@ func doTwo(_: ()->(), _: ()->()) {}
 func doOneInout(_: ()->(), _: inout Int) {}
 
 // Error: Cannot capture nonescaping closure.
-// Verification disabled because it suppresses all the other errors.
-// disabled-note@+1{{parameter 'fn' is implicitly non-escaping}}
+// This triggers an early diagnostics, so it's handled in inout_capture_disgnostics.swift.
 // func reentrantCapturedNoescape(fn: (() -> ()) -> ()) {
-//   disabled-error@+1{{closure use of non-escaping parameter 'fn' may allow it to escape}}
 //   let c = { fn {} }
 //   fn(c)
 // }
@@ -308,8 +304,8 @@ func inoutReadWriteInout(x: inout Int) {
 // CHECK: end_access [[ACCESS]] 
 // CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape19inoutReadWriteInoutySiz1x_tFyycfU_'
 
-// Trap on boxed read + write inout.
-// FIXME: Passing a captured var as inout needs dynamic enforcement.
+// Traps on boxed read + write inout.
+// Covered by Interpreter/enforce_exclusive_access.swift.
 func readBoxWriteInout() {
   var x = 3
   let c = { _ = x }
@@ -333,12 +329,11 @@ func readBoxWriteInout() {
 // CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape17readBoxWriteInoutyyFyycfU_'
 
 // Error: inout cannot be captured.
-// Verification disabled because is suppresses other errors.
-//func inoutReadBoxWriteInout(x: inout Int) {
-//  disabled-error@+1{{escaping closures can only capture inout parameters explicitly by value}}
-//  let c = { _ = x }
-//  doOneInout(c, &x)
-//}
+// This triggers an early diagnostics, so it's handled in inout_capture_disgnostics.swift.
+// func inoutReadBoxWriteInout(x: inout Int) {
+//   let c = { _ = x }
+//   doOneInout(c, &x)
+// }
 
 // Allow aliased noescape write + write.
 func writeWrite() {
@@ -395,9 +390,8 @@ func inoutWriteWrite(x: inout Int) {
 // CHECK: end_access [[ACCESS]] 
 // CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape010inoutWriteE0ySiz1x_tFyycfU0_'
 
-// FIXME: Trap on aliased boxed write + noescape write.
-//
-// See the note above.
+// Traps on aliased boxed write + noescape write.
+// Covered by Interpreter/enforce_exclusive_access.swift.
 func writeWriteBox() {
   var x = 3
   let c = { x = 87 }
@@ -479,8 +473,8 @@ func inoutWriteWriteInout(x: inout Int) {
 // CHECK: end_access [[ACCESS]] 
 // CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape010inoutWriteE5InoutySiz1x_tFyycfU_'
 
-// Trap on boxed write + write inout.
-// FIXME: Passing a captured var as inout needs dynamic enforcement.
+// Traps on boxed write + write inout.
+// Covered by Interpreter/enforce_exclusive_access.swift.
 func writeBoxWriteInout() {
   var x = 3
   let c = { x = 42 }
@@ -504,9 +498,67 @@ func writeBoxWriteInout() {
 // CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape18writeBoxWriteInoutyyFyycfU_'
 
 // Error: Cannot capture inout
-// Verification disabled because it suppresses other errors.
+// This triggers an early diagnostics, so it's handled in inout_capture_disgnostics.swift.
 // func inoutWriteBoxWriteInout(x: inout Int) {
-//   disabled-error@+1{{escaping closures can only capture inout parameters explicitly by value}}
 //   let c = { x = 42 }
 //   doOneInout(c, &x)
 // }
+
+// Helper
+func doBlockInout(_: @convention(block) ()->(), _: inout Int) {}
+
+// FIXME: This case could be statically enforced, but requires quite a bit of SIL pattern matching.
+func readBlockWriteInout() {
+  var x = 3
+  // Around the call: [modify] [static]
+  // Inside closure: [modify] [static]
+  doBlockInout({ _ = x }, &x)
+}
+
+// CHECK-LABEL: sil hidden @_T027access_enforcement_noescape19readBlockWriteInoutyyF : $@convention(thin) () -> () {
+// CHECK: [[F1:%.*]] = function_ref @_T027access_enforcement_noescape19readBlockWriteInoutyyFyycfU_ : $@convention(thin) (@inout_aliasable Int) -> ()
+// CHECK: [[PA:%.*]] = partial_apply [[F1]](%0) : $@convention(thin) (@inout_aliasable Int) -> ()
+// CHECK-NOT: begin_access
+// CHECK: [[WRITE:%.*]] = begin_access [modify] [static] %0 : $*Int
+// CHECK: apply
+// CHECK: end_access [[WRITE]] : $*Int
+// CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape19readBlockWriteInoutyyF'
+
+// CHECK-LABEL: sil private @_T027access_enforcement_noescape19readBlockWriteInoutyyFyycfU_ : $@convention(thin) (@inout_aliasable Int) -> () {
+// CHECK: begin_access [read] [static] %0 : $*Int
+// CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape19readBlockWriteInoutyyFyycfU_'
+
+// Test AccessSummaryAnalysis.
+//
+// The captured @inout_aliasable argument to `doOne` is re-partially applied,
+// then stored is a box before passing it to doBlockInout.
+func noEscapeBlock() {
+  var x = 3
+  doOne {
+    doBlockInout({ _ = x }, &x)
+  }
+}
+// CHECK-LABEL: sil hidden @_T027access_enforcement_noescape13noEscapeBlockyyF : $@convention(thin) () -> () {
+// CHECK: partial_apply
+// CHECK-NOT: begin_access
+// CHECK: apply
+// CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape13noEscapeBlockyyF'
+
+// CHECK-LABEL: sil private @_T027access_enforcement_noescape13noEscapeBlockyyFyycfU_ : $@convention(thin) (@inout_aliasable Int) -> () {
+// CHECK: [[F1:%.*]] = function_ref @_T027access_enforcement_noescape13noEscapeBlockyyFyycfU_yycfU_ : $@convention(thin) (@inout_aliasable Int) -> ()
+// CHECK: [[PA:%.*]] = partial_apply [[F1]](%0) : $@convention(thin) (@inout_aliasable Int) -> ()
+// CHECK: [[STORAGE:%.*]] = alloc_stack $@block_storage @callee_owned () -> ()
+// CHECK: [[ADDR:%.*]] = project_block_storage %5 : $*@block_storage @callee_owned () -> ()
+// CHECK: store [[PA]] to [[ADDR]] : $*@callee_owned () -> ()
+// CHECK: [[BLOCK:%.*]] = init_block_storage_header [[STORAGE]] : $*@block_storage @callee_owned () -> (), invoke %8 : $@convention(c) (@inout_aliasable @block_storage @callee_owned () -> ()) -> (), type $@convention(block) () -> ()
+// CHECK: [[ARG:%.*]] = copy_block [[BLOCK]] : $@convention(block) () -> ()
+// CHECK: [[WRITE:%.*]] = begin_access [modify] [static] %0 : $*Int
+// CHECK: apply %{{.*}}([[ARG]], [[WRITE]]) : $@convention(thin) (@owned @convention(block) () -> (), @inout Int) -> ()
+// CHECK: end_access [[WRITE]] : $*Int
+// CHECK: dealloc_stack [[STORAGE]] : $*@block_storage @callee_owned () -> ()
+// CHECK: strong_release [[PA]] : $@callee_owned () -> ()
+// CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape13noEscapeBlockyyFyycfU_'
+
+// CHECK-LABEL: sil private @_T027access_enforcement_noescape13noEscapeBlockyyFyycfU_yycfU_ : $@convention(thin) (@inout_aliasable Int) -> () {
+// CHECK: begin_access [read] [static] %0 : $*Int
+// CHECK-LABEL: } // end sil function '_T027access_enforcement_noescape13noEscapeBlockyyFyycfU_yycfU_'
diff --git a/test/SILOptimizer/access_summary_analysis.sil b/test/SILOptimizer/access_summary_analysis.sil
@@ -228,6 +228,31 @@ bb0(%0 : $*Int):
   return %8 : $()
 }
 
+// CHECK-LABEL: @reads
+// CHECK-NEXT: (read)
+sil private  @reads : $@convention(thin) (@inout_aliasable Int) -> Int {
+bb0(%0 : $*Int):
+  %3 = begin_access [read] [unknown] %0 : $*Int
+  %4 = load %3 : $*Int
+  end_access %3 : $*Int
+  return %4 : $Int
+}
+
+// CHECK-LABEL: @convertPartialApplyAndPassToPartialApply
+// CHECK-NEXT: (read)
+sil hidden @convertPartialApplyAndPassToPartialApply : $@convention(thin) (@inout_aliasable Int) -> () {
+bb0(%0 : $*Int):
+  %2 = function_ref @takesAutoClosureReturningGeneric : $@convention(thin) <τ_0_0 where τ_0_0 : Equatable> (@owned @callee_owned () -> (@out τ_0_0, @error Error)) -> ()
+  %3 = function_ref @reads : $@convention(thin) (@inout_aliasable Int) -> Int
+  %4 = partial_apply %3(%0) : $@convention(thin) (@inout_aliasable Int) -> Int
+  %5 = convert_function %4 : $@callee_owned () -> Int to $@callee_owned () -> (Int, @error Error)
+  %6 = function_ref @thunkForAutoClosure : $@convention(thin) (@owned @callee_owned () -> (Int, @error Error)) -> (@out Int, @error Error)
+  %7 = partial_apply %6(%5) : $@convention(thin) (@owned @callee_owned () -> (Int, @error Error)) -> (@out Int, @error Error)
+  %8 = apply %2<Int>(%7) : $@convention(thin) <τ_0_0 where τ_0_0 : Equatable> (@owned @callee_owned () -> (@out τ_0_0, @error Error)) -> ()
+  %9 = tuple ()
+  return %9 : $()
+}
+
 // CHECK-LABEL: @selfRecursion
 // CHECK-NEXT: (modify, none)
 sil private @selfRecursion : $@convention(thin) (@inout_aliasable Int, Int) -> () {
diff --git a/test/SILOptimizer/inout_capture_diagnostics.swift b/test/SILOptimizer/inout_capture_diagnostics.swift
@@ -0,0 +1,28 @@
+// RUN: %target-swift-frontend -enforce-exclusivity=checked -Onone -emit-sil -swift-version 4 -verify -parse-as-library %s
+//
+// This is an adjunct to access_enforcement_noescape.swift to cover early static diagnostics.
+
+// Helper
+func doOneInout(_: ()->(), _: inout Int) {}
+
+// Error: Cannot capture nonescaping closure.
+// expected-note@+1{{parameter 'fn' is implicitly non-escaping}}
+func reentrantCapturedNoescape(fn: (() -> ()) -> ()) {
+  // expected-error@+1{{closure use of non-escaping parameter 'fn' may allow it to escape}}
+  let c = { fn {} }
+  fn(c)
+}
+
+// Error: inout cannot be captured.
+func inoutReadBoxWriteInout(x: inout Int) {
+  // expected-error@+1{{escaping closures can only capture inout parameters explicitly by value}}
+  let c = { _ = x }
+  doOneInout(c, &x)
+}
+
+// Error: Cannot capture inout
+func inoutWriteBoxWriteInout(x: inout Int) {
+  // expected-error@+1{{escaping closures can only capture inout parameters explicitly by value}}
+  let c = { x = 42 }
+  doOneInout(c, &x)
+}

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ class ClosureScopeAnalysis : public SILAnalysis {`
`95`	`95`	`IndexLookupFunc(const std::vector<SILFunction *> &indexedScopes)`
`96`	`96`	`: indexedScopes(indexedScopes) {}`
`97`	`97`
`98`		`- Optional<SILFunction *> operator()(int &idx) const {`
	`98`	`+ Optional<SILFunction *> operator()(int idx) const {`
`99`	`99`	`if (auto funcPtr = indexedScopes[idx]) {`
`100`	`100`	`return funcPtr;`
`101`	`101`	`}`