Merge pull request #39537 from eeckstein/fix-memory-lifetime-verifier-5.5

[5.5] Fix two false verification errors in the MemoryLifetimeVerifier

Author: eeckstein

include/swift/SIL/MemoryLocations.h

@@ -43,9 +43,9 @@ void dumpBits(const SmallBitVector &bits);
 ///
 /// Memory locations are limited to addresses which are guaranteed to
 /// be not aliased, like @in/inout parameters and alloc_stack.
-/// Currently only a certain set of address instructions are supported:
-/// Specifically those instructions which are going to be included when SIL
-/// supports opaque values.
+/// Currently only a certain set of address instructions are supported, for
+/// details see `MemoryLocations::analyzeLocationUsesRecursively` and
+/// `MemoryLocations::analyzeAddrProjection`.
 class MemoryLocations {
 public:
 
@@ -288,7 +288,7 @@ class MemoryLocations {
   /// not covered by sub-fields.
   const Bits &getNonTrivialLocations();
 
-  /// Debug dump the MemoryLifetime internals.
+  /// Debug dump the MemoryLocations internals.
   void dump() const;
 
 private:

include/swift/SIL/SILType.h

@@ -294,6 +294,10 @@ class SILType {
   /// even though they are technically trivial.
   bool isTrivial(const SILFunction &F) const;
 
+  /// True if the type is an empty tuple or an empty struct or a tuple or
+  /// struct containing only empty types.
+  bool isEmpty(const SILFunction &F) const;
+
   /// True if the type, or the referenced type of an address type, is known to
   /// be a scalar reference-counted type such as a class, box, or thick function
   /// type. Returns false for non-trivial aggregates.

lib/SIL/IR/SILType.cpp

@@ -104,6 +104,30 @@ bool SILType::isTrivial(const SILFunction &F) const {
   return F.getTypeLowering(contextType).isTrivial();
 }
 
+bool SILType::isEmpty(const SILFunction &F) const {
+  if (auto tupleTy = getAs<TupleType>()) {
+    // A tuple is empty if it either has no elements or if all elements are
+    // empty.
+    for (unsigned idx = 0, num = tupleTy->getNumElements(); idx < num; ++idx) {
+      if (!getTupleElementType(idx).isEmpty(F))
+        return false;
+    }
+    return true;
+  }
+  if (StructDecl *structDecl = getStructOrBoundGenericStruct()) {
+    // Also, a struct is empty if it either has no fields or if all fields are
+    // empty.
+    SILModule &module = F.getModule();
+    TypeExpansionContext typeEx = F.getTypeExpansionContext();
+    for (VarDecl *field : structDecl->getStoredProperties()) {
+      if (!getFieldType(field, module, typeEx).isEmpty(F))
+        return false;
+    }
+    return true;
+  }
+  return false;
+}
+
 bool SILType::isReferenceCounted(SILModule &M) const {
   return M.Types.getTypeLowering(*this,
                                  TypeExpansionContext::minimal())

lib/SIL/Utils/MemoryLocations.cpp

@@ -67,34 +67,6 @@ static bool allUsesInSameBlock(AllocStackInst *ASI) {
   return numDeallocStacks == 1;
 }
 
-/// We don't handle empty tuples and empty structs.
-///
-/// Locations with empty types don't even need a store to count as
-/// "initialized". We don't handle such cases.
-static bool isEmptyType(SILType ty, SILFunction *function) {
-  if (auto tupleTy = ty.getAs<TupleType>()) {
-    // A tuple is empty if it either has no elements or if all elements are
-    // empty.
-    for (unsigned idx = 0, num = tupleTy->getNumElements(); idx < num; ++idx) {
-      if (!isEmptyType(ty.getTupleElementType(idx), function))
-        return false;
-    }
-    return true;
-  }
-  if (StructDecl *structDecl = ty.getStructOrBoundGenericStruct()) {
-    // Also, a struct is empty if it either has no fields or if all fields are
-    // empty.
-    SILModule &module = function->getModule();
-    TypeExpansionContext typeEx = function->getTypeExpansionContext();
-    for (VarDecl *field : structDecl->getStoredProperties()) {
-      if (!isEmptyType(ty.getFieldType(field, module, typeEx), function))
-        return false;
-    }
-    return true;
-  }
-  return false;
-}
-
 } // anonymous namespace
 } // namespace swift
 
@@ -117,7 +89,7 @@ MemoryLocations::Location::Location(SILValue val, unsigned index, int parentIdx)
 
 void MemoryLocations::Location::updateFieldCounters(SILType ty, int increment) {
   SILFunction *function = representativeValue->getFunction();
-  if (!isEmptyType(ty, function)) {
+  if (!ty.isEmpty(*function)) {
     numFieldsNotCoveredBySubfields += increment;
     if (!ty.isTrivial(*function))
       numNonTrivialFieldsNotCovered += increment;
@@ -215,7 +187,11 @@ void MemoryLocations::analyzeLocation(SILValue loc) {
   if (!handleTrivialLocations && loc->getType().isTrivial(*function))
     return;
 
-  if (isEmptyType(loc->getType(), function))
+  /// We don't handle empty tuples and empty structs.
+  ///
+  /// Locations with empty types don't even need a store to count as
+  /// "initialized". We don't handle such cases.
+  if (loc->getType().isEmpty(*function))
     return;
 
   unsigned currentLocIdx = locations.size();
@@ -393,7 +369,7 @@ bool MemoryLocations::analyzeAddrProjection(
     SingleValueInstruction *projection, unsigned parentLocIdx,unsigned fieldNr,
     SmallVectorImpl<SILValue> &collectedVals, SubLocationMap &subLocationMap) {
 
-  if (isEmptyType(projection->getType(), projection->getFunction()))
+  if (projection->getType().isEmpty(*projection->getFunction()))
     return false;
 
   auto key = std::make_pair(parentLocIdx, fieldNr);

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -31,11 +31,8 @@ namespace {
 /// A utility for verifying memory lifetime.
 ///
 /// The MemoryLifetime utility checks the lifetime of memory locations.
-/// This is limited to memory locations which are guaranteed to be not aliased,
-/// like @in or @inout parameters. Also, alloc_stack locations are handled.
-///
-/// In addition to verification, the MemoryLifetime class can be used as utility
-/// (e.g. base class) for optimizations, which need to compute memory lifetime.
+/// This is limited to memory locations which can be handled by
+/// `MemoryLocations`.
 class MemoryLifetimeVerifier {
 
   using Bits = MemoryLocations::Bits;
@@ -178,6 +175,22 @@ static bool isTrivialEnumElem(EnumElementDecl *elem, SILType enumType,
         enumType.getEnumElementType(elem, function).isTrivial(*function);
 }
 
+static bool injectsNoPayloadCase(InjectEnumAddrInst *IEAI) {
+  if (!IEAI->getElement()->hasAssociatedValues())
+    return true;
+  SILType enumType = IEAI->getOperand()->getType();
+  SILFunction *function = IEAI->getFunction();
+  SILType elemType = enumType.getEnumElementType(IEAI->getElement(), function);
+  // Handle empty types (e.g. the empty tuple) as no-payload.
+  return elemType.isEmpty(*function);
+}
+
+static bool isOrHasEnum(SILType type) {
+  return type.getASTType().findIf([](Type ty) {
+    return ty->getEnumOrBoundGenericEnum() != nullptr;
+  });
+}
+
 bool MemoryLifetimeVerifier::storesTrivialEnum(int locIdx,
                         SILBasicBlock::reverse_iterator start,
                         SILBasicBlock::reverse_iterator end) {
@@ -191,7 +204,7 @@ bool MemoryLifetimeVerifier::storesTrivialEnum(int locIdx,
     if (auto *SI = dyn_cast<StoreInst>(&inst)) {
       const Location *loc = locations.getLocation(SI->getDest());
       if (loc && loc->isSubLocation(locIdx) &&
-          SI->getSrc()->getType().getEnumOrBoundGenericEnum()) {
+          isOrHasEnum(SI->getSrc()->getType())) {
         return SI->getOwnershipQualifier() == StoreOwnershipQualifier::Trivial;
       }
     }
@@ -343,7 +356,7 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
       case SILInstructionKind::InjectEnumAddrInst: {
         auto *IEAI = cast<InjectEnumAddrInst>(&I);
         int enumIdx = locations.getLocationIdx(IEAI->getOperand());
-        if (enumIdx >= 0 && !IEAI->getElement()->hasAssociatedValues()) {
+        if (enumIdx >= 0 && injectsNoPayloadCase(IEAI)) {
           // This is a bit tricky: an injected no-payload case means that the
           // "full" enum is initialized. So, for the purpose of dataflow, we
           // treat it like a full initialization of the payload data.
@@ -582,7 +595,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
       case SILInstructionKind::InjectEnumAddrInst: {
         auto *IEAI = cast<InjectEnumAddrInst>(&I);
         int enumIdx = locations.getLocationIdx(IEAI->getOperand());
-        if (enumIdx >= 0 && !IEAI->getElement()->hasAssociatedValues()) {
+        if (enumIdx >= 0 && injectsNoPayloadCase(IEAI)) {
           // Again, an injected no-payload case is treated like a "full"
           // initialization. See initDataflowInBlock().
           requireBitsClear(bits & nonTrivialLocations, IEAI->getOperand(), &I);

test/SIL/memory_lifetime.sil

@@ -464,6 +464,17 @@ bb0(%0 : @owned $T):
   return %r : $()
 }
 
+sil [ossa] @test_store_enum_tuple : $@convention(thin) (Int) -> () {
+bb0(%0 : $Int):
+  %2 = enum $Optional<T>, #Optional.none!enumelt
+  %3 = tuple (%0 : $Int, %2 : $Optional<T>)
+  %8 = alloc_stack $(Int, Optional<T>)
+  store %3 to [trivial] %8 : $*(Int, Optional<T>)
+  dealloc_stack %8 : $*(Int, Optional<T>)
+  %13 = tuple ()
+  return %13 : $()
+}
+
 sil [ossa] @test_select_enum_addr : $@convention(thin) (@in_guaranteed Optional<T>) -> Builtin.Int1 {
 bb0(%0 : $*Optional<T>):
   %1 = integer_literal $Builtin.Int1, -1
@@ -639,3 +650,35 @@ bb0(%0 : $Int, %1 : @guaranteed $@callee_guaranteed (@in_guaranteed (Int, ((), (
   dealloc_stack %2 : $*(Int, ((), ()))
   return %5 : $Int
 }
+
+enum Result<T1, T2>{
+  case success(T1)
+  case failure(T2)
+}
+
+sil @try_get_error : $@convention(thin) () -> @error Error
+
+sil [ossa] @test_init_enum_empty_case : $@convention(thin) () -> @error Error {
+bb0:
+  %0 = alloc_stack $Result<(), Error>
+  %1 = function_ref @try_get_error : $@convention(thin) () -> @error Error
+  try_apply %1() : $@convention(thin) () -> @error Error, normal bb1, error bb2
+
+bb1(%3 : $()):
+  inject_enum_addr %0 : $*Result<(), Error>, #Result.success!enumelt
+  br bb3
+
+bb2(%6 : @owned $Error):
+  %7 = init_enum_data_addr %0 : $*Result<(), Error>, #Result.failure!enumelt
+  store %6 to [init] %7 : $*Error
+  inject_enum_addr %0 : $*Result<(), Error>, #Result.failure!enumelt
+  br bb3
+
+bb3:
+  %11 = load [take] %0 : $*Result<(), Error>
+  destroy_value %11 : $Result<(), Error>
+  dealloc_stack %0 : $*Result<(), Error>
+  %14 = tuple ()
+  return %14 : $()
+}
+

test/SIL/memory_lifetime_failures.sil

@@ -538,3 +538,66 @@ bb0(%0 : $Bool):
   %10 = tuple ()
   return %10 : $()
 }
+
+// MemoryLifetimeVerifier does not detect an error here due to reborrows
+sil [ossa] @test_load_borrow1 : $@convention(thin) (@in Optional<T>) -> () {
+bb0(%0 : $*Optional<T>):
+  destroy_addr %0 : $*Optional<T>
+  %1 = load_borrow %0 : $*Optional<T>
+  br bb1(%1 : $Optional<T>)
+
+bb1(%3 : @guaranteed $Optional<T>):
+  end_borrow %3 : $Optional<T>
+  br bb2
+
+bb2:
+  %r = tuple ()
+  return %r : $()
+}
+
+// CHECK: SIL memory lifetime failure in @test_load_borrow2: memory is not initialized, but should
+sil [ossa] @test_load_borrow2 : $@convention(thin) (@in Optional<T>) -> () {
+bb0(%0 : $*Optional<T>):
+  destroy_addr %0 : $*Optional<T>
+  %1 = load_borrow %0 : $*Optional<T>
+  end_borrow %1 : $Optional<T>
+  br bb1
+
+bb1:
+  %r = tuple ()
+  return %r : $()
+}
+
+enum Result<T1, T2>{
+  case success(T1)
+  case failure(T2)
+}
+
+sil @try_get_error : $@convention(thin) () -> @error Error
+
+// CHECK: SIL memory lifetime failure in @test_init_enum_trivial_case: memory is not initialized, but should
+sil [ossa] @test_init_enum_trivial_case : $@convention(thin) () -> @error Error {
+bb0:
+  %0 = alloc_stack $Result<Int, Error>
+  %1 = function_ref @try_get_error : $@convention(thin) () -> @error Error
+  try_apply %1() : $@convention(thin) () -> @error Error, normal bb1, error bb2
+
+bb1(%3 : $()):
+  inject_enum_addr %0 : $*Result<Int, Error>, #Result.success!enumelt
+  br bb3
+
+
+bb2(%7 : @owned $Error):
+  %8 = init_enum_data_addr %0 : $*Result<Int, Error>, #Result.failure!enumelt
+  store %7 to [init] %8 : $*Error
+  inject_enum_addr %0 : $*Result<Int, Error>, #Result.failure!enumelt
+  br bb3
+
+bb3:
+  %12 = load [take] %0 : $*Result<Int, Error>
+  destroy_value %12 : $Result<Int, Error>
+  dealloc_stack %0 : $*Result<Int, Error>
+  %15 = tuple ()
+  return %15 : $()
+}
+