Merge pull request #35918 from eeckstein/diagnose-lifetime-issues

eeckstein · web-flow · commit 841e4ee97526 · 2021-02-15T18:11:19.000+01:00
SILOptimizer: add a diagnostics pass to warn about lifetime issues with weak references.
diff --git a/include/swift/AST/DiagnosticsSIL.def b/include/swift/AST/DiagnosticsSIL.def
@@ -279,6 +279,9 @@ WARNING(switch_on_a_constant,none,
 NOTE(unreachable_code_note,none, "will never be executed", ())
 WARNING(warn_infinite_recursive_call,none,
         "function call causes an infinite recursion", ())
+WARNING(warn_dead_weak_store,none,
+        "weak reference will always be nil because the referenced object is "
+        "deallocated here", ())
 
 // 'transparent' diagnostics
 ERROR(circular_transparent,none,
diff --git a/include/swift/SILOptimizer/PassManager/Passes.def b/include/swift/SILOptimizer/PassManager/Passes.def
@@ -154,6 +154,8 @@ PASS(DiagnoseInfiniteRecursion, "diagnose-infinite-recursion",
      "Diagnose Infinitely-Recursive Code")
 PASS(DiagnoseInvalidEscapingCaptures, "diagnose-invalid-escaping-captures",
      "Diagnose Invalid Escaping Captures")
+PASS(DiagnoseLifetimeIssues, "diagnose-lifetime-issues",
+     "Diagnose Lifetime Issues")
 PASS(DiagnoseStaticExclusivity, "diagnose-static-exclusivity",
      "Static Enforcement of Law of Exclusivity")
 PASS(DiagnoseUnreachable, "diagnose-unreachable",
diff --git a/include/swift/SILOptimizer/Utils/PrunedLiveness.h b/include/swift/SILOptimizer/Utils/PrunedLiveness.h
@@ -219,6 +219,10 @@ class PrunedLiveness {
   /// live range vs. a nested borrow scope within the extended live range.
   void updateForUse(SILInstruction *user, bool lifetimeEnding);
 
+  /// Updates the liveness for a whole borrow scope, beginning at \p op.
+  /// Returns false if this cannot be done.
+  bool updateForBorrowingOperand(Operand *op);
+
   PrunedLiveBlocks::IsLive getBlockLiveness(SILBasicBlock *bb) const {
     return liveBlocks.getBlockLiveness(bb);
   }
diff --git a/lib/SILOptimizer/Mandatory/CMakeLists.txt b/lib/SILOptimizer/Mandatory/CMakeLists.txt
@@ -10,6 +10,7 @@ target_sources(swiftSILOptimizer PRIVATE
   DataflowDiagnostics.cpp
   DiagnoseInfiniteRecursion.cpp
   DiagnoseInvalidEscapingCaptures.cpp
+  DiagnoseLifetimeIssues.cpp
   DiagnoseStaticExclusivity.cpp
   DiagnoseUnreachable.cpp
   Differentiation.cpp
diff --git a/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp b/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp
@@ -0,0 +1,251 @@
+//==-------- DiagnoseLifetimeIssues.cpp - Diagnose lifetime issues ---------==//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements a diagnostic pass that prints a warning if an object is
+// stored to a weak property (or is weakly captured) and destroyed before the
+// property (or captured reference) is ever used again.
+// This can happen if the programmer relies on the lexical scope to keep an
+// object alive, but copy-propagation can shrink the object's lifetime to its
+// last use.
+// For example:
+//
+// func test() {
+//   let k = Klass()
+//   // k is deallocated immediatly after the closure capture (a store_weak).
+//   functionWithClosure({ [weak k] in
+//                         // crash!
+//                         k!.foo()
+//                       })
+// }
+//
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "diagnose-lifetime-issues"
+#include "swift/AST/DiagnosticsSIL.h"
+#include "swift/SIL/ApplySite.h"
+#include "swift/SIL/BasicBlockBits.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/PrunedLiveness.h"
+#include "swift/Demangling/Demangler.h"
+#include "llvm/Support/Debug.h"
+
+using namespace swift;
+
+namespace {
+
+/// Performs the analysis and prints the warnings.
+class DiagnoseLifetimeIssues {
+  PrunedLiveness liveness;
+
+  /// Reuse a general worklist for def-use traversal.
+  SmallSetVector<SILValue, 8> defUseWorklist;
+
+  bool computeCanonicalLiveness(SILValue def);
+
+  bool isDeadStore(StoreWeakInst *store);
+
+public:
+  DiagnoseLifetimeIssues() {}
+
+  void diagnose(SILFunction *function);
+};
+
+/// Returns true if def is an owned value resulting from an object allocation.
+static bool isAllocation(SILValue def) {
+  if (def.getOwnershipKind() != OwnershipKind::Owned)
+    return false;
+
+  if (isa<AllocRefInst>(def))
+    return true;
+  
+  // Check if it's a call to an allocating initializer.
+  if (auto *applyInst = dyn_cast<ApplyInst>(def)) {
+    SILFunction *callee = applyInst->getReferencedFunctionOrNull();
+    if (!callee)
+      return false;
+    
+    Demangle::StackAllocatedDemangler<1024> demangler;
+    Demangle::Node *root = demangler.demangleSymbol(callee->getName());
+    return root && root->getKind() == Demangle::Node::Kind::Global &&
+           root->getFirstChild()->getKind() == Demangle::Node::Kind::Allocator;
+  }
+    
+  return false;
+}
+
+/// Computes the canoncial lifetime of \p def, like the copy-propagation pass
+/// would do.
+/// The only difference is that we are treating enum instructions (taking a
+/// payload) like copies. Enums are important because the operand of a
+/// store_weak is always an Optional.
+bool DiagnoseLifetimeIssues::computeCanonicalLiveness(SILValue def) {
+  defUseWorklist.clear();
+  defUseWorklist.insert(def);
+  while (!defUseWorklist.empty()) {
+    SILValue value = defUseWorklist.pop_back_val();
+    for (Operand *use : value->getUses()) {
+      auto *user = use->getUser();
+
+      // Recurse through copies and enums.
+      if (isa<CopyValueInst>(user) || isa<EnumInst>(user)) {
+        defUseWorklist.insert(cast<SingleValueInstruction>(user));
+        continue;
+      }
+      switch (use->getOperandOwnership()) {
+      case OperandOwnership::NonUse:
+        break;
+      case OperandOwnership::TrivialUse:
+        llvm_unreachable("this operand cannot handle ownership");
+
+      // Conservatively treat a conversion to an unowned value as a pointer
+      // escape. Is it legal to canonicalize ForwardingUnowned?
+      case OperandOwnership::ForwardingUnowned:
+      case OperandOwnership::PointerEscape:
+        return false;
+      case OperandOwnership::InstantaneousUse:
+      case OperandOwnership::UnownedInstantaneousUse:
+      case OperandOwnership::BitwiseEscape:
+        liveness.updateForUse(user, /*lifetimeEnding*/ false);
+        break;
+      case OperandOwnership::ForwardingConsume:
+        // TODO: handle forwarding instructions, e.g. casts.
+        return false;
+      case OperandOwnership::DestroyingConsume:
+        // destroy_value does not force pruned liveness (but store etc. does).
+        if (!isa<DestroyValueInst>(user))
+          return false;
+        break;
+      case OperandOwnership::Borrow:
+        if (!liveness.updateForBorrowingOperand(use))
+          return false;
+        break;
+      case OperandOwnership::InteriorPointer:
+      case OperandOwnership::ForwardingBorrow:
+      case OperandOwnership::EndBorrow:
+      case OperandOwnership::Reborrow:
+        llvm_unreachable("operand kind cannot take an owned value");
+      }
+    }
+  }
+  return true;
+}
+
+/// Gets the underlying definition of \p val, looking through copy_value and
+/// enum instructions.
+static SILValue getCanonicalDef(SILValue val) {
+  while (true) {
+    if (auto *copyInst = dyn_cast<CopyValueInst>(val)) {
+      SILValue copySrc = copyInst->getOperand();
+      if (copySrc.getOwnershipKind() != OwnershipKind::Owned)
+        return val;
+      val = copySrc;
+      continue;
+    }
+    if (auto *enumInst = dyn_cast<EnumInst>(val)) {
+      if (enumInst->hasOperand()) {
+        val = enumInst->getOperand();
+        continue;
+      }
+    }
+    return val;
+  }
+}
+
+/// Returns true if the stored object of \p store is never loaded within the
+/// lifetime of the stored object.
+bool DiagnoseLifetimeIssues::isDeadStore(StoreWeakInst *store) {
+  SILValue storedDef = getCanonicalDef(store->getSrc());
+  
+  // Only for allocations we know that a destroy will actually deallocate the
+  // object. Otherwise the object could be kept alive by other references and
+  // we would issue a false alarm.
+  if (!isAllocation(storedDef))
+    return false;
+
+  liveness.initializeDefBlock(storedDef->getParentBlock());
+  if (!computeCanonicalLiveness(storedDef))
+    return false;
+
+
+  // Check if the lifetime of the stored object ends at the store_weak.
+  //
+  // A more sophisticated analysis would be to check if there are no
+  // (potential) loads from the store's destination address after the store,
+  // but within the object's liferange. But without a good alias analysis (and
+  // we don't want to use AliasAnalysis in a mandatory pass) it's practially
+  // impossible that a use of the object is not a potential load. So we would
+  // always see a potential load if the lifetime of the object goes beyond the
+  // store_weak.
+
+  SILBasicBlock *storeBlock = store->getParent();
+  if (liveness.getBlockLiveness(storeBlock) != PrunedLiveBlocks::LiveWithin)
+    return false;
+
+  // If there are any uses after the store_weak, it means that the liferange of
+  // the object goes beyond the store_weak.
+  for (SILInstruction &inst : make_range(std::next(store->getIterator()),
+                                         storeBlock->end())) {
+    switch (liveness.isInterestingUser(&inst)) {
+    case PrunedLiveness::NonUser:
+      break;
+    case PrunedLiveness::NonLifetimeEndingUse:
+    case PrunedLiveness::LifetimeEndingUse:
+      return false;
+    }
+  }
+  return true;
+}
+
+/// Prints warnings for dead weak stores in \p function.
+void DiagnoseLifetimeIssues::diagnose(SILFunction *function) {
+  for (SILBasicBlock &block : *function) {
+    for (SILInstruction &inst : block) {
+      if (auto *stWeak = dyn_cast<StoreWeakInst>(&inst)) {
+        if (isDeadStore(stWeak)) {
+          function->getModule().getASTContext().Diags.diagnose(
+              stWeak->getLoc().getSourceLoc(), diag::warn_dead_weak_store);
+        }
+        liveness.clear();
+        continue;
+      }
+    }
+  }
+}
+
+//===----------------------------------------------------------------------===//
+//                            The function pass
+//===----------------------------------------------------------------------===//
+
+class DiagnoseLifetimeIssuesPass : public SILFunctionTransform {
+public:
+  DiagnoseLifetimeIssuesPass() {}
+
+private:
+  void run() override {
+    SILFunction *function = getFunction();
+    // Don't rerun diagnostics on deserialized functions.
+    if (function->wasDeserializedCanonical())
+      return;
+
+    if (!function->hasOwnership())
+      return;
+
+    DiagnoseLifetimeIssues diagnoser;
+    diagnoser.diagnose(function);
+  }
+};
+
+} // end anonymous namespace
+
+SILTransform *swift::createDiagnoseLifetimeIssues() {
+  return new DiagnoseLifetimeIssuesPass();
+}
diff --git a/lib/SILOptimizer/PassManager/PassPipeline.cpp b/lib/SILOptimizer/PassManager/PassPipeline.cpp
@@ -154,6 +154,7 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
   P.addDiagnoseInfiniteRecursion();
   P.addYieldOnceCheck();
   P.addEmitDFDiagnostics();
+  P.addDiagnoseLifetimeIssues();
 
   // Canonical swift requires all non cond_br critical edges to be split.
   P.addSplitNonCondBrCriticalEdges();
diff --git a/lib/SILOptimizer/Utils/CanonicalOSSALifetime.cpp b/lib/SILOptimizer/Utils/CanonicalOSSALifetime.cpp
@@ -408,22 +408,8 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
         recordConsumingUse(use);
         break;
       case OperandOwnership::Borrow:
-        // A nested borrow scope is considered a use-point at each scope ending
-        // instruction.
-        //
-        // TODO: Handle reborrowed copies by considering the extended borrow
-        // scope. Temporarily bail-out on reborrows because we can't handle uses
-        // that aren't dominated by currentDef.
-        if (!BorrowingOperand(use).visitScopeEndingUses(
-              [this](Operand *end) {
-                if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
-                  return false;
-                }
-                liveness.updateForUse(end->getUser(), /*lifetimeEnding*/ false);
-                return true;
-              })) {
+        if (!liveness.updateForBorrowingOperand(use))
           return false;
-        }
         break;
       case OperandOwnership::InteriorPointer:
       case OperandOwnership::ForwardingBorrow:
diff --git a/lib/SILOptimizer/Utils/PrunedLiveness.cpp b/lib/SILOptimizer/Utils/PrunedLiveness.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "swift/SILOptimizer/Utils/PrunedLiveness.h"
+#include "swift/SIL/OwnershipUtils.h"
 
 using namespace swift;
 
@@ -93,3 +94,25 @@ void PrunedLiveness::updateForUse(SILInstruction *user, bool lifetimeEnding) {
   if (!iterAndSuccess.second)
     iterAndSuccess.first->second &= lifetimeEnding;
 }
+
+bool PrunedLiveness::updateForBorrowingOperand(Operand *op) {
+  assert(op->getOperandOwnership() == OperandOwnership::Borrow);
+
+  // A nested borrow scope is considered a use-point at each scope ending
+  // instruction.
+  //
+  // TODO: Handle reborrowed copies by considering the extended borrow
+  // scope. Temporarily bail-out on reborrows because we can't handle uses
+  // that aren't dominated by currentDef.
+  if (!BorrowingOperand(op).visitScopeEndingUses(
+        [this](Operand *end) {
+          if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
+            return false;
+          }
+          updateForUse(end->getUser(), /*lifetimeEnding*/ false);
+          return true;
+        })) {
+    return false;
+  }
+  return true;
+}
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -2050,6 +2050,9 @@ static void diagnoseUnownedImmediateDeallocationImpl(ASTContext &ctx,
   if (varDecl->getDeclContext()->isTypeContext())
     storageKind = SK_Property;
 
+  // TODO: The DiagnoseLifetimeIssuesPass prints a similiar warning in this
+  // situation. We should only print one warning.
+
   ctx.Diags.diagnose(diagLoc, diag::unowned_assignment_immediate_deallocation,
                      varDecl->getName(), ownershipAttr->get(),
                      unsigned(storageKind))
diff --git a/test/SILOptimizer/definite_init_diagnostics.swift b/test/SILOptimizer/definite_init_diagnostics.swift
@@ -107,6 +107,7 @@ func test2() {
   weak var w1 : SomeClass?
   _ = w1                // ok: default-initialized
 
+  // expected-warning@+4 {{weak reference will always be nil because the referenced object is deallocated here}}
   // expected-warning@+3 {{instance will be immediately deallocated because variable 'w2' is 'weak'}}
   // expected-note@+2 {{a strong reference is required to prevent the instance from being deallocated}}
   // expected-note@+1 {{'w2' declared here}}
@@ -1336,7 +1337,7 @@ func testDontDiagnoseUnownedImmediateDeallocationThroughStrong() {
   weak var c1: SomeClass?
   do {
     let tmp = SomeClass()
-    c1 = tmp
+    c1 = tmp // expected-warning {{weak reference will always be nil because the referenced object is deallocated here}}
   }
 
   unowned let c2: SomeClass
@@ -1347,7 +1348,7 @@ func testDontDiagnoseUnownedImmediateDeallocationThroughStrong() {
 
   weak var c3: SomeClass?
   let c3Tmp = SomeClass()
-  c3 = c3Tmp
+  c3 = c3Tmp // expected-warning {{weak reference will always be nil because the referenced object is deallocated here}}
 
   unowned let c4: SomeClass
   let c4Tmp = SomeClass()
diff --git a/test/SILOptimizer/diagnose_lifetime_issues.swift b/test/SILOptimizer/diagnose_lifetime_issues.swift
