Fix @_rawLayout initialization to avoid spurious lifetime ends.

jckarter · jckarter · commit 9b783560ad9d · 2023-08-01T08:34:02.000-07:00
We can't really treat them as always-initialized because that makes move checking
think that there's a value to destroy even on initialization, causing deinits to
run on uninitialized memory. Remove my previous hack, and use a `zeroInitializer`
to initialize the value state when emitting `init`, which is where we really need
the bootstrapping-into-initialized behavior. rdar://113057256
diff --git a/lib/IRGen/GenBuiltin.cpp b/lib/IRGen/GenBuiltin.cpp
@@ -1188,9 +1188,18 @@ void irgen::emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &Builtin,
     // Build a zero initializer of the result type.
     auto valueTy = getLoweredTypeAndTypeInfo(IGF.IGM,
                                              substitutions.getReplacementTypes()[0]);
-    auto schema = valueTy.second.getSchema();
-    for (auto &elt : schema) {
-      out.add(llvm::Constant::getNullValue(elt.getScalarType()));
+    
+    if (args.size() > 0) {
+      // `memset` the memory addressed by the argument.
+      auto address = args.claimNext();
+      IGF.Builder.CreateMemSet(valueTy.second.getAddressForPointer(address),
+                               llvm::ConstantInt::get(IGF.IGM.Int8Ty, 0),
+                               valueTy.second.getSize(IGF, argTypes[0]));
+    } else {
+      auto schema = valueTy.second.getSchema();
+      for (auto &elt : schema) {
+        out.add(llvm::Constant::getNullValue(elt.getScalarType()));
+      }
     }
     return;
   }
diff --git a/lib/SIL/IR/SILInstruction.cpp b/lib/SIL/IR/SILInstruction.cpp
@@ -976,6 +976,13 @@ MemoryBehavior SILInstruction::getMemoryBehavior() const {
   if (auto *BI = dyn_cast<BuiltinInst>(this)) {
     // Handle Swift builtin functions.
     const BuiltinInfo &BInfo = BI->getBuiltinInfo();
+    if (BInfo.ID == BuiltinValueKind::ZeroInitializer) {
+      // The address form of `zeroInitializer` writes to its argument to
+      // initialize it. The value form has no side effects.
+      return BI->getArguments().size() > 0
+        ? MemoryBehavior::MayWrite
+        : MemoryBehavior::None;
+    }
     if (BInfo.ID != BuiltinValueKind::None)
       return BInfo.isReadNone() ? MemoryBehavior::None
                                 : MemoryBehavior::MayHaveSideEffects;
diff --git a/lib/SIL/Utils/AddressWalker.cpp b/lib/SIL/Utils/AddressWalker.cpp
@@ -185,6 +185,7 @@ AddressUseKind TransitiveAddressWalker::walk(SILValue projectedAddress) && {
         case BuiltinValueKind::GenericFRem:
         case BuiltinValueKind::GenericXor:
         case BuiltinValueKind::TaskRunInline:
+        case BuiltinValueKind::ZeroInitializer:
           callVisitUse(op);
           continue;
         default:
diff --git a/lib/SIL/Utils/MemAccessUtils.cpp b/lib/SIL/Utils/MemAccessUtils.cpp
@@ -2592,6 +2592,13 @@ static void visitBuiltinAddress(BuiltinInst *builtin,
       // SIL address.
       // visitor(&builtin->getAllOperands()[0]);
       return;
+      
+    // zeroInitializer with an address operand zeroes the address.
+    case BuiltinValueKind::ZeroInitializer:
+      if (builtin->getAllOperands().size() > 0) {
+        visitor(&builtin->getAllOperands()[0]);
+      }
+      return;
 
     // Arrays: (T.Type, Builtin.RawPointer, Builtin.RawPointer,
     // Builtin.Word)
diff --git a/lib/SILGen/SILGenConstructor.cpp b/lib/SILGen/SILGenConstructor.cpp
@@ -722,9 +722,20 @@ void SILGenFunction::emitValueConstructor(ConstructorDecl *ctor) {
     // DISCUSSION: This only happens with noncopyable types since the memory
     // lifetime checker doesn't seem to process trivial locations. But empty
     // move only structs are non-trivial, so we need to handle this here.
-    if (isa<StructDecl>(nominal) && nominal->isMoveOnly()
-        && !nominal->getAttrs().hasAttribute<RawLayoutAttr>()
-        && nominal->getStoredProperties().empty()) {
+    if (nominal->getAttrs().hasAttribute<RawLayoutAttr>()) {
+      // Raw memory is not directly decomposable, but we still want to mark
+      // it as initialized. Use a zero initializer.
+      auto &C = ctor->getASTContext();
+      auto zeroInit = getBuiltinValueDecl(C, C.getIdentifier("zeroInitializer"));
+      B.createBuiltin(ctor, zeroInit->getBaseIdentifier(),
+                      SILType::getEmptyTupleType(C),
+                      SubstitutionMap::get(zeroInit->getInnermostDeclContext()
+                                               ->getGenericSignatureOfContext(),
+                                           {selfDecl->getType()},
+                                           {}),
+                      selfLV.getLValueAddress());
+    } else if (isa<StructDecl>(nominal) && nominal->isMoveOnly()
+               && nominal->getStoredProperties().empty()) {
       auto *si = B.createStruct(ctor, lowering.getLoweredType(), {});
       B.emitStoreValueOperation(ctor, si, selfLV.getLValueAddress(),
                                 StoreOwnershipQualifier::Init);
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp
@@ -952,13 +952,7 @@ void UseState::initializeLiveness(
     SILValue operand = address->getOperand();
     if (auto *c = dyn_cast<CopyableToMoveOnlyWrapperAddrInst>(operand))
       operand = c->getOperand();
-    // If the type uses raw layout, its initialization is unmanaged, so consider
-    // it always initialized.
-    auto s = operand->getType().getStructOrBoundGenericStruct();
-    if (s && s->getAttrs().hasAttribute<RawLayoutAttr>()) {
-      recordInitUse(address, address, liveness.getTopLevelSpan());
-      liveness.initializeDef(address, liveness.getTopLevelSpan());
-    } else if (auto *fArg = dyn_cast<SILFunctionArgument>(operand)) {
+    if (auto *fArg = dyn_cast<SILFunctionArgument>(operand)) {
       switch (fArg->getArgumentConvention()) {
       case swift::SILArgumentConvention::Indirect_In:
       case swift::SILArgumentConvention::Indirect_In_Guaranteed:
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyUtils.cpp
@@ -215,6 +215,13 @@ bool noncopyable::memInstMustInitialize(Operand *memOper) {
     return qual == StoreOwnershipQualifier::Init ||
            qual == StoreOwnershipQualifier::Trivial;
   }
+  case SILInstructionKind::BuiltinInst: {
+    auto bi = cast<BuiltinInst>(memInst);
+    if (bi->getBuiltinKind() == BuiltinValueKind::ZeroInitializer) {
+      // `zeroInitializer` with an address operand zeroes out the address operand
+      return true;
+    }
+  }
 
 #define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)             \
   case SILInstructionKind::Store##Name##Inst:                                  \
diff --git a/test/Interpreter/raw_layout_deinit.swift b/test/Interpreter/raw_layout_deinit.swift
@@ -0,0 +1,35 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift -enable-experimental-feature RawLayout %s -o %t/a.out
+// RUN: %target-codesign %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+// REQUIRES: executable_test
+
+@_rawLayout(size: 16, alignment: 16)
+struct Foo: ~Copyable {
+  init(_ value: Int) {
+    print("Foo.init(\(value))")
+  }
+
+  deinit {
+    print("Foo.deinit")
+  }
+
+  func bar() {
+    print("Foo.bar()")
+  }
+}
+
+func test() {
+  let foo = Foo(42)
+  foo.bar()
+}
+
+print("-- start")
+test()
+print("-- done")
+
+// CHECK:      -- start
+// CHECK-NEXT: Foo.init(42)
+// CHECK-NEXT: Foo.bar()
+// CHECK-NEXT: Foo.deinit
+// CHECK-NEXT: -- done
diff --git a/test/SILOptimizer/moveonly_raw_layout.swift b/test/SILOptimizer/moveonly_raw_layout.swift
@@ -0,0 +1,77 @@
+// RUN: %target-swift-frontend -enable-experimental-feature BuiltinModule -enable-experimental-feature RawLayout -emit-sil %s | %FileCheck %s
+
+import Builtin
+
+@_silgen_name("init_lock")
+func init_lock(_: Builtin.RawPointer)
+
+@_silgen_name("deinit_lock")
+func deinit_lock(_: Builtin.RawPointer)
+
+@_rawLayout(size: 4, alignment: 4)
+struct Lock: ~Copyable {
+    var _address: Builtin.RawPointer { return Builtin.addressOfBorrow(self) }
+
+    // CHECK-LABEL: // Lock.init()
+    // CHECK-NEXT: sil{{.*}} @[[INIT:\$.*4LockV.*fC]] :
+    init() {
+        // CHECK-NOT: destroy_addr
+        // CHECK: builtin "zeroInitializer"<Lock>
+        // CHECK-NOT: destroy_addr
+        // CHECK: [[F:%.*]] = function_ref @init_lock
+        // CHECK: apply [[F]](
+        // CHECK-NOT: destroy_addr
+        // CHECK: } // end sil function '[[INIT]]'
+        init_lock(_address)
+    }
+
+    // CHECK-LABEL: // Lock.deinit
+    // CHECK-NEXT: sil{{.*}} @[[DEINIT:\$.*4LockV.*fD]] :
+    deinit {
+        // CHECK-NOT: destroy_addr
+        // CHECK: [[F:%.*]] = function_ref @deinit_lock
+        // CHECK: apply [[F]](
+        // CHECK-NOT: destroy_addr
+        // CHECK: } // end sil function '[[DEINIT]]'
+        deinit_lock(_address)
+    }
+}
+
+@_silgen_name("borrow_lock")
+func borrow_lock(_: borrowing Lock)
+
+// CHECK-LABEL: sil{{.*}} @{{.*}}7useLock
+public func useLock() {
+    // CHECK: [[L:%.*]] = alloc_stack
+    // CHECK-NOT: destroy_addr [[L]] :
+    // CHECK: [[F:%.*]] = function_ref @[[INIT]]
+    // CHECK: apply [[F]]([[L]], 
+    var l = Lock()
+    // CHECK-NOT: destroy_addr [[L]] :
+    // CHECK: [[L_BORROW:%.*]] = begin_access [read] [static] [[L]] :
+    // CHECK: [[F:%.*]] = function_ref @borrow_lock
+    // CHECK: apply [[F]]([[L_BORROW]])
+    // CHECK: end_access [[L_BORROW]]
+    borrow_lock(l)
+
+    // CHECK: [[L2:%.*]] = alloc_stack
+    // CHECK-NOT: destroy_addr [[L2]] :
+    // CHECK: [[F:%.*]] = function_ref @[[INIT]]
+    // CHECK: apply [[F]]([[L2]], 
+    // CHECK: [[L_INOUT:%.*]] = begin_access [modify] [static] [[L]] :
+    // CHECK: destroy_addr [[L]] :
+    // CHECK: copy_addr [take] [[L2]] to [init] [[L_INOUT]]
+    // CHECK: end_access [[L_INOUT]]
+    // CHECK-NOT: destroy_addr [[L2]] :
+    // CHECK: dealloc_stack [[L2]]
+    l = Lock()
+    // CHECK-NOT: destroy_addr [[L]] :
+    // CHECK: [[L_BORROW:%.*]] = begin_access [read] [static] [[L]] :
+    // CHECK: [[F:%.*]] = function_ref @borrow_lock
+    // CHECK: apply [[F]]([[L_BORROW]])
+    // CHECK: end_access [[L_BORROW]]
+    borrow_lock(l)
+
+    // CHECK: destroy_addr [[L]]
+    // CHECK: dealloc_stack [[L]]
+}
