Merge pull request #66778 from atrick/5.9-fix-field-liveness

[5.9] Fix MoveOnlyAddressChecker to handle value deinits

Author: atrick

lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp

@@ -47,6 +47,15 @@ static StructDecl *getFullyReferenceableStruct(SILType ktypeTy) {
   return structDecl;
 }
 
+static bool isValueTypeWithDeinit(SILType type) {
+  // Do not look inside an aggregate type that has a user-deinit, for which
+  // memberwise-destruction is not equivalent to aggregate destruction.
+  if (auto *nominal = type.getNominalOrBoundGenericNominal()) {
+    return nominal->getValueTypeDestructor() != nullptr;
+  }
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 //                         MARK: TypeSubElementCount
 //===----------------------------------------------------------------------===//
@@ -70,8 +79,13 @@ TypeSubElementCount::TypeSubElementCount(SILType type, SILModule &mod,
           type.getFieldType(fieldDecl, mod, context), mod, context);
     number = numElements;
 
+    if (isValueTypeWithDeinit(type)) {
+      // 'self' has its own liveness represented as an additional field at the
+      // end of the structure.
+      ++number;
+    }
     // If we do not have any elements, just set our size to 1.
-    if (numElements == 0)
+    if (number == 0)
       number = 1;
 
     return;
@@ -350,6 +364,10 @@ void TypeTreeLeafTypeRange::constructFilteredProjections(
       callback(newValue, TypeTreeLeafTypeRange(start, next));
       start = next;
     }
+    if (isValueTypeWithDeinit(type)) {
+      // 'self' has its own liveness
+      ++start;
+    }
     assert(start == endEltOffset);
     return;
   }

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -437,6 +437,7 @@ static bool memInstMustConsume(Operand *memOper) {
 
   SILInstruction *memInst = memOper->getUser();
 
+  // FIXME: drop_deinit must be handled here!
   switch (memInst->getKind()) {
   default:
     return false;

test/Interpreter/moveonly_discard.swift

@@ -90,7 +90,34 @@ struct SillyEmptyGeneric<T>: ~Copyable {
   deinit { fatalError("ran unexpectedly!") }
 }
 
+struct SingleMutableField: ~Copyable {
+  var value = 0
+
+  consuming func justDiscard() {
+    discard self
+  }
+
+  deinit {
+    print("SingleMutableField.deinit")
+  }
+}
+
+// rdar://110232973 ([move-only] Checker should distinguish in between
+// field of single field struct vs parent field itself (was: mutation
+// of field in noncopyable struct should not trigger deinit))
+//
+// This test must not be in a closure.
+@inline(never)
+func testSingleMutableFieldNoMemberReinit() {
+  var x = SingleMutableField()
+  x.value = 20 // should not trigger deinit.
+  // CHECK-NOT: SingleMutableField.deinit
+  x.justDiscard()
+}
+
 func main() {
+  testSingleMutableFieldNoMemberReinit()
+
   let _ = {
     let x = FileDescriptor() // 0
     x.close()

test/SILOptimizer/moveonly_addresschecker.sil

@@ -3,6 +3,7 @@
 sil_stage raw
 
 import Swift
+import Builtin
 
 public class CopyableKlass {}
 
@@ -52,6 +53,17 @@ final class ClassContainingMoveOnly {
   var value = NonTrivialStruct()
 }
 
+struct SingleTrivialFieldAndDeinit: ~Copyable {
+    var value: Builtin.IntLiteral
+
+    consuming func finalize()
+
+    deinit
+}
+
+sil @getSingleTrivialFieldAndDeinit : $@convention(thin) () -> @owned SingleTrivialFieldAndDeinit
+sil @finalizeSingleTrivialFieldAndDeinit : $@convention(thin) (@owned SingleTrivialFieldAndDeinit) -> ()
+
 @_hasStorage @_hasInitialValue var varGlobal: NonTrivialStruct { get set }
 @_hasStorage @_hasInitialValue let letGlobal: NonTrivialStruct { get }
 sil_global hidden @$s23moveonly_addresschecker9varGlobalAA16NonTrivialStructVvp : $NonTrivialStruct
@@ -632,3 +644,36 @@ bb0:
   %22 = tuple ()
   return %22 : $()
 }
+
+// rdar://110232973 ([move-only] Checker should distinguish in between field of single field struct
+// vs parent field itself (was: mutation of field in noncopyable struct should not trigger deinit))
+//
+// Test that the SingleTrivialFieldAndDeinit aggregate is not
+// deinitialized when it's only field is consumed.
+//
+// CHECK-LABEL: sil hidden [ossa] @testNoFieldReinit : $@convention(thin) () -> () {
+// CHECK: [[ALLOC:%.*]] = alloc_stack [lexical] $SingleTrivialFieldAndDeinit
+// CHECK-NOT: destroy_addr [[ALLOC]]
+// CHECK-LABEL: } // end sil function 'testNoFieldReinit'
+sil hidden [ossa] @testNoFieldReinit : $@convention(thin) () -> () {
+bb0:
+  %0 = alloc_stack [lexical] $SingleTrivialFieldAndDeinit, var
+  %1 = mark_must_check [consumable_and_assignable] %0 : $*SingleTrivialFieldAndDeinit
+  %3 = function_ref @getSingleTrivialFieldAndDeinit : $@convention(thin) () -> @owned SingleTrivialFieldAndDeinit
+  %4 = apply %3() : $@convention(thin) () -> @owned SingleTrivialFieldAndDeinit
+  store %4 to [init] %1 : $*SingleTrivialFieldAndDeinit
+  %6 = integer_literal $Builtin.IntLiteral, 20
+  %10 = begin_access [modify] [static] %1 : $*SingleTrivialFieldAndDeinit
+  %11 = struct_element_addr %10 : $*SingleTrivialFieldAndDeinit, #SingleTrivialFieldAndDeinit.value
+  store %6 to [trivial] %11 : $*Builtin.IntLiteral
+  end_access %10 : $*SingleTrivialFieldAndDeinit
+  %14 = begin_access [deinit] [static] %1 : $*SingleTrivialFieldAndDeinit
+  %15 = load [take] %14 : $*SingleTrivialFieldAndDeinit
+  %16 = function_ref @finalizeSingleTrivialFieldAndDeinit : $@convention(thin) (@owned SingleTrivialFieldAndDeinit) -> ()
+  %17 = apply %16(%15) : $@convention(thin) (@owned SingleTrivialFieldAndDeinit) -> ()
+  end_access %14 : $*SingleTrivialFieldAndDeinit
+  destroy_addr %1 : $*SingleTrivialFieldAndDeinit
+  dealloc_stack %0 : $*SingleTrivialFieldAndDeinit
+  %21 = tuple ()
+  return %21 : $()
+}