[silgenpattern] Fix a leak in tuple pattern emission.

Today SILGenPattern maintains the following invariants:

1. All values passed into a switch must be either TakeAlways or BorrowAlways and
   loadable input values will be loaded.

2. Loadable types passed to a subtree must be loaded.

3. TakeOnSuccess can only occur with address only types and only in subtrees.

4. A TakeOnSuccess value must go through "fake borrowing"
   (i.e. forward/unforwarding) to ensure that along failing cases, we properly
   re-enable the cleanup on the aggregate. This means that TakeOnSuccess can
   never be handled as a loadable value with ownership enabled and that any
   take_on_success value since the original cleanup on the parent value was
   disabled must be at +1.

5. We use BorrowAlways instead of TakeOnSuccess for objects to express the
   notion that the object is not owned by the sub-tree.

The bug in this tuple code occured at the a place in the code where we go from
an address only parent type to a loadable subtype via TakeOnSuccess. I was
trying to follow (5) and thus I converted the subvalue to use a {load_borrow,
BorrowAlways}. The problem with this is that I lost the cleanup from the tuple
subvalue since take_on_success is just simulating +0 and thus entails having a
cleanup for each of the underlying tuple values.

The fix here was to:

* Create a cleanup for the loadable subvalue leaving it in memory. This address
  version of the subvalue we use for the purposes of unforwarding. This avoids
  (4).
* load_borrow the subvalue and pass that off to the subtree. This ensures that
  we respect (2), (3), (4).
* Unforward in the failure case the in memory subvalue.

This gives us both characteristics as well as in the future the possibility of
enforcing via the ownership verifier that the borrow ends before the
destroy_addr.

I also sprinkled some assertions liberally to maintain invariants.

rdar://47034816

Author: gottesmm

lib/SILGen/SILGenPattern.cpp

@@ -743,7 +743,7 @@ forwardIntoSubtree(SILGenFunction &SGF, SILLocation loc,
   (void)consumptionKind;
 
   // If we have an object and it is take always, we need to borrow the value
-  // since we do not own the value at this point.
+  // since our subtree does not own the value.
   if (outerMV.getType().isObject()) {
     assert(consumptionKind == CastConsumptionKind::TakeAlways &&
            "Object without cleanup that is not take_always?!");
@@ -1429,45 +1429,59 @@ emitTupleDispatch(ArrayRef<RowToSpecialize> rows, ConsumableManagedValue src,
 
   // If our source is an address that is loadable, perform a load_borrow.
   if (src.getType().isAddress() && src.getType().isLoadable(SGF.getModule())) {
+    // We should only see take_on_success if we have a base type that is address
+    // only.
+    assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
+           "Can only occur if base type is address only?!");
     src = {SGF.B.createLoadBorrow(loc, src.getFinalManagedValue()),
            CastConsumptionKind::BorrowAlways};
   }
 
   // Then if we have an object...
   if (src.getType().isObject()) {
-    // Make sure that if we ahve a copy_on_success, non-trivial value that we do
+    // Make sure that if we have a copy_on_success, non-trivial value that we do
     // not have a value with @owned ownership.
     assert((!src.getType().isTrivial(SGF.getModule()) ||
             src.getFinalConsumption() != CastConsumptionKind::CopyOnSuccess ||
             src.getOwnershipKind() != ValueOwnershipKind::Owned) &&
            "@owned value without cleanup + copy_on_success");
 
-    // If we have are asked to perform TakeOnSuccess, borrow the value instead.
-    //
-    // The reason why do this for TakeOnSuccess is that we want to not have to
-    // deal with unforwarding of aggregate tuples in failing cases since that
-    // causes ownership invariants to be violated since we already forwarded the
-    // aggregate to create cleanups on its elements.
-    //
-    // In contrast, we do still want to allow for TakeAlways variants to not
-    // need to borrow, so we do not borrow if we take always.
-    if (!src.getType().isTrivial(SGF.getModule()) &&
-        src.getFinalConsumption() == CastConsumptionKind::TakeOnSuccess) {
-      src = {src.getFinalManagedValue().borrow(SGF, loc),
-             CastConsumptionKind::BorrowAlways};
-    }
+    // We should only see take_on_success if we have a base type that is address
+    // only.
+    assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
+           "Can only occur if base type is address only?!");
 
     // Then perform a forward or reborrow destructure on the object.
     return emitTupleObjectDispatch(rows, src, handleCase, outerFailure);
   }
 
+  // Construct the specialized rows.
+  SmallVector<SpecializedRow, 4> specializedRows;
+  specializedRows.resize(rows.size());
+  for (unsigned i = 0, e = rows.size(); i != e; ++i) {
+    specializedRows[i].RowIndex = rows[i].RowIndex;
+
+    auto pattern = cast<TuplePattern>(rows[i].Pattern);
+    for (auto &elt : pattern->getElements()) {
+      specializedRows[i].Patterns.push_back(elt.getPattern());
+    }
+  }
+
   // At this point we know that we must have an address only type, since we
   // would have loaded it earlier.
   SILValue v = src.getFinalManagedValue().forward(SGF);
   assert(v->getType().isAddressOnly(SGF.getModule()) &&
          "Loadable values were handled earlier");
 
-  SmallVector<ConsumableManagedValue, 4> destructured;
+  // The destructured tuple that we pass off to our sub pattern. This may
+  // contain values that we have performed a load_borrow from subsequent to
+  // "performing a SILGenPattern borrow".
+  SmallVector<ConsumableManagedValue, 4> subPatternArgs;
+
+  // An array of values that have the same underlying values as our
+  // subPatternArgs, but may have a different cleanup and final consumption
+  // kind. These are at +1 and are unforwarded.
+  SmallVector<ConsumableManagedValue, 4> unforwardArgs;
 
   // Break down the values.
   auto tupleSILTy = v->getType();
@@ -1476,65 +1490,87 @@ emitTupleDispatch(ArrayRef<RowToSpecialize> rows, ConsumableManagedValue src,
     auto &fieldTL = SGF.getTypeLowering(fieldTy);
 
     SILValue member = SGF.B.createTupleElementAddr(loc, v, i, fieldTy);
-    ConsumableManagedValue memberCMV;
+    // Inline constructor.
+    auto memberCMV = ([&]() -> ConsumableManagedValue {
+      if (!fieldTL.isLoadable()) {
+        // If we have an address only type, just get the managed
+        // subobject.
+        return getManagedSubobject(SGF, member, fieldTL,
+                                   src.getFinalConsumption());
+      }
 
-    // If we have a loadable sub-type of our tuple...
-    if (fieldTL.isLoadable()) {
+      // If we have a loadable type, then we have a loadable sub-type of the
+      // underlying address only tuple.
+      auto memberMV = ManagedValue::forUnmanaged(member);
       switch (src.getFinalConsumption()) {
       case CastConsumptionKind::TakeAlways: {
-        // and our consumption is take always, perform a load [take] and
-        // continue.
-        auto memberMV = ManagedValue::forUnmanaged(member);
-        memberCMV = {SGF.B.createLoadTake(loc, memberMV),
-                     CastConsumptionKind::TakeAlways};
-        break;
+        // If our original source value is take always, perform a load [take].
+        return {SGF.B.createLoadTake(loc, memberMV),
+                CastConsumptionKind::TakeAlways};
+      }
+      case CastConsumptionKind::TakeOnSuccess: {
+        // If we have a take_on_success, we propagate down the member as a +1
+        // address value and do not load.
+        //
+        // DISCUSSION: Unforwarding objects violates ownership since
+        // unforwarding relies on forwarding an aggregate into subvalues and
+        // on failure disabling the subvalue cleanups and re-enabling the
+        // cleanup for the aggregate (which was already destroyed). So we are
+        // forced to use an address here so we can forward/unforward this
+        // value. We maintain our invariants that loadable types are always
+        // loaded and are never take on success by passing down to our
+        // subPattern a borrow of this value. See below.
+        return getManagedSubobject(SGF, member, fieldTL,
+                                   src.getFinalConsumption());
       }
-      case CastConsumptionKind::TakeOnSuccess:
       case CastConsumptionKind::CopyOnSuccess: {
-        // otherwise we have take on success or copy on success perform a
-        // load_borrow.
+        // We translate copy_on_success => borrow_always.
         auto memberMV = ManagedValue::forUnmanaged(member);
-        memberCMV = {SGF.B.createLoadBorrow(loc, memberMV),
-                     CastConsumptionKind::BorrowAlways};
-        break;
+        return {SGF.B.createLoadBorrow(loc, memberMV),
+                CastConsumptionKind::BorrowAlways};
       }
-      case CastConsumptionKind::BorrowAlways:
-        llvm_unreachable("Borrow always can not be used on objects");
+      case CastConsumptionKind::BorrowAlways: {
+        llvm_unreachable(
+            "Borrow always can only occur along object only code paths");
       }
-    } else {
-      // Otherwise, if we have an address only type, just get the managed
-      // subobject.
-      memberCMV =
-          getManagedSubobject(SGF, member, fieldTL, src.getFinalConsumption());
-    }
-
-    destructured.push_back(memberCMV);
-  }
-
-  // Construct the specialized rows.
-  SmallVector<SpecializedRow, 4> specializedRows;
-  specializedRows.resize(rows.size());
-  for (unsigned i = 0, e = rows.size(); i != e; ++i) {
-    specializedRows[i].RowIndex = rows[i].RowIndex;
+      }
+    }());
 
-    auto pattern = cast<TuplePattern>(rows[i].Pattern);
-    for (auto &elt : pattern->getElements()) {
-      specializedRows[i].Patterns.push_back(elt.getPattern());
+    // If we aren't loadable, add to the unforward array.
+    if (!fieldTL.isLoadable()) {
+      unforwardArgs.push_back(memberCMV);
+    } else {
+      // If we have a loadable type that we didn't load, we must have had a
+      // take_on_success address. This means that our parent cleanup is
+      // currently persistently active, so we needed to propagate an active +1
+      // cleanup on our address so we can take if we actually succeed. That
+      // being said, we do not want to pass objects with take_on_success into
+      // the actual subtree. So we perform a load_borrow at this point. This
+      // will ensure that we will always finish the end_borrow before we jumped
+      // to a failure point, but at the same time the original +1 value will be
+      // appropriately destroyed/forwarded around.
+      if (memberCMV.getType().isAddress()) {
+        unforwardArgs.push_back(memberCMV);
+        auto val = memberCMV.getFinalManagedValue();
+        memberCMV = {SGF.B.createLoadBorrow(loc, val),
+                     CastConsumptionKind::BorrowAlways};
+      }
     }
+    subPatternArgs.push_back(memberCMV);
   }
 
   // Maybe revert to the original cleanups during failure branches.
   const FailureHandler *innerFailure = &outerFailure;
   FailureHandler specializedFailure = [&](SILLocation loc) {
     ArgUnforwarder unforwarder(SGF);
-    unforwarder.unforwardBorrowedValues(src, destructured);
+    unforwarder.unforwardBorrowedValues(src, unforwardArgs);
     outerFailure(loc);
   };
   if (ArgUnforwarder::requiresUnforwarding(SGF, src))
     innerFailure = &specializedFailure;
 
   // Recurse.
-  handleCase(destructured, specializedRows, *innerFailure);
+  handleCase(subPatternArgs, specializedRows, *innerFailure);
 }
 
 static CanType getTargetType(const RowToSpecialize &row) {
@@ -1564,46 +1600,53 @@ emitCastOperand(SILGenFunction &SGF, SILLocation loc,
     return src;
   }
 
+  // We know that we must have a loadable type at this point since address only
+  // types do not need reabstraction and are addresses. So we should have exited
+  // above already.
+  assert(src.getType().isLoadable(SGF.getModule()) &&
+         "Should have a loadable value at this point");
+
+  // Since our finalValue is loadable, we could not have had a take_on_success
+  // here.
+  assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
+         "Loadable types can not have take_on_success?!");
+
   std::unique_ptr<TemporaryInitialization> init;
   SGFContext ctx;
   if (requiresAddress) {
     init = SGF.emitTemporary(loc, srcAbstractTL);
     ctx = SGFContext(init.get());
   }
 
-  ManagedValue substValue = SGF.getManagedValue(loc, src);
-  ManagedValue finalValue;
+  // This will always produce a +1 take always value no matter what src's
+  // ownership is.
+  ManagedValue finalValue = SGF.getManagedValue(loc, src);
   if (hasAbstraction) {
-    assert(src.getType().isObject() &&
-           "address-only type with abstraction difference?");
-    // Produce the value at +1.
-    finalValue = SGF.emitSubstToOrigValue(loc, substValue,
-                                          abstraction, sourceType, ctx);
-  } else {
-    finalValue = substValue;
-  }
-
-  if (requiresAddress) {
-    if (finalValue.getOwnershipKind() == ValueOwnershipKind::Guaranteed)
-      finalValue = finalValue.copy(SGF, loc);
-    SGF.B.emitStoreValueOperation(loc, finalValue.forward(SGF),
-                                  init->getAddress(),
-                                  StoreOwnershipQualifier::Init);
-    init->finishInitialization(SGF);
-
-    // If we had borrow_always, we need to switch to copy_on_success since
-    // that is the address only variant of borrow_always.
-    auto consumption = src.getFinalConsumption();
-    if (consumption == CastConsumptionKind::BorrowAlways)
-      consumption = CastConsumptionKind::CopyOnSuccess;
-    ConsumableManagedValue result = {init->getManagedAddress(), consumption};
-    if (ArgUnforwarder::requiresUnforwarding(SGF, src))
-      borrowedValues.push_back(result);
-
-    return result;
-  }
-
-  return ConsumableManagedValue::forOwned(finalValue);
+    // Reabstract the value if we need to. This should produce a +1 value as
+    // well.
+    finalValue =
+        SGF.emitSubstToOrigValue(loc, finalValue, abstraction, sourceType, ctx);
+  }
+  assert(finalValue.isPlusOne(SGF));
+
+  // If we at this point do not require an address, return final value. We know
+  // that it is a +1 take always value.
+  if (!requiresAddress) {
+    return ConsumableManagedValue::forOwned(finalValue);
+  }
+
+  // At this point, we know that we have a non-address only type since we are
+  // materializing an object into memory and addresses can not be stored into
+  // memory.
+  SGF.B.emitStoreValueOperation(loc, finalValue.forward(SGF),
+                                init->getAddress(),
+                                StoreOwnershipQualifier::Init);
+  init->finishInitialization(SGF);
+
+  // We know that either our initial value was already take_always or we made a
+  // copy of the underlying value. In either case, we now have a take_always +1
+  // value.
+  return ConsumableManagedValue::forOwned(init->getManagedAddress());
 }
 
 /// Perform specialized dispatch for a sequence of IsPatterns.
@@ -1938,27 +1981,18 @@ void PatternMatchEmission::emitEnumElementDispatch(
 
   // If our source is an address that is loadable, perform a load_borrow.
   if (src.getType().isAddress() && src.getType().isLoadable(SGF.getModule())) {
+    assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
+           "Can only have take_on_success with address only values");
     src = {SGF.B.createLoadBorrow(loc, src.getFinalManagedValue()),
            CastConsumptionKind::BorrowAlways};
   }
 
   // If we have an object...
   if (src.getType().isObject()) {
-    // And we have a non-trivial object type that we are asked to perform take
-    // on success for, borrow the value instead.
-    //
-    // The reason why do this for TakeOnSuccess is that we want to not have to
-    // deal with unforwarding of aggregate tuples in failing cases since that
-    // causes ownership invariants to be violated since we already forwarded the
-    // aggregate to create cleanups on its elements.
-    //
-    // In contrast, we do still want to allow for TakeAlways variants to not
-    // need to borrow, so we do not borrow if we take always.
-    if (!src.getType().isTrivial(SGF.getModule()) &&
-        src.getFinalConsumption() == CastConsumptionKind::TakeOnSuccess) {
-      src = {src.getFinalManagedValue().borrow(SGF, loc),
-             CastConsumptionKind::BorrowAlways};
-    }
+    // Do a quick assert that we do not have take_on_success. This should only
+    // be passed take_on_success if src is an address only type.
+    assert(src.getFinalConsumption() != CastConsumptionKind::TakeOnSuccess &&
+           "Can only have take_on_success with address only values");
 
     // Finally perform the enum element dispatch.
     return emitEnumElementObjectDispatch(rows, src, handleCase, outerFailure,

test/Interpreter/switch.swift

@@ -168,4 +168,58 @@ SwitchTestSuite.test("GenericVar") {
   expectTrue(l === Gesture.pinch(l).valueVar as! LifetimeTracked)
 }
 
+SwitchTestSuite.test("TupleUnforwarding") {
+  // None of these switches should leak.
+  do {
+    let l = LifetimeTracked(0)
+    let r: Optional<Any> = LifetimeTracked(0) as Any
+
+    switch (l, r) {
+    case (_, _):
+      break
+    default:
+      break
+    }
+  }
+
+  do {
+    let l = LifetimeTracked(0)
+    let r: Optional<Any> = LifetimeTracked(0) as Any
+    switch (l, r) {
+    case let (x, _):
+      break
+    case let (_, y as AnyObject):
+      break
+    default:
+      break
+    }
+  }
+
+  do {
+    let l: Optional<LifetimeTracked> = LifetimeTracked(0)
+    let r: Optional<Any> = LifetimeTracked(0) as Any
+    switch (l, r) {
+    case let (_, _):
+      break
+    case let (_, y as AnyObject):
+      break
+    default:
+      break
+    }
+  }
+
+  do {
+    let l = LifetimeTracked(0)
+    let r: Optional<Any> = LifetimeTracked(0) as Any
+    switch (l, r) {
+    case let (_, y as AnyObject):
+      break
+    case let (x as AnyObject, _):
+      break
+    default:
+      break
+    }
+  }
+}
+
 runAllTests()