Merge pull request #65644 from atrick/5.9-fix-address-consume

[5.9] Fix ManagedValue::isPlusOne for addresses

Author: atrick

lib/SILGen/ManagedValue.cpp

@@ -155,22 +155,22 @@ SILValue ManagedValue::forward(SILGenFunction &SGF) const {
 
 void ManagedValue::forwardInto(SILGenFunction &SGF, SILLocation loc,
                                SILValue address) {
-  assert(isPlusOne(SGF));
+  assert(isPlusOneOrTrivial(SGF));
   auto &addrTL = SGF.getTypeLowering(address->getType());
   SGF.emitSemanticStore(loc, forward(SGF), address, addrTL, IsInitialization);
 }
 
 void ManagedValue::assignInto(SILGenFunction &SGF, SILLocation loc,
                               SILValue address) {
-  assert(isPlusOne(SGF));
+  assert(isPlusOneOrTrivial(SGF));
   auto &addrTL = SGF.getTypeLowering(address->getType());
   SGF.emitSemanticStore(loc, forward(SGF), address, addrTL,
                         IsNotInitialization);
 }
 
 void ManagedValue::forwardInto(SILGenFunction &SGF, SILLocation loc,
                                Initialization *dest) {
-  assert(isPlusOne(SGF));
+  assert(isPlusOneOrTrivial(SGF));
   dest->copyOrInitValueInto(SGF, loc, *this, /*isInit*/ true);
   dest->finishInitialization(SGF);
 }
@@ -281,7 +281,7 @@ ManagedValue ManagedValue::ensurePlusOne(SILGenFunction &SGF,
   if (isa<SILUndef>(getValue()))
     return *this;
 
-  if (!isPlusOne(SGF)) {
+  if (!isPlusOneOrTrivial(SGF)) {
     return copy(SGF, loc);
   }
   return *this;
@@ -293,19 +293,20 @@ bool ManagedValue::isPlusOne(SILGenFunction &SGF) const {
   if (isa<SILUndef>(getValue()))
     return true;
 
-  // Ignore trivial values since for our purposes they are always at +1 since
-  // they can always be passed to +1 APIs.
-  if (getType().isTrivial(SGF.F))
-    return true;
-
-  // If we have an object and the object has any ownership, the same
-  // property applies.
+  // A value without ownership can always be passed to +1 APIs.
+  //
+  // This is not true for address types because deinitializing an in-memory
+  // value invalidates the storage.
   if (getType().isObject() && getOwnershipKind() == OwnershipKind::None)
     return true;
 
   return hasCleanup();
 }
 
+bool ManagedValue::isPlusOneOrTrivial(SILGenFunction &SGF) const {
+  return getType().isTrivial(SGF.F) || isPlusOne(SGF);
+}
+
 bool ManagedValue::isPlusZero() const {
   // SILUndef can always be passed to +0 APIs.
   if (isa<SILUndef>(getValue()))

lib/SILGen/ManagedValue.h

@@ -217,19 +217,34 @@ class ManagedValue {
     return !hasCleanup();
   }
 
-  /// Returns true if this is an managed value that can be used safely as a +1
-  /// managed value.
+  /// Returns true if this managed value can be consumed.
   ///
-  /// This returns true iff:
+  /// This is true if either this value has a cleanup or if it is a trivial
+  /// object value. For address values, this returns true only if the value has
+  /// a cleanup regardless of whether the type is trivial.
   ///
-  /// 1. All sub-values are trivially typed.
-  /// 2. There exists at least one non-trivial typed sub-value and all such
-  /// sub-values all have cleanups.
+  /// When an object value is trivial, it can be passed to a consuming operation
+  /// without destroying it. Consuming a value by address, however, always
+  /// deinitializes the memory regardless of whether or not it is trivial.
   ///
-  /// *NOTE* Due to 1. isPlusOne and isPlusZero both return true for managed
-  /// values consisting of only trivial values.
+  /// Use this before emitting an operation that "takes" this value or passing
+  /// this value to a call that consumes the argument.
   bool isPlusOne(SILGenFunction &SGF) const;
 
+  /// Returns true if this managed value can be forwarded without necessarilly
+  /// destroying the original.
+  ///
+  /// This is true if either isPlusOne is true or the value is trivial. Unlike
+  /// isPlusOne(), this returns true for trivial address values regardless of
+  /// whether the value has a cleanup. A +1 value can be created from a trivial
+  /// value without consuming the original.
+  ///
+  /// Use this when storing this value into a new location simply by forwarding
+  /// the cleanup without destroying the original value. If it's necessary to
+  /// "take" or otherwise immediately consume the original value, then use
+  /// isPlusOne() instead.
+  bool isPlusOneOrTrivial(SILGenFunction &SGF) const;
+
   /// Returns true if this is an ManagedValue that can be used safely as a +0
   /// ManagedValue.
   ///

lib/SILGen/RValue.cpp

@@ -550,7 +550,7 @@ SILValue RValue::forwardAsSingleStorageValue(SILGenFunction &SGF,
 void RValue::forwardInto(SILGenFunction &SGF, SILLocation loc, 
                          Initialization *I) && {
   assert(isComplete() && "rvalue is not complete");
-  assert(isPlusOne(SGF) && "Can not forward borrowed RValues");
+  assert(isPlusOneOrTrivial(SGF) && "Can not forward borrowed RValues");
   ArrayRef<ManagedValue> elts = values;
   copyOrInitValuesInto<ImplodeKind::Forward>(I, elts, type, loc, SGF);
 }
@@ -588,7 +588,7 @@ static void assignRecursive(SILGenFunction &SGF, SILLocation loc,
 void RValue::assignInto(SILGenFunction &SGF, SILLocation loc,
                         SILValue destAddr) && {
   assert(isComplete() && "rvalue is not complete");
-  assert(isPlusOne(SGF) && "Can not assign borrowed RValues");
+  assert(isPlusOneOrTrivial(SGF) && "Can not assign borrowed RValues");
   ArrayRef<ManagedValue> srcValues = values;
   assignRecursive(SGF, loc, type, srcValues, destAddr);
   assert(srcValues.empty() && "didn't claim all elements!");
@@ -734,7 +734,7 @@ RValue RValue::copy(SILGenFunction &SGF, SILLocation loc) const & {
 }
 
 RValue RValue::ensurePlusOne(SILGenFunction &SGF, SILLocation loc) && {
-  if (!isPlusOne(SGF))
+  if (!isPlusOneOrTrivial(SGF))
     return copy(SGF, loc);
   return std::move(*this);
 }
@@ -751,7 +751,8 @@ RValue RValue::borrow(SILGenFunction &SGF, SILLocation loc) const & {
 }
 
 ManagedValue RValue::materialize(SILGenFunction &SGF, SILLocation loc) && {
-  assert(isPlusOne(SGF) && "Can not materialize a non-plus one RValue");
+  assert(isPlusOneOrTrivial(SGF) &&
+         "Can not materialize a non-plus one RValue");
   auto &paramTL = SGF.getTypeLowering(getType());
 
   // If we're already materialized, we're done.
@@ -823,6 +824,13 @@ bool RValue::isPlusOne(SILGenFunction &SGF) const & {
       values, [&SGF](ManagedValue mv) -> bool { return mv.isPlusOne(SGF); });
 }
 
+bool RValue::isPlusOneOrTrivial(SILGenFunction &SGF) const & {
+  return llvm::all_of(
+      values, [&SGF](ManagedValue mv) -> bool {
+        return mv.isPlusOneOrTrivial(SGF);
+      });
+}
+
 bool RValue::isPlusZero(SILGenFunction &SGF) const & {
   return llvm::none_of(values,
                        [](ManagedValue mv) -> bool { return mv.isPlusZero(); });

lib/SILGen/RValue.h

@@ -242,18 +242,24 @@ class RValue {
     return value;
   }
 
-  /// Returns true if this is an rvalue that can be used safely as a +1 rvalue.
+  /// Returns true if this rvalue can be consumed.
   ///
-  /// This returns true iff:
+  /// This is true if each element either has a cleanup or is an SSA value
+  /// without ownership.
   ///
-  /// 1. All sub-values are trivially typed.
-  /// 2. There exists at least one non-trivial typed sub-value and all such
-  /// sub-values all have cleanups.
-  ///
-  /// *NOTE* Due to 1. isPlusOne and isPlusZero both return true for rvalues
-  /// consisting of only trivial values.
+  /// When an SSA value does not have ownership, it can be used by a consuming
+  /// operation without destroying it. Consuming a value by address, however,
+  /// deinitializes the memory regardless of whether the value has ownership.
   bool isPlusOne(SILGenFunction &SGF) const &;
 
+  /// Returns true if this rvalue can be forwarded without necessarilly
+  /// destroying the original.
+  ///
+  /// This is true if either isPlusOne is true or the value is trivial. A
+  /// trivial value in memory can be forwarded as a +1 value without
+  /// deinitializing the memory.
+  bool isPlusOneOrTrivial(SILGenFunction &SGF) const &;
+
   /// Returns true if this is an rvalue that can be used safely as a +0 rvalue.
   ///
   /// Specifically, we return true if:

lib/SILGen/SILGenConvert.cpp

@@ -999,7 +999,7 @@ ManagedValue SILGenFunction::manageOpaqueValue(ManagedValue value,
                                                SGFContext C) {
   // If the opaque value is consumable, we can just return the
   // value with a cleanup. There is no need to retain it separately.
-  if (value.isPlusOne(*this))
+  if (value.isPlusOneOrTrivial(*this))
     return value;
 
   // If the context wants a +0 value, guaranteed or immediate, we can

lib/SILGen/SILGenDecl.cpp

@@ -145,7 +145,8 @@ void TupleInitialization::copyOrInitValueInto(SILGenFunction &SGF,
     // In the address case, we forward the underlying value and store it
     // into memory and then create a +1 cleanup. since we assume here
     // that we have a +1 value since we are forwarding into memory.
-    assert(value.isPlusOne(SGF) && "Can not store a +0 value into memory?!");
+    assert(value.isPlusOneOrTrivial(SGF) &&
+           "Can not store a +0 value into memory?!");
     CleanupCloner cloner(SGF, value);
     SILValue v = value.forward(SGF);
 

lib/SILGen/SILGenLValue.cpp

@@ -4679,7 +4679,7 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
             emitLoad(loc, projection.getValue(), origFormalType,
                      substFormalType, rvalueTL, C, IsNotTake, isBaseGuaranteed);
       } else if (isReadAccessResultOwned(src.getAccessKind()) &&
-          !projection.isPlusOne(*this)) {
+          !projection.isPlusOneOrTrivial(*this)) {
 
         // Before we copy, if we have a move only wrapped value, unwrap the
         // value using a guaranteed moveonlywrapper_to_copyable.

lib/SILGen/SILGenPack.cpp

@@ -860,14 +860,14 @@ SILGenFunction::emitPackTransform(SILLocation loc,
                                   CanPackType outputFormalPackType,
                                   unsigned outputComponentIndex,
                                   bool isSimpleProjection,
-                                  bool outputIsPlusOne,
+                                  bool canForwardOutput,
                llvm::function_ref<ManagedValue(ManagedValue input,
                                                SILType outputEltTy,
                                                SGFContext context)> emitBody) {
 
   // This is an inherent limitation of the representation; we need pack
   // coroutines to get around it.
-  assert((isSimpleProjection || outputIsPlusOne) &&
+  assert((isSimpleProjection || canForwardOutput) &&
          "we cannot support complex transformations that yield borrows");
 
   CleanupCloner inputCloner(*this, inputPackMV);
@@ -890,7 +890,7 @@ SILGenFunction::emitPackTransform(SILLocation loc,
                       {&inputEltTy, &outputEltTy});
 
   auto &outputEltTL = getTypeLowering(outputEltTy);
-  bool outputNeedsCleanup = (outputIsPlusOne && !outputEltTL.isTrivial());
+  bool outputNeedsCleanup = (canForwardOutput && !outputEltTL.isTrivial());
 
   // If the transformation is not a simple projection, we need to
   // create a tuple to hold the transformed values.
@@ -947,10 +947,10 @@ SILGenFunction::emitPackTransform(SILLocation loc,
     // Apply the transform.
     ManagedValue outputElt =
       emitBody(inputElt, outputEltTy,
-               outputIsPlusOne ? SGFContext(outputEltInit.get())
+               canForwardOutput ? SGFContext(outputEltInit.get())
                                : SGFContext::AllowGuaranteedPlusZero);
-    assert(outputIsPlusOne == (outputElt.isInContext() ||
-                               outputElt.isPlusOne(*this)) &&
+    assert(canForwardOutput == (outputElt.isInContext() ||
+                               outputElt.isPlusOneOrTrivial(*this)) &&
            "transformation produced a value of the wrong ownership");
     assert((outputElt.isInContext() ||
             outputElt.getType() == outputEltTy) &&
@@ -991,7 +991,7 @@ SILGenFunction::emitPackTransform(SILLocation loc,
                                                   outputComponentIndex,
                                                   /*limit*/ SILValue());
     return ManagedValue::forOwnedAddressRValue(outputPackAddr, cleanup);
-  } else if (outputIsPlusOne) {
+  } else if (canForwardOutput) {
     return ManagedValue::forTrivialAddressRValue(outputPackAddr);
   } else {
     return ManagedValue::forBorrowedAddressRValue(outputPackAddr);

lib/SILGen/SILGenPattern.cpp

@@ -1679,7 +1679,7 @@ emitCastOperand(SILGenFunction &SGF, SILLocation loc,
     finalValue =
         SGF.emitSubstToOrigValue(loc, finalValue, abstraction, sourceType, ctx);
   }
-  assert(finalValue.isPlusOne(SGF));
+  assert(finalValue.isPlusOneOrTrivial(SGF));
 
   // If we at this point do not require an address, return final value. We know
   // that it is a +1 take always value.

lib/SILGen/SILGenProlog.cpp

@@ -295,7 +295,7 @@ class EmitBBArguments : public CanTypeVisitor<EmitBBArguments,
     }
 
     if (emitInto) {
-      if (mv.isPlusOne(SGF))
+      if (mv.isPlusOneOrTrivial(SGF))
         mv.forwardInto(SGF, loc, emitInto);
       else
         mv.copyInto(SGF, loc, emitInto);

lib/SILGen/Scope.cpp

@@ -77,7 +77,8 @@ static void lifetimeExtendAddressOnlyRValueSubValues(
 
 RValue Scope::popPreservingValue(RValue &&rv) {
   auto &SGF = cleanups.SGF;
-  assert(rv.isPlusOne(SGF) && "Can only push plus one rvalues through a scope");
+  assert(rv.isPlusOneOrTrivial(SGF) &&
+         "Can only push plus one rvalues through a scope");
 
   // Perform a quick check if we have an incontext value. If so, just pop and
   // return rv.

test/SILGen/protocol_enum_witness.swift

@@ -1,5 +1,5 @@
 // RUN: %target-swift-emit-silgen %s | %FileCheck %s
-// RUN: %target-swift-emit-silgen %s -enable-library-evolution
+// RUN: %target-swift-emit-silgen %s -enable-library-evolution | %FileCheck %s --check-prefix=CHECK-LIB
 
 public protocol Foo {
   static var button: Self { get }
@@ -55,3 +55,27 @@ enum InternalEnumWithPublicStruct : Foo {
 
 // CHECK-LABEL: sil_witness_table [serialized] AnotherBar: AnotherFoo module protocol_enum_witness {
 // CHECK: method #AnotherFoo.bar: <Self where Self : AnotherFoo> (Self.Type) -> (Int) -> Self : @$s21protocol_enum_witness10AnotherBarOAA0D3FooA2aDP3bar3argxSi_tFZTW
+
+// -----------------------------------------------------------------------------
+// rdar://108001491 (SIL verification failed: Found mutating or consuming use of
+//                  an in_guaranteed parameter?!:
+//                  !ImmutableAddressUseVerifier().isMutatingOrConsuming(fArg))
+
+public struct EntityIdentifier {
+  public let value: Swift.UInt64
+}
+
+public protocol ObjectProtocol {
+  static func entityReference(_ id: EntityIdentifier) -> Self
+}
+
+public enum Object : ObjectProtocol {
+  case entityReference(EntityIdentifier)
+}
+
+// CHECK-LIB-LABEL: sil private [transparent] [thunk] [ossa] @$s21protocol_enum_witness6ObjectOAA0D8ProtocolA2aDP15entityReferenceyxAA16EntityIdentifierVFZTW : $@convention(witness_method: ObjectProtocol) (@in_guaranteed EntityIdentifier, @thick Object.Type) -> @out Object {
+// CHECK-LIB: bb0(%0 : $*Object, %1 : $*EntityIdentifier, %2 : $@thick Object.Type):
+// CHECK-LIB: [[TEMP:%.*]] = alloc_stack $EntityIdentifier
+// CHECK-LIB: copy_addr %1 to [init] %3 : $*EntityIdentifier
+// CHECK-LIB: apply %{{.*}}(%0, [[TEMP]], %{{.*}}) : $@convention(method) (@in EntityIdentifier, @thin Object.Type) -> @out Object
+// CHECK-LIB-LABEL: } // end sil function '$s21protocol_enum_witness6ObjectOAA0D8ProtocolA2aDP15entityReferenceyxAA16EntityIdentifierVFZTW'