DI: Lower AssignInst in a post-processing pass

Compiler passes that intermingle analysis with mutation of the CFG
are fraught with danger. The bug here was that a single AssignInst
could appear twice in DI's Uses list, once as a store use and once
as a load use.

When processing Uses, we would lower AssignInsts on the fly. We would
take care to erase the instruction pointer from the current Use, but
if a subsequent Use *also* referenced the now-deleted AssignInst, we
would crash.

Handle this in the standard way: instead of lowering assignments
right away, just build a list of assignments that we're planning on
lowering and process them all at the very end.

This has the added benefit of simplifying the code, because we no
longer have to work as hard to keep the state of the Uses list
consistent while lowering AssignInsts. The rest of DI should not
care that the lowering got postponed either, since it was already
expected to handle any ordering of elements in the Uses list, so
it could not assume that any particular AssignInst has been lowered.

Fixes <https://bugs.swift.org/browse/SR-9451>.

Author: slavapestov

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -390,6 +390,7 @@ namespace {
     SmallVectorImpl<DIMemoryUse> &Uses;
     TinyPtrVector<SILInstruction *> &StoresToSelf;
     SmallVectorImpl<SILInstruction *> &Destroys;
+    SmallVector<unsigned, 8> NeedsUpdateForInitState;
     std::vector<ConditionalDestroy> ConditionalDestroys;
 
     llvm::SmallDenseMap<SILBasicBlock*, LiveOutBlockState, 32> PerBlockInfo;
@@ -471,7 +472,8 @@ namespace {
                               bool SuperInitDone,
                               bool FailedSelfUse);
 
-    void handleSelfInitUse(DIMemoryUse &Use);
+    void handleSelfInitUse(unsigned UseID);
+
     void updateInstructionForInitState(DIMemoryUse &Use);
 
 
@@ -777,7 +779,7 @@ void LifetimeChecker::doIt() {
       handleEscapeUse(Use);
       break;
     case DIUseKind::SelfInit:
-      handleSelfInitUse(Use);
+      handleSelfInitUse(i);
       break;
     case DIUseKind::LoadForTypeOfSelf:
       handleLoadForTypeOfSelfUse(Use);
@@ -806,6 +808,12 @@ void LifetimeChecker::doIt() {
     ControlVariable = handleConditionalInitAssign();
   if (!ConditionalDestroys.empty())
     handleConditionalDestroys(ControlVariable);
+
+  // handleStoreUse(), handleSelfInitUse() and handleConditionalInitAssign()
+  // postpone lowering of assignment instructions to avoid deleting
+  // instructions that still appear in the Uses list.
+  for (unsigned UseID : NeedsUpdateForInitState)
+    updateInstructionForInitState(Uses[UseID]);
 }
 
 void LifetimeChecker::handleLoadUse(const DIMemoryUse &Use) {
@@ -1016,7 +1024,7 @@ void LifetimeChecker::handleStoreUse(unsigned UseID) {
 
   // Otherwise, we have a definite init or assign.  Make sure the instruction
   // itself is tagged properly.
-  updateInstructionForInitState(Use);
+  NeedsUpdateForInitState.push_back(UseID);
 }
 
 /// Check whether the instruction is an application.
@@ -1757,7 +1765,8 @@ void LifetimeChecker::handleLoadUseFailure(const DIMemoryUse &Use,
 
 /// handleSelfInitUse - When processing a 'self' argument on a class, this is
 /// a call to self.init or super.init.
-void LifetimeChecker::handleSelfInitUse(DIMemoryUse &Use) {
+void LifetimeChecker::handleSelfInitUse(unsigned UseID) {
+  auto &Use = Uses[UseID];
   auto *Inst = Use.Inst;
 
   assert(TheMemory.isAnyInitSelf());
@@ -1792,7 +1801,7 @@ void LifetimeChecker::handleSelfInitUse(DIMemoryUse &Use) {
 
     // Lower Assign instructions if needed.
     if (isa<AssignInst>(Use.Inst))
-      updateInstructionForInitState(Use);
+      NeedsUpdateForInitState.push_back(UseID);
   } else {
     // super.init also requires that all ivars are initialized before the
     // superclass initializer runs.
@@ -1852,14 +1861,13 @@ void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
   if (auto *AI = dyn_cast<AssignInst>(Inst)) {
     // Remove this instruction from our data structures, since we will be
     // removing it.
-    auto Kind = Use.Kind;
     Use.Inst = nullptr;
     NonLoadUses.erase(Inst);
 
     PartialInitializationKind PartialInitKind;
 
     if (TheMemory.isClassInitSelf() &&
-        Kind == DIUseKind::SelfInit) {
+        Use.Kind == DIUseKind::SelfInit) {
       assert(InitKind == IsInitialization);
       PartialInitKind = PartialInitializationKind::IsReinitialization;
     } else {
@@ -1868,27 +1876,8 @@ void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
                          : PartialInitializationKind::IsNotInitialization);
     }
 
-    unsigned FirstElement = Use.FirstElement;
-    unsigned NumElements = Use.NumElements;
-
-    SmallVector<SILInstruction*, 4> InsertedInsts;
-    SILBuilderWithScope B(Inst, &InsertedInsts);
+    SILBuilderWithScope B(Inst);
     lowerAssignInstruction(B, AI, PartialInitKind);
-
-    // If lowering of the assign introduced any new loads or stores, keep track
-    // of them.
-    for (auto I : InsertedInsts) {
-      if (isa<StoreInst>(I)) {
-        NonLoadUses[I] = Uses.size();
-        Uses.push_back(DIMemoryUse(I, Kind, FirstElement, NumElements));
-      } else if (isa<LoadInst>(I)) {
-        // If we have a re-initialization, the value must be a class,
-        // and the load is just there so we can free the uninitialized
-        // object husk; it's not an actual use of 'self'.
-        if (PartialInitKind != PartialInitializationKind::IsReinitialization)
-          Uses.push_back(DIMemoryUse(I, Load, FirstElement, NumElements));
-      }
-    }
     return;
   }
 
@@ -2257,15 +2246,12 @@ SILValue LifetimeChecker::handleConditionalInitAssign() {
     // Finally, now that we know the value is uninitialized on all paths, it is
     // safe to do an unconditional initialization.
     Use.Kind = DIUseKind::Initialization;
-    
-    // Now that the instruction has a concrete "init" form, update it to reflect
-    // that.  Note that this can invalidate the Uses vector and delete
-    // the instruction.
-    updateInstructionForInitState(Use);
-
-    // Revisit the instruction on the next pass through the loop, so that we
-    // emit a mask update as appropriate.
-    --i;
+    NeedsUpdateForInitState.push_back(i);
+
+    // Update the control variable.
+    APInt Bitmask = Use.getElementBitmask(NumMemoryElements);
+    SILBuilderWithScope SB(Use.Inst);
+    updateControlVariable(Loc, Bitmask, ControlVariableAddr, OrFn, SB);
   }
 
   // At each block that stores to self, mark the self value as having been

test/SILOptimizer/definite_init_diagnostics.swift

@@ -1551,3 +1551,31 @@ func testOptionalUnwrapNoError() -> Int? {
   x = 0
   return x!
 }
+
+// <https://bugs.swift.org/browse/SR-9451>
+class StrongCycle {
+  var c: StrongCycle
+  var d: Int
+  init(first: ()) {
+    self.d = 10
+    self.c = self // expected-error {{variable 'self.c' used before being initialized}}
+  }
+
+  init(second: ()) {
+    self.c = self // expected-error {{variable 'self.c' used before being initialized}}
+    self.d = 10
+  }
+}
+
+class WeakCycle {
+  weak var c: WeakCycle?
+  var d: Int
+  init(first: ()) { // FIXME: This is inconsistent with the strong reference behavior above
+    self.d = 10
+    self.c = self
+  }
+  init(second: ()) {
+    self.c = self // expected-error {{variable 'self.d' used before being initialized}}
+    self.d = 10
+  }
+}

test/SILOptimizer/definite_init_markuninitialized_var.sil

@@ -260,11 +260,7 @@ bb0:
 
   %b = alloc_box $<τ_0_0> { var τ_0_0 } <@sil_weak Optional<SomeClass>>
   %ba = project_box %b : $<τ_0_0> { var τ_0_0 } <@sil_weak Optional<SomeClass>>, 0
-  %c = mark_uninitialized [var] %ba : $*@sil_weak Optional<SomeClass> // expected-note {{variable defined here}}
-
-  // Invalid load to keep the alloc_box around so we can check init semantics.
-  %c_loaded = load_weak %c : $*@sil_weak Optional<SomeClass>    // expected-error {{used before being initialized}}
-  destroy_value %c_loaded : $Optional<SomeClass>
+  %c = mark_uninitialized [var] %ba : $*@sil_weak Optional<SomeClass>
 
   %f = function_ref @getSomeOptionalClass : $@convention(thin) () -> @owned Optional<SomeClass>