SILGen: Track original variable's cleanup state for addressable buffer cleanups.

Don't try to delete the value on paths where the value hasn't yet been deleted.

Author: jckarter

lib/SILGen/Cleanup.h

@@ -74,6 +74,10 @@ enum class CleanupState {
   PersistentlyActive
 };
 
+inline bool isActiveCleanupState(CleanupState state) {
+  return state >= CleanupState::Active;
+}
+
 llvm::raw_ostream &operator<<(raw_ostream &os, CleanupState state);
 
 class LLVM_LIBRARY_VISIBILITY Cleanup {
@@ -109,7 +113,7 @@ class LLVM_LIBRARY_VISIBILITY Cleanup {
   virtual void setState(SILGenFunction &SGF, CleanupState newState) {
     state = newState;
   }
-  bool isActive() const { return state >= CleanupState::Active; }
+  bool isActive() const { return isActiveCleanupState(state); }
   bool isDead() const { return state == CleanupState::Dead; }
 
   virtual void emit(SILGenFunction &SGF, CleanupLocation loc,

lib/SILGen/SILGenDecl.cpp

@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "Cleanup.h"
 #include "Initialization.h"
 #include "LValue.h"
 #include "RValue.h"
@@ -682,10 +683,18 @@ namespace {
 
 static void deallocateAddressable(SILGenFunction &SGF,
                 SILLocation l,
-                const SILGenFunction::AddressableBuffer::State &state) {
-  SGF.B.createEndBorrow(l, state.storeBorrow);
+                const SILGenFunction::AddressableBuffer::State &state,
+                CleanupState cleanupState) {
+  // Only delete the value if the variable was active on this path.
+  // The stack slot exists within the variable scope regardless, so always
+  // needs to be cleaned up.
+  bool active = isActiveCleanupState(cleanupState);
+  if (active) {
+    SGF.B.createEndBorrow(l, state.storeBorrow);
+  }
   SGF.B.createDeallocStack(l, state.allocStack);
-  if (state.reabstraction
+  if (active
+      && state.reabstraction
       && !state.reabstraction->getType().isTrivial(SGF.F)) {
     SGF.B.createDestroyValue(l, state.reabstraction);
   }
@@ -695,8 +704,11 @@ static void deallocateAddressable(SILGenFunction &SGF,
 /// binding.
 class DeallocateLocalVariableAddressableBuffer : public Cleanup {
   ValueDecl *vd;
+  CleanupHandle destroyVarHandle;
 public:
-  DeallocateLocalVariableAddressableBuffer(ValueDecl *vd) : vd(vd) {}
+  DeallocateLocalVariableAddressableBuffer(ValueDecl *vd,
+                                           CleanupHandle destroyVarHandle)
+    : vd(vd), destroyVarHandle(destroyVarHandle) {}
 
   void emit(SILGenFunction &SGF, CleanupLocation l,
             ForUnwind_t forUnwind) override {
@@ -705,14 +717,20 @@ class DeallocateLocalVariableAddressableBuffer : public Cleanup {
       return;
     }
 
+    // Reflect the state of the variable's original cleanup.
+    CleanupState cleanupState = CleanupState::Active;
+    if (destroyVarHandle.isValid()) {
+      cleanupState = SGF.Cleanups.getCleanup(destroyVarHandle).getState();
+    }
+
     if (auto *state = addressableBuffer->getState()) {
-      // The addressable buffer was forced, so clean it up now.
-      deallocateAddressable(SGF, l, *state);
+      // The addressable buffer was already forced, so clean up now.
+      deallocateAddressable(SGF, l, *state, cleanupState);
     } else {
       // Remember this insert location in case we need to force the addressable
       // buffer later.
       SILInstruction *marker = SGF.B.createTuple(l, {});
-      addressableBuffer->cleanupPoints.emplace_back(marker);
+      addressableBuffer->cleanupPoints.push_back({marker, cleanupState});
     }
   }
 
@@ -820,7 +838,7 @@ class LetValueInitialization : public Initialization {
 
     // If the binding has an addressable buffer forced, it should be cleaned
     // up at this scope.
-    SGF.enterLocalVariableAddressableBufferScope(vd);
+    SGF.enterLocalVariableAddressableBufferScope(vd, DestroyCleanup);
   }
 
   ~LetValueInitialization() override {
@@ -2237,10 +2255,11 @@ void SILGenFunction::destroyLocalVariable(SILLocation silLoc, VarDecl *vd) {
 }
 
 void
-SILGenFunction::enterLocalVariableAddressableBufferScope(VarDecl *decl) {
+SILGenFunction::enterLocalVariableAddressableBufferScope(VarDecl *decl,
+                                                         CleanupHandle destroyCleanup) {
   auto marker = B.createTuple(decl, {});
   AddressableBuffers[decl].insertPoint = marker;
-  Cleanups.pushCleanup<DeallocateLocalVariableAddressableBuffer>(decl);
+  Cleanups.pushCleanup<DeallocateLocalVariableAddressableBuffer>(decl, destroyCleanup);
 }
 
 SILValue
@@ -2355,10 +2374,11 @@ SILGenFunction::getLocalVariableAddressableBuffer(VarDecl *decl,
   decltype(addressableBuffer->cleanupPoints) cleanupPoints;
   cleanupPoints.swap(addressableBuffer->cleanupPoints);
 
-  for (SILInstruction *cleanupPoint : cleanupPoints) {
-    SavedInsertionPointRAII insertCleanup(B, cleanupPoint);
-    deallocateAddressable(*this, cleanupPoint->getLoc(), *newState);
-    cleanupPoint->eraseFromParent();
+  for (auto &cleanupPoint : cleanupPoints) {
+    SavedInsertionPointRAII insertCleanup(B, cleanupPoint.inst);
+    deallocateAddressable(*this, cleanupPoint.inst->getLoc(), *newState,
+                          cleanupPoint.state);
+    cleanupPoint.inst->eraseFromParent();
   }
 
   return storeBorrow;
@@ -2400,8 +2420,8 @@ SILGenFunction::AddressableBuffer::~AddressableBuffer() {
   if (insertPoint) {
     insertPoint->eraseFromParent();
   }
-  for (auto cleanupPoint : cleanupPoints) {
-    cleanupPoint->eraseFromParent();
+  for (auto &cleanupPoint : cleanupPoints) {
+    cleanupPoint.inst->eraseFromParent();
   }
   if (auto state = stateOrAlias.dyn_cast<State*>()) {
     delete state;

lib/SILGen/SILGenFunction.h

@@ -520,7 +520,11 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
     // representation is demanded, but the addressable representation
     // gets demanded later, we save the insertion points where the
     // representation would be cleaned up so we can backfill them.
-    llvm::SmallVector<SILInstruction*, 1> cleanupPoints;
+    struct CleanupPoint {
+      SILInstruction *inst;
+      CleanupState state;
+    };
+    llvm::SmallVector<CleanupPoint, 1> cleanupPoints;
 
     AddressableBuffer() = default;
 
@@ -562,7 +566,13 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   ///
   /// This must be enclosed within the scope of the value binding for the
   /// variable, and cover the scope in which the variable can be referenced.
-  void enterLocalVariableAddressableBufferScope(VarDecl *decl);
+  ///
+  /// If \c destroyVarCleanup is provided, then the state of the referenced
+  /// cleanup is tracked to determine whether the variable is initialized on
+  /// every exit path out of the scope. Otherwise, the variable is assumed to
+  /// be active on every exit.
+  void enterLocalVariableAddressableBufferScope(VarDecl *decl,
+                    CleanupHandle destroyVarCleanup = CleanupHandle::invalid());
 
   /// Get a stable address which is suitable for forming dependent pointers
   /// if possible.

test/SILGen/if_guard_addressable.swift

@@ -0,0 +1,19 @@
+// RUN: %target-swift-emit-silgen -enable-experimental-feature AddressableParameters %s 
+
+// REQUIRES: swift_feature_AddressableParameters
+
+struct Foo {}
+
+func foo(_: borrowing @_addressable Foo) {}
+
+func bar() -> (Foo, Int)? { return nil }
+
+func test1() {
+	guard let (f, i) = bar() else {return}
+	
+	foo(f)
+}
+
+func test2() {
+	if let (f, i) = bar() {foo(f)} else {}
+}