[sil] Expand immutable address use verification to in_guaranteed parameters.

include/swift/SIL/SILFunctionConventions.h

@@ -322,6 +322,10 @@ class SILFunctionConventions {
                          SILParameterTypeFunc(silConv)));
   }
 
+  SILYieldInfo getYieldInfoForOperandIndex(unsigned opIndex) const {
+    return getYields()[opIndex];
+  }
+
   //===--------------------------------------------------------------------===//
   // SILArgument API, including indirect results and parameters.
   //

include/swift/SIL/SILInstruction.h

@@ -7031,6 +7031,11 @@ class YieldInst final
   SuccessorListTy getSuccessors() {
     return DestBBs;
   }
+
+  SILYieldInfo getYieldInfoForOperand(const Operand &op) const;
+
+  SILArgumentConvention
+  getArgumentConventionForOperand(const Operand &op) const;
 };
 
 /// BranchInst - An unconditional branch.

lib/SIL/SILInstructions.cpp

@@ -1153,6 +1153,19 @@ YieldInst *YieldInst::create(SILDebugLocation loc,
   return ::new (Buffer) YieldInst(loc, yieldedValues, normalBB, unwindBB);
 }
 
+SILYieldInfo YieldInst::getYieldInfoForOperand(const Operand &op) const {
+  // We expect op to be our operand.
+  assert(op.getUser() == this);
+  auto conv = getFunction()->getConventions();
+  return conv.getYieldInfoForOperandIndex(op.getOperandNumber());
+}
+
+SILArgumentConvention
+YieldInst::getArgumentConventionForOperand(const Operand &op) const {
+  auto conv = getYieldInfoForOperand(op).getConvention();
+  return SILArgumentConvention(conv);
+}
+
 BranchInst *BranchInst::create(SILDebugLocation Loc, SILBasicBlock *DestBB,
                                SILFunction &F) {
   return create(Loc, DestBB, {}, F);

lib/SIL/SILVerifier.cpp

@@ -416,10 +416,7 @@ void verifyKeyPathComponent(SILModule &M,
 struct ImmutableAddressUseVerifier {
   SmallVector<Operand *, 32> worklist;
 
-  bool isConsumingOrMutatingApplyUse(Operand *use) {
-    ApplySite apply(use->getUser());
-    assert(apply && "Not an apply instruction kind");
-    auto conv = apply.getArgumentConvention(*use);
+  bool isConsumingOrMutatingArgumentConvention(SILArgumentConvention conv) {
     switch (conv) {
     case SILArgumentConvention::Indirect_In_Guaranteed:
       return false;
@@ -448,10 +445,24 @@ struct ImmutableAddressUseVerifier {
       return true; // return something "conservative".
     }
     llvm_unreachable("covered switch isn't covered?!");
-  };
+  }
 
-  /// A "copy_addr %src [take] to *" is consuming on "%src".
-  /// A "copy_addr * to * %dst" is mutating on "%dst".
+  bool isConsumingOrMutatingApplyUse(Operand *use) {
+    ApplySite apply(use->getUser());
+    assert(apply && "Not an apply instruction kind");
+    auto conv = apply.getArgumentConvention(*use);
+    return isConsumingOrMutatingArgumentConvention(conv);
+  }
+
+  bool isConsumingOrMutatingYieldUse(Operand *use) {
+    // For now, just say that it is non-consuming for now.
+    auto *yield = cast<YieldInst>(use->getUser());
+    auto conv = yield->getArgumentConventionForOperand(*use);
+    return isConsumingOrMutatingArgumentConvention(conv);
+  }
+
+  // A "copy_addr %src [take] to *" is consuming on "%src".
+  // A "copy_addr * to * %dst" is mutating on "%dst".
   bool isConsumingOrMutatingCopyAddrUse(Operand *use) {
     auto *copyAddr = cast<CopyAddrInst>(use->getUser());
     if (copyAddr->getDest() == use->get())
@@ -462,37 +473,56 @@ struct ImmutableAddressUseVerifier {
   }
 
   bool isCastToNonConsuming(UncheckedAddrCastInst *i) {
-    for (auto *use : i->getUses()) {
+    // Check if any of our uses are consuming. If none of them are consuming, we
+    // are good to go.
+    return llvm::none_of(i->getUses(), [&](Operand *use) -> bool {
       auto *inst = use->getUser();
       switch (inst->getKind()) {
+      default:
+        return false;
       case SILInstructionKind::ApplyInst:
       case SILInstructionKind::TryApplyInst:
       case SILInstructionKind::PartialApplyInst:
-        if (isConsumingOrMutatingApplyUse(use))
-          return false;
-        continue;
-      default:
-        continue;
+      case SILInstructionKind::BeginApplyInst:
+        return isConsumingOrMutatingApplyUse(use);
       }
-    }
-    return true;
+    });
   }
 
   bool isMutatingOrConsuming(SILValue address) {
-    for (auto *use : address->getUses()) {
+    copy(address->getUses(), std::back_inserter(worklist));
+    while (!worklist.empty()) {
+      auto *use = worklist.pop_back_val();
       auto *inst = use->getUser();
       if (inst->isTypeDependentOperand(*use))
         continue;
       switch (inst->getKind()) {
       case SILInstructionKind::MarkDependenceInst:
+      case SILInstructionKind::LoadBorrowInst:
+      case SILInstructionKind::DebugValueAddrInst:
+      case SILInstructionKind::ExistentialMetatypeInst:
+      case SILInstructionKind::ValueMetatypeInst:
+      case SILInstructionKind::FixLifetimeInst:
+      case SILInstructionKind::KeyPathInst:
+      case SILInstructionKind::SwitchEnumAddrInst:
+        break;
+      case SILInstructionKind::AddressToPointerInst:
+        // We assume that the user is attempting to do something unsafe since we
+        // are converting to a raw pointer. So just ignore this use.
+        //
+        // TODO: Can we do better?
         break;
       case SILInstructionKind::ApplyInst:
       case SILInstructionKind::TryApplyInst:
       case SILInstructionKind::PartialApplyInst:
+      case SILInstructionKind::BeginApplyInst:
         if (isConsumingOrMutatingApplyUse(use))
           return true;
-        else
-          break;
+        break;
+      case SILInstructionKind::YieldInst:
+        if (isConsumingOrMutatingYieldUse(use))
+          return true;
+        break;
       case SILInstructionKind::CopyAddrInst:
         if (isConsumingOrMutatingCopyAddrUse(use))
           return true;
@@ -519,11 +549,32 @@ struct ImmutableAddressUseVerifier {
         break;
       case SILInstructionKind::LoadInst:
         // A 'non-taking' value load is harmless.
-        return cast<LoadInst>(inst)->getOwnershipQualifier() ==
-               LoadOwnershipQualifier::Take;
+        if (cast<LoadInst>(inst)->getOwnershipQualifier() ==
+            LoadOwnershipQualifier::Take)
+          return true;
         break;
-      case SILInstructionKind::DebugValueAddrInst:
-        // Harmless use.
+#define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)             \
+  case SILInstructionKind::Load##Name##Inst:                                   \
+    if (cast<Load##Name##Inst>(inst)->isTake())                                \
+      return true;                                                             \
+    break;
+#include "swift/AST/ReferenceStorage.def"
+      case SILInstructionKind::OpenExistentialAddrInst:
+        // If we have a mutable use, return true. Otherwise fallthrough since we
+        // want to look through immutable uses.
+        if (cast<OpenExistentialAddrInst>(inst)->getAccessKind() !=
+            OpenedExistentialAccess::Immutable)
+          return true;
+        LLVM_FALLTHROUGH;
+      case SILInstructionKind::StructElementAddrInst:
+      case SILInstructionKind::TupleElementAddrInst:
+      case SILInstructionKind::IndexAddrInst:
+      case SILInstructionKind::TailAddrInst:
+      case SILInstructionKind::IndexRawPointerInst:
+        // Add these to our worklist.
+        for (auto result : inst->getResults()) {
+          copy(result->getUses(), std::back_inserter(worklist));
+        }
         break;
       default:
         llvm_unreachable("Unhandled unexpected instruction");
@@ -840,7 +891,19 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                     || arg->getParent()->getSinglePredecessorBlock(),
                 "Non-branch terminator must have a unique successor.");
       }
+      return;
     }
+
+    // If we are not in lowered SIL and have an in_guaranteed function argument,
+    // verify that we do not mutate or consume it.
+    auto *fArg = cast<SILFunctionArgument>(arg);
+    if (fArg->getModule().getStage() == SILStage::Lowered ||
+        !fArg->getType().isAddress() ||
+        !fArg->hasConvention(SILArgumentConvention::Indirect_In_Guaranteed))
+      return;
+
+    require(!ImmutableAddressUseVerifier().isMutatingOrConsuming(fArg),
+            "Found mutating or consuming use of an in_guaranteed parameter?!");
   }
 
   void visitSILInstruction(SILInstruction *I) {

lib/SILGen/SILGenPoly.cpp

@@ -1116,9 +1116,13 @@ namespace {
 
           // Load if necessary.
           if (elt.getType().isAddress()) {
+            // This code assumes that we have each element at +1. So, if we do
+            // not have a cleanup, we emit a load [copy]. This can occur if we
+            // are translating in_guaranteed parameters.
+            IsTake_t isTakeVal = elt.isPlusZero() ? IsNotTake : IsTake;
             elt = SGF.emitLoad(Loc, elt.forward(SGF),
-                               SGF.getTypeLowering(elt.getType()),
-                               SGFContext(), IsTake);
+                               SGF.getTypeLowering(elt.getType()), SGFContext(),
+                               isTakeVal);
           }
         }
 

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -813,11 +813,12 @@ static bool canReplaceCopiedArg(FullApplySite Apply, SILValue Arg,
   if (!IEA)
     return false;
 
-  // If the witness method mutates Arg, we cannot replace Arg with
-  // the source of a copy. Otherwise the call would modify another value than
-  // the original argument.
+  // If the witness method does not take the value as guaranteed, we cannot
+  // replace Arg with the source of a copy. Otherwise the call would modify
+  // another value than the original argument (in the case of indirect mutating)
+  // or create a use-after-free (in the case of indirect consuming).
   auto origConv = Apply.getOrigCalleeConv();
-  if (origConv.getParamInfoForSILArg(ArgIdx).isIndirectMutating())
+  if (!origConv.getParamInfoForSILArg(ArgIdx).isIndirectInGuaranteed())
     return false;
 
   auto *DT = DA->get(Apply.getFunction());

test/SILOptimizer/escape_analysis.sil

@@ -362,7 +362,7 @@ bb0(%0: $*Int32, %1: $*Int32):
 // CHECK-NEXT:    Arg %1 Esc: A, Succ: (%1.1)
 // CHECK-NEXT:    Con %1.1 Esc: A, Succ:
 // CHECK-NEXT:  End
-sil @copy_addr_take_content : $@convention(thin) (@in_guaranteed Int32) -> @out Int32 {
+sil @copy_addr_take_content : $@convention(thin) (@in Int32) -> @out Int32 {
 bb0(%0: $*Int32, %1: $*Int32):
   copy_addr [take] %1 to [initialization] %0 : $*Int32
   %2 = tuple()
@@ -375,7 +375,7 @@ bb0(%0: $*Int32, %1: $*Int32):
 // CHECK-NEXT:    Arg %1 Esc: G, Succ: (%1.1)
 // CHECK-NEXT:    Con %1.1 Esc: G, Succ:
 // CHECK-NEXT:  End
-sil @copy_addr_noinit_content : $@convention(thin) (@in_guaranteed Int32) -> @out Int32 {
+sil @copy_addr_noinit_content : $@convention(thin) (@in Int32) -> @out Int32 {
 bb0(%0: $*Int32, %1: $*Int32):
   copy_addr [take] %1 to %0 : $*Int32
   %2 = tuple()

test/SILOptimizer/performance_inliner.sil

@@ -1,4 +1,3 @@
-
 // RUN: %target-sil-opt -enable-sil-verify-all %s -inline -sil-combine | %FileCheck %s
 
 sil_stage canonical
@@ -907,7 +906,6 @@ bb1(%6 : $()):
 
 bb2:
   strong_release %2 : $@callee_owned () -> (@out T, @error Error)
-  destroy_addr %1 : $*T
   %10 = tuple ()
   return %10 : $()
 

test/SILOptimizer/predictable_deadalloc_elim.sil

@@ -212,11 +212,11 @@ bb3:
 }
 
 // We should not promote this due to this being an assign to %2.
-// CHECK-LABEL: sil @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+// CHECK-LABEL: sil @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 // CHECK: alloc_stack
 // CHECK: copy_addr
 // CHECK: } // end sil function 'simple_diamond_with_assign'
-sil @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+sil @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 bb0(%0 : $Builtin.NativeObject, %1 : $*Builtin.NativeObject):
   %2 = alloc_stack $Builtin.NativeObject
   store %0 to %2 : $*Builtin.NativeObject
@@ -241,11 +241,11 @@ bb3:
 // this test shows that we /tried/ and failed with the available value test
 // instead of failing earlier due to the copy_addr being an assign since we
 // explode the copy_addr.
-// CHECK-LABEL: sil @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+// CHECK-LABEL: sil @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 // CHECK: alloc_stack
 // CHECK-NOT: copy_addr
 // CHECK: } // end sil function 'simple_diamond_with_assign_remove'
-sil @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+sil @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 bb0(%0 : $Builtin.NativeObject, %1 : $*Builtin.NativeObject):
   %2 = alloc_stack $Builtin.NativeObject
   store %0 to %2 : $*Builtin.NativeObject

test/SILOptimizer/predictable_deadalloc_elim_ownership.sil

@@ -227,11 +227,11 @@ bb3:
 }
 
 // We should not promote this due to this being an assign to %2.
-// CHECK-LABEL: sil [ossa] @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+// CHECK-LABEL: sil [ossa] @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 // CHECK: alloc_stack
 // CHECK: copy_addr
 // CHECK: } // end sil function 'simple_diamond_with_assign'
-sil [ossa] @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+sil [ossa] @simple_diamond_with_assign : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 bb0(%0 : @owned $Builtin.NativeObject, %1 : $*Builtin.NativeObject):
   %2 = alloc_stack $Builtin.NativeObject
   store %0 to [init] %2 : $*Builtin.NativeObject
@@ -256,11 +256,12 @@ bb3:
 // this test shows that we /tried/ and failed with the available value test
 // instead of failing earlier due to the copy_addr being an assign since we
 // explode the copy_addr.
-// CHECK-LABEL: sil [ossa] @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+//
+// CHECK-LABEL: sil [ossa] @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 // CHECK: alloc_stack
 // CHECK-NOT: copy_addr
 // CHECK: } // end sil function 'simple_diamond_with_assign_remove'
-sil [ossa] @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in_guaranteed Builtin.NativeObject) -> () {
+sil [ossa] @simple_diamond_with_assign_remove : $@convention(thin) (@owned Builtin.NativeObject, @in Builtin.NativeObject) -> () {
 bb0(%0 : @owned $Builtin.NativeObject, %1 : $*Builtin.NativeObject):
   %2 = alloc_stack $Builtin.NativeObject
   store %0 to [init] %2 : $*Builtin.NativeObject

test/SILOptimizer/sil_combine_concrete_existential.sil

@@ -499,3 +499,36 @@ bb0(%0 : $Array<Int>):
   %25 = tuple ()
   return %25 : $()
 }
+
+struct InGuaranteedArgTestNativeObjectWrapper {
+  var i: Builtin.NativeObject
+}
+
+protocol InGuaranteedArgTestProtocol {}
+extension InGuaranteedArgTestNativeObjectWrapper : InGuaranteedArgTestProtocol {}
+
+sil @in_guaranteed_arg_test_callee : $@convention(thin) <τ_0_0 where τ_0_0 : InGuaranteedArgTestProtocol> (@in τ_0_0) -> ()
+
+// The current transformation is not smart enough to remove the
+// destroy_addr. Simply substituting the copy source for the apply argument
+// would introduce a use-after-free. = (.
+//
+// CHECK-LABEL: sil @in_guaranteed_arg_test_caller : $@convention(thin) <τ_0_0 where τ_0_0 : InGuaranteedArgTestProtocol> (@in τ_0_0) -> () {
+// CHECK: apply {{%.*}}<@opened("C494A60E-71EA-11E9-B8C0-D0817AD3F8AD") InGuaranteedArgTestProtocol>
+// CHECK: } // end sil function 'in_guaranteed_arg_test_caller'
+sil @in_guaranteed_arg_test_caller : $@convention(thin) <τ_0_0 where τ_0_0 : InGuaranteedArgTestProtocol> (@in τ_0_0) -> () {
+bb0(%0 : $*τ_0_0):
+  %1 = alloc_stack $InGuaranteedArgTestProtocol
+  %2 = init_existential_addr %1 : $*InGuaranteedArgTestProtocol, $τ_0_0
+  copy_addr [take] %0 to [initialization] %2 : $*τ_0_0
+  %3 = function_ref @in_guaranteed_arg_test_callee : $@convention(thin) <τ_0_0 where τ_0_0 : InGuaranteedArgTestProtocol> (@in τ_0_0) -> ()
+  %4 = alloc_stack $InGuaranteedArgTestProtocol
+  copy_addr %1 to [initialization] %4 : $*InGuaranteedArgTestProtocol
+  %5 = open_existential_addr mutable_access %4 : $*InGuaranteedArgTestProtocol to $*@opened("C494A60E-71EA-11E9-B8C0-D0817AD3F8AD") InGuaranteedArgTestProtocol
+  apply %3<@opened("C494A60E-71EA-11E9-B8C0-D0817AD3F8AD") InGuaranteedArgTestProtocol>(%5) : $@convention(thin) <τ_0_0 where τ_0_0 : InGuaranteedArgTestProtocol> (@in τ_0_0) -> ()
+  dealloc_stack %4 : $*InGuaranteedArgTestProtocol
+  destroy_addr %1 : $*InGuaranteedArgTestProtocol
+  dealloc_stack %1 : $*InGuaranteedArgTestProtocol
+  %9999 = tuple()
+  return %9999 : $()
+}

test/SILOptimizer/sil_combiner_concrete_prop_all_args.sil

@@ -178,7 +178,7 @@ bb0(%0 : $*SomeNoClass):
 sil hidden [signature_optimized_thunk] [always_inline] @$s21existential_transform12wrap_foo_ncp1as5Int32VAA19SomeNoClassProtocol_p_tF : $@convention(thin) (@in_guaranteed SomeNoClassProtocol) -> Int32 {
 bb0(%0 : $*SomeNoClassProtocol):
   %1 = function_ref @$s21existential_transform12wrap_foo_ncp1as5Int32VAA19SomeNoClassProtocol_p_tFTf4e_n : $@convention(thin) <τ_0_0 where τ_0_0 : SomeNoClassProtocol> (@in_guaranteed τ_0_0) -> Int32 
-  %2 = open_existential_addr mutable_access %0 : $*SomeNoClassProtocol to $*@opened("CC97FCD6-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocol 
+  %2 = open_existential_addr immutable_access %0 : $*SomeNoClassProtocol to $*@opened("CC97FCD6-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocol 
   %3 = apply %1<@opened("CC97FCD6-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocol>(%2) : $@convention(thin) <τ_0_0 where τ_0_0 : SomeNoClassProtocol> (@in_guaranteed τ_0_0) -> Int32 
   return %3 : $Int32                              
 } // end sil function '$s21existential_transform12wrap_foo_ncp1as5Int32VAA19SomeNoClassProtocol_p_tF'
@@ -302,7 +302,7 @@ bb0(%0 : $*SomeNoClassComp):
 sil hidden [signature_optimized_thunk] [always_inline] @$s21existential_transform25wrap_no_foo_bar_comp_ncpc1as5Int32VAA23SomeNoClassProtocolComp_AA0j5OtherklmN0p_tF : $@convention(thin) (@in_guaranteed SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp) -> Int32 {
 bb0(%0 : $*SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp):
   %1 = function_ref @$s21existential_transform25wrap_no_foo_bar_comp_ncpc1as5Int32VAA23SomeNoClassProtocolComp_AA0j5OtherklmN0p_tFTf4e_n : $@convention(thin) <τ_0_0 where τ_0_0 : SomeNoClassProtocolComp, τ_0_0 : SomeOtherNoClassProtocolComp> (@in_guaranteed τ_0_0) -> Int32 
-  %2 = open_existential_addr mutable_access %0 : $*SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp to $*@opened("CC983642-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp 
+  %2 = open_existential_addr immutable_access %0 : $*SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp to $*@opened("CC983642-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp 
   %3 = apply %1<@opened("CC983642-AC7C-11E8-B742-D0817AD4059B") SomeNoClassProtocolComp & SomeOtherNoClassProtocolComp>(%2) : $@convention(thin) <τ_0_0 where τ_0_0 : SomeNoClassProtocolComp, τ_0_0 : SomeOtherNoClassProtocolComp> (@in_guaranteed τ_0_0) -> Int32 
   return %3 : $Int32                              
 } // end sil function '$s21existential_transform25wrap_no_foo_bar_comp_ncpc1as5Int32VAA23SomeNoClassProtocolComp_AA0j5OtherklmN0p_tF'