[llvm] [Analysis] Require explict updates to BFI (PR #185184)

[Alexis Engelke via llvm-commits]

================
@@ -841,26 +835,34 @@ void IrreducibleGraph::addEdges(const BlockNode &Node,
 ///         series by simulation.)
 template <class BT> class BlockFrequencyInfoImpl : BlockFrequencyInfoImplBase {
   using BlockT = typename bfi_detail::TypeMap<BT>::BlockT;
-  using BlockKeyT = typename bfi_detail::TypeMap<BT>::BlockKeyT;
   using FunctionT = typename bfi_detail::TypeMap<BT>::FunctionT;
   using BranchProbabilityInfoT =
       typename bfi_detail::TypeMap<BT>::BranchProbabilityInfoT;
   using LoopT = typename bfi_detail::TypeMap<BT>::LoopT;
   using LoopInfoT = typename bfi_detail::TypeMap<BT>::LoopInfoT;
   using Successor = GraphTraits<const BlockT *>;
   using Predecessor = GraphTraits<Inverse<const BlockT *>>;
-  using BFICallbackVH =
-      bfi_detail::BFICallbackVH<BlockT, BlockFrequencyInfoImpl>;
 
   const BranchProbabilityInfoT *BPI = nullptr;
   const LoopInfoT *LI = nullptr;
   const FunctionT *F = nullptr;
 
   // All blocks in reverse postorder.
-  std::vector<BFICallbackVH> RPOT;
+  std::vector<const BlockT *> RPOT;
   DenseMap<const BlockT *, BlockNode> Nodes;
 
-  BlockNode getNode(const BlockT *BB) const { return Nodes.lookup(BB); }
+  BlockNode getNode(const BlockT *BB) const {
+#ifdef EXPENSIVE_CHECKS
+    // Try to catch cases where blocks were deleted (use-after-free can be
+    // caught by ASan) or moved to a different function, but the BFI was not
+    // updated. This is an asymptotically expensive check.
+    for (auto Node : RPOT)
+      assert((!Node || Node->getParent() == F) &&
+             "BFI stores basic block outside of function, missing update!");
+#endif
----------------
aengelke wrote:

AssertingVH is too strong, because there are too many cases where a cached BFI is stored in the analysis manager and is only invalidated at the end of a pass that deletes a block. It would require all passes to explicitly invalidate the BFI *before* erasing any blocks, even if they don't use it at all and don't claim to preserve it.

PoisoningVH is too weak: the stale block pointers aren't actually accessed, they're just there and in very rare cases cause problems when the address of a deleted BasicBlock gets reused, but this is too unlikely to be caught in non-release builds.

What we'd want is a value handle that poisons the entire BFI and assert that the BFI is not poisoned if the BFI continues to be used. (Doable but a bit annoying to implement due to joint handling of MIR.) MachineBasicBlocks have no ValueHandle and the staleness problem can occur there as well (allocations are recycled); this pretty hacky implementation did catch issues there that a purely VH-based implementation would not catch.

(NB: CallbackVH was introduced in [D75341](https://reviews.llvm.org/D75341).)

https://github.com/llvm/llvm-project/pull/185184