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

The SIL optimizer has fundamental bugs that result in dropping non-Copyable
struct & enum the deinitializers.

Fix this by

1. correctly representing the ownership of struct & enum values that are
   initialized from trivial values.

2. checking move-only types before deleting forwarding instructions.

These bugs block other bug fixes. They are exposed by other unrelated SIL
optimizations to SIL. I'm sure its possible to expose the bugs with source-level
tests, but the current order of inlining and deinit devirtualization has been
hiding the bugs and complicates reproduction.

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
+}