Fix AccessEnforcementReleaseSinking. Check for illegal cases.

In the included, test case, the optimization was sinking
releases past is_escaping_closure.

Rewrite the isBarrier logic to be conservative and define the
mayCheckRefCount property in SIL/InstructionUtils. Properties that may
need to be updated when SIL changes belong there.

Note that it is particularly bad behavior if the presence of access
markers in the code cause miscompiles unrelated to access enforcement.

Fixes <rdar://problem/45846920> TestFoundation, TestProcess, closure
argument passed as @NoEscape to Objective-C has escaped.

Author: atrick

include/swift/SIL/InstructionUtils.h

@@ -17,6 +17,10 @@
 
 namespace swift {
 
+//===----------------------------------------------------------------------===//
+//                         SSA Use-Def Helpers
+//===----------------------------------------------------------------------===//
+
 /// Strip off casts/indexing insts/address projections from V until there is
 /// nothing left to strip.
 SILValue getUnderlyingObject(SILValue V);
@@ -78,6 +82,10 @@ SILValue stripExpectIntrinsic(SILValue V);
 /// ust return V.
 SILValue stripBorrow(SILValue V);
 
+//===----------------------------------------------------------------------===//
+//                         Instruction Properties
+//===----------------------------------------------------------------------===//
+
 /// Return a non-null SingleValueInstruction if the given instruction merely
 /// copies the value of its first operand, possibly changing its type or
 /// ownership state, but otherwise having no effect.
@@ -110,6 +118,10 @@ bool isIncidentalUse(SILInstruction *user);
 /// only used in recognizable patterns without otherwise "escaping".
 bool onlyAffectsRefCount(SILInstruction *user);
 
+/// Returns true if the given user instruction checks the ref count of a
+/// pointer.
+bool mayCheckRefCount(SILInstruction *User);
+
 /// Return true when the instruction represents added instrumentation for
 /// run-time sanitizers.
 bool isSanitizerInstrumentation(SILInstruction *Instruction);

include/swift/SILOptimizer/Analysis/ARCAnalysis.h

@@ -51,9 +51,6 @@ bool isReleaseInstruction(SILInstruction *II);
 bool mayDecrementRefCount(SILInstruction *User, SILValue Ptr,
                           AliasAnalysis *AA);
 
-/// \returns True if the user \p User checks the ref count of a pointer.
-bool mayCheckRefCount(SILInstruction *User);
-
 /// \returns True if the \p User might use the pointer \p Ptr in a manner that
 /// requires \p Ptr to be alive before Inst or the release of Ptr may use memory
 /// accessed by \p User.

lib/SIL/InstructionUtils.cpp

@@ -296,6 +296,10 @@ bool swift::onlyAffectsRefCount(SILInstruction *user) {
   }
 }
 
+bool swift::mayCheckRefCount(SILInstruction *User) {
+  return isa<IsUniqueInst>(User) || isa<IsEscapingClosureInst>(User);
+}
+
 bool swift::isSanitizerInstrumentation(SILInstruction *Instruction) {
   auto *BI = dyn_cast<BuiltinInst>(Instruction);
   if (!BI)

lib/SILOptimizer/Analysis/ARCAnalysis.cpp

@@ -87,11 +87,6 @@ bool swift::mayDecrementRefCount(SILInstruction *User,
   return true;
 }
 
-bool swift::mayCheckRefCount(SILInstruction *User) {
-  return isa<IsUniqueInst>(User) ||
-         isa<IsEscapingClosureInst>(User);
-}
-
 //===----------------------------------------------------------------------===//
 //                                Use Analysis
 //===----------------------------------------------------------------------===//

lib/SILOptimizer/Transforms/ARCCodeMotion.cpp

@@ -70,13 +70,14 @@
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "sil-rr-code-motion"
+#include "swift/SIL/InstructionUtils.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SILOptimizer/Analysis/ARCAnalysis.h"
 #include "swift/SILOptimizer/Analysis/AliasAnalysis.h"
 #include "swift/SILOptimizer/Analysis/EscapeAnalysis.h"
 #include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
-#include "swift/SILOptimizer/Analysis/RCIdentityAnalysis.h"
 #include "swift/SILOptimizer/Analysis/ProgramTerminationAnalysis.h"
+#include "swift/SILOptimizer/Analysis/RCIdentityAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/CFG.h"

lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp

@@ -27,6 +27,7 @@
 
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/DebugUtils.h"
+#include "swift/SIL/InstructionUtils.h"
 #include "swift/SIL/SILFunction.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 
@@ -47,15 +48,67 @@ static bool isSinkable(SILInstruction &inst) {
 }
 
 // Returns a bool: If this is a "barrier" instruction for this opt
-static bool isBarrier(SILInstruction &inst) {
-  switch (inst.getKind()) {
-  default:
-    return FullApplySite::isa(&inst) != FullApplySite();
-  case SILInstructionKind::BeginAccessInst:
-  case SILInstructionKind::BeginUnpairedAccessInst:
-  case SILInstructionKind::IsUniqueInst:
+static bool isBarrier(SILInstruction *inst) {
+  // Calls hide many dangers, from checking reference counts, to beginning
+  // keypath access, to forcing memory to be live. Checking for these and other
+  // possible barries at ever call is certainly not worth it.
+  if (FullApplySite::isa(inst) != FullApplySite())
+    return true;
+
+  // Don't extend lifetime past any sort of uniqueness check.
+  if (mayCheckRefCount(inst))
     return true;
+
+  // Don't extend object lifetime past deallocation.
+  if (isa<DeallocationInst>(inst))
+    return true;
+
+  // Avoid introducing access conflicts.
+  if (isa<BeginAccessInst>(inst) || isa<BeginUnpairedAccessInst>(inst))
+    return true;
+
+  if (auto *BI = dyn_cast<BuiltinInst>(inst)) {
+    auto kind = BI->getBuiltinKind();
+    if (!kind)
+      return false; // LLVM intrinsics are not barriers.
+
+    // Whitelist the safe builtin categories. Builtins should generally be
+    // treated conservatively, because introducing a new builtin does not
+    // require updating all passes to be aware of it. Avoid a default to ensure
+    // that all categories are covered.
+    switch (kind.getValue()) {
+    case BuiltinValueKind::None:
+      llvm_unreachable("Builtin must has a non-empty kind.");
+#define BUILTIN_NO_BARRIER(Id)                                                 \
+  case BuiltinValueKind::Id:                                                   \
+    return false;
+#define BUILTIN_CAST_OPERATION(Id, Name, Attrs) BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_CAST_OR_BITCAST_OPERATION(Id, Name, Attrs)                     \
+  BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_BINARY_OPERATION(Id, Name, Attrs, Overload)                    \
+  BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_BINARY_OPERATION_WITH_OVERFLOW(Id, Name, UncheckedID, Attrs,   \
+                                               Overload)                       \
+  BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_UNARY_OPERATION(Id, Name, Attrs, Overload)                     \
+  BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_BINARY_PREDICATE(Id, Name, Attrs, Overload)                    \
+  BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_SIL_OPERATION(Id, Name, Overload)                              \
+  case BuiltinValueKind::Id:                                                   \
+    llvm_unreachable("SIL operation must be lowered to instructions.");
+#define BUILTIN_RUNTIME_CALL(Id, Name, Attrs)                                  \
+  case BuiltinValueKind::Id:                                                   \
+    return true; // A runtime call could be anything.
+#define BUILTIN_MISC_OPERATION(Id, Name, Attrs, Overload) BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_SANITIZER_OPERATION(Id, Name, Attrs) BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_TYPE_CHECKER_OPERATION(Id, Name) BUILTIN_NO_BARRIER(Id)
+#define BUILTIN_TYPE_TRAIT_OPERATION(Id, Name) BUILTIN_NO_BARRIER(Id)
+
+#include "swift/AST/Builtins.def"
+    }
   }
+  return false;
 }
 
 // Processes a block bottom-up, keeping a lookout for end_access instructions
@@ -70,7 +123,7 @@ static void processBlock(SILBasicBlock &block) {
       if (!bottomEndAccessInst) {
         bottomEndAccessInst = currEAI;
       }
-    } else if (isBarrier(currIns)) {
+    } else if (isBarrier(&currIns)) {
       LLVM_DEBUG(llvm::dbgs() << "Found a barrier " << currIns
                               << ", clearing last seen end_access\n");
       bottomEndAccessInst = nullptr;

test/SILOptimizer/access_sink.sil

@@ -215,3 +215,68 @@ bb0:
   %ret = tuple ()
   return %ret : $()
 }
+
+// testSinkAfterEscapingClosureCheck:
+// <rdar://problem/45846920> TestFoundation, TestProcess, closure
+// argument passed as @noescape to Objective-C has escaped
+class IntWrapper {
+  var value: Builtin.Int64
+
+  init(v: Builtin.Int64) {
+    value = v
+  }
+}
+
+sil @takesNoEscapeBlockClosure : $@convention(thin) (@guaranteed @convention(block) @noescape () -> ()) -> ()
+
+sil @closureThatModifiesCapture: $@convention(thin) ({ var Builtin.Int64 }) -> ()
+
+sil [reabstraction_thunk] @thunkForCalleeGuaranteed : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> ()
+sil [reabstraction_thunk] @withoutActuallyEscapingThunk : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
+
+// The Copy release cannot be moved below the is_escaping_closure check.
+//
+// CHECK-LABEL: sil @testSinkAfterEscapingClosureCheck : $@convention(thin) (@guaranteed IntWrapper) -> () {
+// CHECK: bb0(%0 : $IntWrapper):
+// CHECK: [[BA:%.*]] = begin_access [read] [dynamic] %{{.*}} : $*Builtin.Int64
+// CHECK: [[COPY:%.*]] = copy_block %{{.*}} : $@convention(block) @noescape () -> ()
+// CHECK: [[F:%.*]] = function_ref @takesNoEscapeBlockClosure : $@convention(thin) (@guaranteed @convention(block) @noescape () -> ()) -> ()
+// CHECK: apply [[F]]([[COPY]]) : $@convention(thin) (@guaranteed @convention(block) @noescape () -> ()) -> ()
+// CHECK: strong_release [[COPY]] : $@convention(block) @noescape () -> ()
+// CHECK: is_escaping_closure
+// CHECK: cond_fail
+// CHECK: end_access [[BA]] : $*Builtin.Int64
+// CHECK-LABEL: } // end sil function 'testSinkAfterEscapingClosureCheck'
+sil @testSinkAfterEscapingClosureCheck : $@convention(thin) (@guaranteed IntWrapper) -> () {
+bb0(%0 : $IntWrapper):
+  %va = ref_element_addr %0 : $IntWrapper, #IntWrapper.value
+  %ba = begin_access [read] [dynamic] %va : $*Builtin.Int64
+  %value = load %ba : $*Builtin.Int64
+  %box = alloc_box ${ var Builtin.Int64 }
+  %boxaddr = project_box %box : ${ var Builtin.Int64 }, 0
+  store %value to %boxaddr : $*Builtin.Int64
+  %closureF = function_ref @closureThatModifiesCapture : $@convention(thin) ({ var Builtin.Int64 }) -> ()
+  %closure = partial_apply [callee_guaranteed] %closureF(%box) : $@convention(thin) ({ var Builtin.Int64 }) -> ()
+  %conv = convert_escape_to_noescape %closure : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> ()
+  %thunk = function_ref @withoutActuallyEscapingThunk : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
+  %pathunk = partial_apply [callee_guaranteed] %thunk(%conv) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
+  %md = mark_dependence %pathunk : $@callee_guaranteed () -> () on %conv : $@noescape @callee_guaranteed () -> ()
+  strong_retain %md : $@callee_guaranteed () -> ()
+  %allocblock = alloc_stack $@block_storage @callee_guaranteed () -> ()
+  %blockaddr = project_block_storage %allocblock : $*@block_storage @callee_guaranteed () -> ()
+  store %md to %blockaddr : $*@callee_guaranteed () -> ()
+  %blockF = function_ref @thunkForCalleeGuaranteed : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> ()
+  %initblock = init_block_storage_header %allocblock : $*@block_storage @callee_guaranteed () -> (), invoke %blockF : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> (), type $@convention(block) @noescape () -> ()
+  %copyblock = copy_block %initblock : $@convention(block) @noescape () -> ()
+  %f = function_ref @takesNoEscapeBlockClosure : $@convention(thin) (@guaranteed @convention(block) @noescape () -> ()) -> ()
+  %call = apply %f(%copyblock) : $@convention(thin) (@guaranteed @convention(block) @noescape () -> ()) -> ()
+  strong_release %copyblock : $@convention(block) @noescape () -> ()
+  %isescape = is_escaping_closure %md : $@callee_guaranteed () -> ()
+  cond_fail %isescape : $Builtin.Int1
+  end_access %ba : $*Builtin.Int64
+  destroy_addr %blockaddr : $*@callee_guaranteed() -> ()
+  dealloc_stack %allocblock : $*@block_storage @callee_guaranteed () -> ()
+  strong_release %box : ${ var Builtin.Int64 }
+  %ret = tuple ()
+  return %ret : $()
+}