Merge pull request swiftlang#20367 from atrick/fix-access-sink

Fix AccessEnforcementReleaseSinking. Check for illegal cases.

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 : $()
+}