Fix spurious weak lifetime diagnostic.

Borrowed values can still escape and be copied:

  %alloc = allocation
  %borrow = begin_borrow %alloc
  apply(%borrow) // escape
  end_borrow %borrow
  weak_store %alloc

Don't warn in this case.

While we're at it, handle all kinds of forwarding operations
consistently. Otherwise, it's far too easy for the anlysis to be
defeated, making it unlikely to catch bugs that cause spurious
warnings.

Fixes rdar://79146338 (Xcode warns that "referenced object is
deallocated here" but that object was passed into a method that causes
strong retention)

Author: atrick

lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp

@@ -31,14 +31,15 @@
 
 #define DEBUG_TYPE "diagnose-lifetime-issues"
 #include "swift/AST/DiagnosticsSIL.h"
+#include "swift/Demangling/Demangler.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/BasicBlockBits.h"
+#include "swift/SIL/OwnershipUtils.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/PrunedLiveness.h"
-#include "swift/Demangling/Demangler.h"
+#include "clang/AST/DeclObjC.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Debug.h"
-#include "clang/AST/DeclObjC.h"
 
 using namespace swift;
 
@@ -156,19 +157,24 @@ static bool isStoreObjcWeak(SILInstruction *inst, Operand *op) {
 /// Returns the state of \p def. See DiagnoseLifetimeIssues::State.
 DiagnoseLifetimeIssues::State DiagnoseLifetimeIssues::
 visitUses(SILValue def, bool updateLivenessAndWeakStores, int callDepth) {
-  SmallSetVector<SILValue, 32> defUseWorklist;
-  defUseWorklist.insert(def);
+  SmallPtrSet<SILValue, 32> defUseVisited;
+  SmallVector<SILValue, 32> defUseVector;
+  auto pushDef = [&](SILValue value) {
+    if (defUseVisited.insert(value).second)
+      defUseVector.push_back(value);
+  };
+
+  pushDef(def);
   bool foundWeakStore = false;
-  while (!defUseWorklist.empty()) {
-    SILValue value = defUseWorklist.pop_back_val();
+  while (!defUseVector.empty()) {
+    SILValue value = defUseVector.pop_back_val();
     for (Operand *use : value->getUses()) {
       auto *user = use->getUser();
 
       // Recurse through copies and enums. Enums are important because the
       // operand of a store_weak is always an Optional.
-      if (isa<CopyValueInst>(user) || isa<EnumInst>(user) ||
-          isa<InitExistentialRefInst>(user)) {
-        defUseWorklist.insert(cast<SingleValueInstruction>(user));
+      if (isa<CopyValueInst>(user)) {
+        pushDef(cast<SingleValueInstruction>(user));
         continue;
       }
       if (isa<StoreWeakInst>(user) || isStoreObjcWeak(user, use)) {
@@ -208,22 +214,53 @@ visitUses(SILValue def, bool updateLivenessAndWeakStores, int callDepth) {
         if (updateLivenessAndWeakStores)
           liveness.updateForUse(user, /*lifetimeEnding*/ false);
         break;
+      case OperandOwnership::ForwardingBorrow:
       case OperandOwnership::ForwardingConsume:
-        // TODO: handle forwarding instructions, e.g. casts.
-        return CanEscape;
+        // TermInst includes ReturnInst, which is generally an escape.
+        // If this is called as part of getArgumentState, then it is not really
+        // an escape, but we don't currently follow returned values.
+        if (isa<TermInst>(user))
+          return CanEscape;
+
+        for (SILValue result : user->getResults()) {
+          // This assumes that forwarding to a trivial value cannot extend the
+          // lifetime. This way, simply projecting a trivial value out of an
+          // aggregate isn't considered an escape.
+          if (result.getOwnershipKind() == OwnershipKind::None)
+            continue;
+
+          pushDef(result);
+        }
+        continue;
       case OperandOwnership::DestroyingConsume:
         // destroy_value does not force pruned liveness (but store etc. does).
         if (!isa<DestroyValueInst>(user))
           return CanEscape;
         break;
-      case OperandOwnership::Borrow:
+      case OperandOwnership::Borrow: {
         if (updateLivenessAndWeakStores &&
             !liveness.updateForBorrowingOperand(use))
           return CanEscape;
+        BorrowingOperand borrowOper(use);
+        if (borrowOper.hasBorrowIntroducingUser()) {
+          if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(user))
+            pushDef(beginBorrow);
+          else
+            return CanEscape;
+        }
         break;
-      case OperandOwnership::InteriorPointer:
-      case OperandOwnership::ForwardingBorrow:
+      }
       case OperandOwnership::EndBorrow:
+        continue;
+
+      case OperandOwnership::InteriorPointer:
+        // Treat most interior pointers as escapes until they can be audited.
+        // But if the interior pointer cannot be used to copy the parent
+        // reference, then it does not need to be considered an escape.
+        if (isa<RefElementAddrInst>(user)) {
+          continue;
+        }
+        return CanEscape;
       case OperandOwnership::Reborrow:
         return CanEscape;
       }

test/SILOptimizer/diagnose_lifetime_issues.sil

@@ -0,0 +1,121 @@
+// RUN: %target-sil-opt -enable-sil-verify-all %s -diagnose-lifetime-issues  -o /dev/null -verify
+
+import Builtin
+import Swift
+
+enum FakeOptional<T> {
+case none
+case some(T)
+}
+
+class Delegate {
+  func foo() { }
+}
+
+class MyDelegate : Delegate {}
+
+final class Container {
+  weak var delegate: Delegate?
+  var strongRef: Delegate
+
+  func callDelegate()
+
+  func strongDelegate(d: Delegate)
+}
+
+class WeakCycle {
+  weak var c: WeakCycle?
+}
+
+sil [ossa] @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
+sil [ossa] @$s24diagnose_lifetime_issues14strongDelegate1dyAA0D0C_tF : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
+
+// Test a warning for a reference that is copied, cast, and assigned
+// to an enum before it is assigned to a weak reference.
+sil [ossa] @testCastWarn : $@convention(thin) (@guaranteed Container) -> () {
+bb0(%0 : @guaranteed $Container):
+  debug_value %0 : $Container, let, name "container", argno 1
+  %2 = metatype $@thick MyDelegate.Type
+  // function_ref MyDelegate.__allocating_init()
+  %3 = function_ref @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
+
+  // This is the owned allocation.
+  %4 = apply %3(%2) : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
+  %11 = copy_value %4 : $MyDelegate
+
+  // This upcast+enum is not an escape.
+  %12 = upcast %11 : $MyDelegate to $Delegate
+  %13 = enum $Optional<Delegate>, #Optional.some!enumelt, %12 : $Delegate
+  %14 = ref_element_addr %0 : $Container, #Container.delegate
+  %15 = begin_access [modify] [dynamic] %14 : $*@sil_weak Optional<Delegate>
+
+  // This is the weak assignment.
+  store_weak %13 to %15 : $*@sil_weak Optional<Delegate> // expected-warning {{weak reference will always be nil because the referenced object is deallocated here}}
+  destroy_value %13 : $Optional<Delegate>
+  end_access %15 : $*@sil_weak Optional<Delegate>
+  destroy_value %4 : $MyDelegate
+  %20 = tuple ()
+  return %20 : $()
+}
+
+// Test that a reference that escapes within a borrow scope has no warning.
+sil hidden [ossa] @testBorrowNoWarn : $@convention(thin) (@guaranteed Container) -> () {
+// %0 "container"
+bb0(%0 : @guaranteed $Container):
+  debug_value %0 : $Container, let, name "container", argno 1
+  %2 = metatype $@thick MyDelegate.Type
+  // function_ref MyDelegate.__allocating_init()
+  %3 = function_ref @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
+
+  // This is the owned allocation.
+  %4 = apply %3(%2) : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate
+  debug_value %4 : $MyDelegate, let, name "delegate"
+  %6 = begin_borrow %4 : $MyDelegate
+  %7 = upcast %6 : $MyDelegate to $Delegate
+  // function_ref Container.strongDelegate(d:)
+  %8 = function_ref @$s24diagnose_lifetime_issues14strongDelegate1dyAA0D0C_tF : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
+
+  // This apply is an escape.
+  %9 = apply %8(%7, %0) : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> ()
+  end_borrow %6 : $MyDelegate
+  %11 = copy_value %4 : $MyDelegate
+  %12 = upcast %11 : $MyDelegate to $Delegate
+  %13 = enum $Optional<Delegate>, #Optional.some!enumelt, %12 : $Delegate
+  %14 = ref_element_addr %0 : $Container, #Container.delegate
+  %15 = begin_access [modify] [dynamic] %14 : $*@sil_weak Optional<Delegate>
+
+  // This is the weak assignment.
+  store_weak %13 to %15 : $*@sil_weak Optional<Delegate>
+  destroy_value %13 : $Optional<Delegate>
+  end_access %15 : $*@sil_weak Optional<Delegate>
+  destroy_value %4 : $MyDelegate
+  %20 = tuple ()
+  return %20 : $()
+}
+
+// Helper for testReturnsAfterStore
+sil hidden [ossa] @testStoresWeakly : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle {
+bb0(%0 : @owned $WeakCycle):
+  %18 = begin_borrow %0 : $WeakCycle
+  %19 = copy_value %0 : $WeakCycle
+  %20 = enum $Optional<WeakCycle>, #Optional.some!enumelt, %19 : $WeakCycle
+  %21 = ref_element_addr %18 : $WeakCycle, #WeakCycle.c
+  %22 = begin_access [modify] [dynamic] %21 : $*@sil_weak Optional<WeakCycle>
+  store_weak %20 to %22 : $*@sil_weak Optional<WeakCycle>
+  destroy_value %20 : $Optional<WeakCycle>
+  end_access %22 : $*@sil_weak Optional<WeakCycle>
+  end_borrow %18 : $WeakCycle
+  %27 = copy_value %0 : $WeakCycle
+  destroy_value %0 : $WeakCycle
+  return %27 : $WeakCycle
+}
+
+// Test no warning for a value returned after a weak store.
+sil hidden [exact_self_class] [ossa] @testReturnsAfterStore : $@convention(method) (@thick WeakCycle.Type) -> @owned WeakCycle {
+bb0(%0 : $@thick WeakCycle.Type):
+  %1 = alloc_ref $WeakCycle
+
+  %2 = function_ref @testStoresWeakly : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle
+  %3 = apply %2(%1) : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle
+  return %3 : $WeakCycle
+}