Merge pull request swiftlang#83763 from atrick/fix-dce-struct-deinit

SIL: fix miscompiles of non-Copyable struct/enum with deinits

Author: atrick

include/swift/SIL/SILInstruction.h

@@ -1293,7 +1293,9 @@ class ForwardingInstruction {
   /// The resulting forwarded value's ownership, returned by getOwnershipKind(),
   /// is not identical to the forwarding ownership. It differs when the result
   /// is trivial type. e.g. an owned or guaranteed value can be cast to a
-  /// trivial type using owned or guaranteed forwarding.
+  /// trivial type using owned or guaranteed forwarding. Similarly, if a trivial
+  /// value is forwarded into an owned non-Copyable struct or enum, forwarding
+  /// ownership is 'none' while value ownerhip is 'owned'.
   ValueOwnershipKind getForwardingOwnershipKind() const {
     return ownershipKind;
   }
@@ -7009,8 +7011,9 @@ class EnumInst
   EnumInst(SILDebugLocation DebugLoc, SILValue Operand,
            EnumElementDecl *Element, SILType ResultTy,
            ValueOwnershipKind forwardingOwnershipKind)
-      : InstructionBase(DebugLoc, ResultTy, forwardingOwnershipKind),
-        Element(Element) {
+    : InstructionBase(DebugLoc, ResultTy,
+                      forwardingOwnershipKind.forwardToInit(ResultTy)),
+      Element(Element) {
     sharedUInt32().EnumInst.caseIndex = InvalidCaseIndex;
 
     if (Operand) {

include/swift/SIL/SILValue.h

@@ -353,6 +353,30 @@ struct ValueOwnershipKind {
                       });
   }
 
+  // An initialized value whose nominal type has a deinit() must be 'owned'. For
+  // example, an owned struct/enum-with-deinit may be initialized by
+  // "forwarding" a trivial value. A struct/enum-with-deinit must be prevented
+  // from forwarding a guaranteed value.
+  //
+  // Simply consider all non-Copyable types to be 'owned'. This could instead be
+  // limited to isValueTypeWithDeinit(). However, forcing non-Copyable types to
+  // be owned is consistent with the fact that their type is non-Trivial and
+  // simplifies reasoning about non-Copyable ownership.
+  ValueOwnershipKind forwardToInit(SILType nominalType) {
+    if (nominalType.isMoveOnly()) {
+      switch (value) {
+      case OwnershipKind::Any:
+      case OwnershipKind::None:
+      case OwnershipKind::Owned:
+        return OwnershipKind::Owned;
+      case OwnershipKind::Guaranteed:
+      case OwnershipKind::Unowned:
+        ABORT("Cannot initialize a nonCopyable type with a guaranteed value");
+      }
+    }
+    return *this;
+  }
+
   StringRef asString() const;
 };
 

lib/SIL/IR/SILInstructions.cpp

@@ -1509,8 +1509,9 @@ StructInst *StructInst::create(SILDebugLocation Loc, SILType Ty,
 StructInst::StructInst(SILDebugLocation Loc, SILType Ty,
                        ArrayRef<SILValue> Elems,
                        ValueOwnershipKind forwardingOwnershipKind)
-    : InstructionBaseWithTrailingOperands(Elems, Loc, Ty,
-                                          forwardingOwnershipKind) {
+    : InstructionBaseWithTrailingOperands(
+      Elems, Loc, Ty, forwardingOwnershipKind.forwardToInit(Ty))
+{
   assert(!Ty.getStructOrBoundGenericStruct()->hasUnreferenceableStorage());
 }
 

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

@@ -548,6 +548,10 @@ static SILBasicBlock::iterator
 eliminateUnneededForwardingUnarySingleValueInst(SingleValueInstruction *inst,
                                                 CanonicalizeInstruction &pass) {
   auto next = std::next(inst->getIterator());
+  if (inst->getType().isValueTypeWithDeinit()) {
+    // Avoid bypassing non-Copyable struct/enum deinitializers.
+    return next;
+  }
   if (isa<DropDeinitInst>(inst))
     return next;
   if (auto *uedi = dyn_cast<UncheckedEnumDataInst>(inst)) {

test/SILOptimizer/sil_combine_moveonly.sil

@@ -31,6 +31,17 @@ struct Wrapper<T>: ~Copyable {
   var t: T
 }
 
+struct NonCopyableForwardingStruct : ~Copyable {
+  var some: Int
+  deinit
+}
+
+enum NonCopyableForwardingEnum : ~Copyable {
+  case some(Int)
+  case nothing
+  deinit
+}
+
 sil @getWrappedValue : $@convention(thin) <T> (@in_guaranteed Wrapper<T>) -> @out T
 
 // Test that a release_value is not removed for a struct-with-deinit.
@@ -110,3 +121,51 @@ bb0(%0 : @owned $S):
   %64 = tuple ()
   return %64 : $()
 }
+
+// Test a "dead" non-Copyable struct-with-deinit that forwards a
+// single trivial value. The destroy_value cannot be eliminated
+// without first devirtualizing the deinit.
+//
+// CHECK-LABEL: sil [ossa] @testNonCopyableForwardingStructDeinit : $@convention(thin) (Int) -> () {
+// CHECK: [[STRUCT:%[0-9]+]] = struct $NonCopyableForwardingStruct (%0 : $Int)
+// CHECK: destroy_value [[STRUCT]]
+// CHECK-LABEL: } // end sil function 'testNonCopyableForwardingStructDeinit'
+sil [ossa] @testNonCopyableForwardingStructDeinit : $@convention(thin) (Int) -> () {
+bb0(%0: $Int):
+  %1 = struct $NonCopyableForwardingStruct(%0 : $Int)
+  destroy_value %1
+  %13 = tuple ()
+  return %13
+}
+
+// Test a "dead" non-Copyable enum-with-deinit that forwards a single
+// trivial value. The destroy_value cannot be eliminated without first
+// devirtualizing the deinit.
+//
+// CHECK-LABEL: sil [ossa] @testNonCopyableForwardingEnumDeinit : $@convention(thin) (Int) -> () {
+// CHECK:   [[ENUM:%[0-9]+]] = enum $NonCopyableForwardingEnum, #NonCopyableForwardingEnum.some!enumelt, %0
+// CHECK:   destroy_value [[ENUM]]
+// CHECK-LABEL: } // end sil function 'testNonCopyableForwardingEnumDeinit'
+sil [ossa] @testNonCopyableForwardingEnumDeinit : $@convention(thin) (Int) -> () {
+bb0(%0 : $Int):
+  %1 = enum $NonCopyableForwardingEnum, #NonCopyableForwardingEnum.some!enumelt, %0
+  destroy_value %1
+  %13 = tuple ()
+  return %13
+}
+
+// Test a "dead" non-Copyable enum-with-deinit and an empty case. The
+// destroy_value cannot be eliminated without first devirtualizing the
+// deinit.
+//
+// CHECK-LABEL: sil [ossa] @testNonCopyableEmptyEnumDeinit : $@convention(thin) () -> () {
+// CHECK:   [[ENUM:%[0-9]+]] = enum $NonCopyableForwardingEnum, #NonCopyableForwardingEnum.nothing
+// CHECK:   destroy_value [[ENUM]]
+// CHECK-LABEL: } // end sil function 'testNonCopyableEmptyEnumDeinit'
+sil [ossa] @testNonCopyableEmptyEnumDeinit : $@convention(thin) () -> () {
+bb0:
+  %0 = enum $NonCopyableForwardingEnum, #NonCopyableForwardingEnum.nothing
+  destroy_value %0
+  %13 = tuple ()
+  return %13
+}