Merge pull request #21745 from gottesmm/pr-7badf1ef083f46a99dde5777619f7f410168e8a5

Author: swift-ci

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()