[LifetimeSafety] Make the dataflow analysis generic

[usx95]

Refactored the lifetime safety analysis to use a generic dataflow framework with a policy-based design.

Changes

• Introduced a generic DataflowAnalysis template class that can be specialized for different analyses
• Renamed LifetimeLattice to LoanPropagationLattice to better reflect its purpose
• Created a LoanPropagationAnalysis class that inherits from the generic framework
• Moved transfer functions from the standalone Transferer class into the analysis class
• Restructured the code to separate the dataflow engine from the specific analysis logic
• Updated debug output and test expectations to use the new class names

Motivation

In order to add more analyses, e.g. loan expiry and origin liveness, the previous implementation would have separate, nearly identical dataflow runners for each analysis. This change creates a single, reusable component, which will make it much simpler to add subsequent analyses without repeating boilerplate code.

This is quite close to the existing dataflow framework!

[usx95]

• [LifetimeSafety] Add loan expiry analysis #148712
• [LifetimeSafety] Make the dataflow analysis generic #148222 👈 (View in Graphite)
• [LifetimeSafety] Implement dataflow analysis for loan propagation #148065 : 1 other dependent PR (#147315 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

Refactored the lifetime safety analysis to use a generic dataflow framework with a policy-based design.

Changes

• Introduced a generic DataflowAnalysis template class that can be specialized for different analyses
• Renamed LifetimeLattice to LoanPropagationLattice to better reflect its purpose
• Created a LoanPropagationAnalysis class that inherits from the generic framework
• Moved transfer functions from the standalone Transferer class into the analysis class
• Restructured the code to separate the dataflow engine from the specific analysis logic
• Updated debug output and test expectations to use the new class names

Motivation

In order to add more analyses, e.g. loan expiry and origin liveness, the previous implementation would have separate, nearly identical dataflow runners for each analysis. This change creates a single, reusable component, which will make it much simpler to add subsequent analyses without repeating boilerplate code.

This is quite close to the existing dataflow framework!

---

Patch is 22.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/148222.diff

2 Files Affected:

• (modified) clang/lib/Analysis/LifetimeSafety.cpp (+188-167)
• (modified) clang/test/Sema/warn-lifetime-safety-dataflow.cpp (+15-15)

diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp
index bf67bea6c9933..99bd7bc36faf5 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/clang/lib/Analysis/LifetimeSafety.cpp
@@ -496,7 +496,130 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
 };
 
 // ========================================================================= //
-//                              The Dataflow Lattice
+//                         Generic Dataflow Analysis
+// ========================================================================= //
+/// A generic, policy-based driver for forward dataflow analyses. It combines
+/// the dataflow runner and the transferer logic into a single class hierarchy.
+///
+/// The derived class is expected to provide:
+/// - A `Lattice` type.
+/// - `const char *getAnalysisName() const`
+/// - `Lattice getInitialState();` The initial state at the function entry.
+/// - `Lattice join(Lattice, Lattice);` Merges states from multiple CFG paths.
+/// - `Lattice transfer(Lattice, const FactType&);` Defines how a single
+///   lifetime-relevant `Fact` transforms the lattice state. Only overloads
+///   for facts relevant to the analysis need to be implemented.
+///
+/// \tparam Derived The CRTP derived class that implements the specific
+/// analysis.
+/// \tparam LatticeType The lattice type used by the analysis.
+/// TODO: Maybe use the dataflow framework! The framework might need changes
+/// to support the current comparison done at block-entry.
+template <typename Derived, typename LatticeType> class DataflowAnalysis {
+public:
+  using Lattice = LatticeType;
+
+private:
+  const CFG &Cfg;
+  AnalysisDeclContext &AC;
+
+  llvm::DenseMap<const CFGBlock *, Lattice> BlockEntryStates;
+  llvm::DenseMap<const CFGBlock *, Lattice> BlockExitStates;
+
+protected:
+  FactManager &AllFacts;
+
+  explicit DataflowAnalysis(const CFG &C, AnalysisDeclContext &AC,
+                            FactManager &F)
+      : Cfg(C), AC(AC), AllFacts(F) {}
+
+public:
+  void run() {
+    Derived &D = static_cast<Derived &>(*this);
+    llvm::TimeTraceScope Time(D.getAnalysisName());
+
+    ForwardDataflowWorklist Worklist(Cfg, AC);
+    const CFGBlock *Entry = &Cfg.getEntry();
+    BlockEntryStates[Entry] = D.getInitialState();
+    Worklist.enqueueBlock(Entry);
+
+    while (const CFGBlock *B = Worklist.dequeue()) {
+      Lattice EntryState = getEntryState(B);
+      Lattice ExitState = transferBlock(B, EntryState);
+      BlockExitStates[B] = ExitState;
+
+      for (const CFGBlock *Successor : B->succs()) {
+        auto SuccIt = BlockEntryStates.find(Successor);
+        Lattice OldSuccEntryState = (SuccIt != BlockEntryStates.end())
+                                        ? SuccIt->second
+                                        : D.getInitialState();
+        Lattice NewSuccEntryState = D.join(OldSuccEntryState, ExitState);
+        // Enqueue the successor if its entry state has changed.
+        // TODO(opt): Consider changing 'join' to report a change if !=
+        // comparison is found expensive.
+        if (SuccIt == BlockEntryStates.end() ||
+            NewSuccEntryState != OldSuccEntryState) {
+          BlockEntryStates[Successor] = NewSuccEntryState;
+          Worklist.enqueueBlock(Successor);
+        }
+      }
+    }
+  }
+
+  Lattice getEntryState(const CFGBlock *B) const {
+    return BlockEntryStates.lookup(B);
+  }
+
+  Lattice getExitState(const CFGBlock *B) const {
+    return BlockExitStates.lookup(B);
+  }
+
+  void dump() const {
+    const Derived *D = static_cast<const Derived *>(this);
+    llvm::dbgs() << "==========================================\n";
+    llvm::dbgs() << "   " << D->getAnalysisName() << " results:\n";
+    llvm::dbgs() << "==========================================\n";
+    const CFGBlock &B = Cfg.getExit();
+    getExitState(&B).dump(llvm::dbgs());
+  }
+
+private:
+  /// Computes the exit state of a block by applying all its facts sequentially
+  /// to a given entry state.
+  /// TODO: We might need to store intermediate states per-fact in the block for
+  /// later analysis.
+  Lattice transferBlock(const CFGBlock *Block, Lattice EntryState) {
+    Lattice BlockState = EntryState;
+    for (const Fact *F : AllFacts.getFacts(Block)) {
+      BlockState = transferFact(BlockState, F);
+    }
+    return BlockState;
+  }
+
+  Lattice transferFact(Lattice In, const Fact *F) {
+    Derived *d = static_cast<Derived *>(this);
+    switch (F->getKind()) {
+    case Fact::Kind::Issue:
+      return d->transfer(In, *F->getAs<IssueFact>());
+    case Fact::Kind::Expire:
+      return d->transfer(In, *F->getAs<ExpireFact>());
+    case Fact::Kind::AssignOrigin:
+      return d->transfer(In, *F->getAs<AssignOriginFact>());
+    case Fact::Kind::ReturnOfOrigin:
+      return d->transfer(In, *F->getAs<ReturnOfOriginFact>());
+    }
+    llvm_unreachable("Unknown fact kind");
+  }
+
+public:
+  Lattice transfer(Lattice In, const IssueFact &) { return In; }
+  Lattice transfer(Lattice In, const ExpireFact &) { return In; }
+  Lattice transfer(Lattice In, const AssignOriginFact &) { return In; }
+  Lattice transfer(Lattice In, const ReturnOfOriginFact &) { return In; }
+};
+
+// ========================================================================= //
+//                          Loan Propagation Analysis
 // ========================================================================= //
 
 // Using LLVM's immutable collections is efficient for dataflow analysis
@@ -517,10 +640,10 @@ struct LifetimeFactory {
   }
 };
 
-/// LifetimeLattice represents the state of our analysis at a given program
-/// point. It is an immutable object, and all operations produce a new
+/// LoanPropagationLattice represents the state of our analysis at a given
+/// program point. It is an immutable object, and all operations produce a new
 /// instance rather than modifying the existing one.
-struct LifetimeLattice {
+struct LoanPropagationLattice {
   /// The map from an origin to the set of loans it contains.
   /// The lattice has a finite height: An origin's loan set is bounded by the
   /// total number of loans in the function.
@@ -528,61 +651,16 @@ struct LifetimeLattice {
   /// not expressions, because expressions are not visible across blocks.
   OriginLoanMap Origins = OriginLoanMap(nullptr);
 
-  explicit LifetimeLattice(const OriginLoanMap &S) : Origins(S) {}
-  LifetimeLattice() = default;
+  explicit LoanPropagationLattice(const OriginLoanMap &S) : Origins(S) {}
+  LoanPropagationLattice() = default;
 
-  bool operator==(const LifetimeLattice &Other) const {
+  bool operator==(const LoanPropagationLattice &Other) const {
     return Origins == Other.Origins;
   }
-  bool operator!=(const LifetimeLattice &Other) const {
+  bool operator!=(const LoanPropagationLattice &Other) const {
     return !(*this == Other);
   }
 
-  LoanSet getLoans(OriginID OID) const {
-    if (auto *Loans = Origins.lookup(OID))
-      return *Loans;
-    return LoanSet(nullptr);
-  }
-
-  /// Computes the union of two lattices by performing a key-wise join of
-  /// their OriginLoanMaps.
-  // TODO(opt): This key-wise join is a performance bottleneck. A more
-  // efficient merge could be implemented using a Patricia Trie or HAMT
-  // instead of the current AVL-tree-based ImmutableMap.
-  // TODO(opt): Keep the state small by removing origins which become dead.
-  LifetimeLattice join(const LifetimeLattice &Other,
-                       LifetimeFactory &Factory) const {
-    /// Merge the smaller map into the larger one ensuring we iterate over the
-    /// smaller map.
-    if (Origins.getHeight() < Other.Origins.getHeight())
-      return Other.join(*this, Factory);
-
-    OriginLoanMap JoinedState = Origins;
-    // For each origin in the other map, union its loan set with ours.
-    for (const auto &Entry : Other.Origins) {
-      OriginID OID = Entry.first;
-      LoanSet OtherLoanSet = Entry.second;
-      JoinedState = Factory.OriginMapFactory.add(
-          JoinedState, OID, join(getLoans(OID), OtherLoanSet, Factory));
-    }
-    return LifetimeLattice(JoinedState);
-  }
-
-  LoanSet join(LoanSet a, LoanSet b, LifetimeFactory &Factory) const {
-    /// Merge the smaller set into the larger one ensuring we iterate over the
-    /// smaller set.
-    if (a.getHeight() < b.getHeight())
-      std::swap(a, b);
-    LoanSet Result = a;
-    for (LoanID LID : b) {
-      /// TODO(opt): Profiling shows that this loop is a major performance
-      /// bottleneck. Investigate using a BitVector to represent the set of
-      /// loans for improved join performance.
-      Result = Factory.LoanSetFact.add(Result, LID);
-    }
-    return Result;
-  }
-
   void dump(llvm::raw_ostream &OS) const {
     OS << "LifetimeLattice State:\n";
     if (Origins.isEmpty())
@@ -596,143 +674,85 @@ struct LifetimeLattice {
   }
 };
 
-// ========================================================================= //
-//                              The Transfer Function
-// ========================================================================= //
-class Transferer {
-  FactManager &AllFacts;
+class LoanPropagationAnalysis
+    : public DataflowAnalysis<LoanPropagationAnalysis, LoanPropagationLattice> {
+
   LifetimeFactory &Factory;
 
 public:
-  explicit Transferer(FactManager &F, LifetimeFactory &Factory)
-      : AllFacts(F), Factory(Factory) {}
+  LoanPropagationAnalysis(const CFG &C, AnalysisDeclContext &AC, FactManager &F,
+                          LifetimeFactory &Factory)
+      : DataflowAnalysis(C, AC, F), Factory(Factory) {}
 
-  /// Computes the exit state of a block by applying all its facts sequentially
-  /// to a given entry state.
-  /// TODO: We might need to store intermediate states per-fact in the block for
-  /// later analysis.
-  LifetimeLattice transferBlock(const CFGBlock *Block,
-                                LifetimeLattice EntryState) {
-    LifetimeLattice BlockState = EntryState;
-    llvm::ArrayRef<const Fact *> Facts = AllFacts.getFacts(Block);
+  using DataflowAnalysis<LoanPropagationAnalysis, Lattice>::transfer;
 
-    for (const Fact *F : Facts) {
-      BlockState = transferFact(BlockState, F);
+  const char *getAnalysisName() const { return "LoanPropagation"; }
+
+  Lattice getInitialState() { return Lattice{}; }
+
+  /// Computes the union of two lattices by performing a key-wise join of
+  /// their OriginLoanMaps.
+  // TODO(opt): This key-wise join is a performance bottleneck. A more
+  // efficient merge could be implemented using a Patricia Trie or HAMT
+  // instead of the current AVL-tree-based ImmutableMap.
+  // TODO(opt): Keep the state small by removing origins which become dead.
+  Lattice join(Lattice A, Lattice B) {
+    /// Merge the smaller map into the larger one ensuring we iterate over the
+    /// smaller map.
+    if (A.Origins.getHeight() < B.Origins.getHeight())
+      std::swap(A, B);
+
+    OriginLoanMap JoinedState = A.Origins;
+    // For each origin in the other map, union its loan set with ours.
+    for (const auto &Entry : B.Origins) {
+      OriginID OID = Entry.first;
+      LoanSet OtherLoanSet = Entry.second;
+      JoinedState = Factory.OriginMapFactory.add(
+          JoinedState, OID, join(getLoans(A, OID), OtherLoanSet));
     }
-    return BlockState;
+    return Lattice(JoinedState);
   }
 
-private:
-  LifetimeLattice transferFact(LifetimeLattice In, const Fact *F) {
-    switch (F->getKind()) {
-    case Fact::Kind::Issue:
-      return transfer(In, *F->getAs<IssueFact>());
-    case Fact::Kind::AssignOrigin:
-      return transfer(In, *F->getAs<AssignOriginFact>());
-    // Expire and ReturnOfOrigin facts don't modify the Origins and the State.
-    case Fact::Kind::Expire:
-    case Fact::Kind::ReturnOfOrigin:
-      return In;
-    }
-    llvm_unreachable("Unknown fact kind");
+  LoanSet join(LoanSet A, LoanSet B) {
+    if (A.getHeight() < B.getHeight())
+      std::swap(A, B);
+    for (LoanID L : B)
+      A = Factory.LoanSetFact.add(A, L);
+    return A;
   }
 
   /// A new loan is issued to the origin. Old loans are erased.
-  LifetimeLattice transfer(LifetimeLattice In, const IssueFact &F) {
+  Lattice transfer(Lattice In, const IssueFact &F) {
     OriginID OID = F.getOriginID();
     LoanID LID = F.getLoanID();
-    return LifetimeLattice(Factory.OriginMapFactory.add(
+    return LoanPropagationLattice(Factory.OriginMapFactory.add(
         In.Origins, OID, Factory.createLoanSet(LID)));
   }
 
   /// The destination origin's loan set is replaced by the source's.
   /// This implicitly "resets" the old loans of the destination.
-  LifetimeLattice transfer(LifetimeLattice InState, const AssignOriginFact &F) {
+  Lattice transfer(Lattice In, const AssignOriginFact &F) {
     OriginID DestOID = F.getDestOriginID();
     OriginID SrcOID = F.getSrcOriginID();
-    LoanSet SrcLoans = InState.getLoans(SrcOID);
-    return LifetimeLattice(
-        Factory.OriginMapFactory.add(InState.Origins, DestOID, SrcLoans));
+    LoanSet SrcLoans = getLoans(In, SrcOID);
+    return LoanPropagationLattice(
+        Factory.OriginMapFactory.add(In.Origins, DestOID, SrcLoans));
   }
-};
 
-// ========================================================================= //
-//                              Dataflow analysis
-// ========================================================================= //
-
-/// Drives the intra-procedural dataflow analysis.
-///
-/// Orchestrates the analysis by iterating over the CFG using a worklist
-/// algorithm. It computes a fixed point by propagating the LifetimeLattice
-/// state through each block until the state no longer changes.
-/// TODO: Maybe use the dataflow framework! The framework might need changes
-/// to support the current comparison done at block-entry.
-class LifetimeDataflow {
-  const CFG &Cfg;
-  AnalysisDeclContext &AC;
-  LifetimeFactory LifetimeFact;
-
-  Transferer Xfer;
-
-  /// Stores the merged analysis state at the entry of each CFG block.
-  llvm::DenseMap<const CFGBlock *, LifetimeLattice> BlockEntryStates;
-  /// Stores the analysis state at the exit of each CFG block, after the
-  /// transfer function has been applied.
-  llvm::DenseMap<const CFGBlock *, LifetimeLattice> BlockExitStates;
-
-public:
-  LifetimeDataflow(const CFG &C, FactManager &FS, AnalysisDeclContext &AC)
-      : Cfg(C), AC(AC), Xfer(FS, LifetimeFact) {}
-
-  void run() {
-    llvm::TimeTraceScope TimeProfile("Lifetime Dataflow");
-    ForwardDataflowWorklist Worklist(Cfg, AC);
-    const CFGBlock *Entry = &Cfg.getEntry();
-    BlockEntryStates[Entry] = LifetimeLattice{};
-    Worklist.enqueueBlock(Entry);
-    while (const CFGBlock *B = Worklist.dequeue()) {
-      LifetimeLattice EntryState = getEntryState(B);
-      LifetimeLattice ExitState = Xfer.transferBlock(B, EntryState);
-      BlockExitStates[B] = ExitState;
-
-      for (const CFGBlock *Successor : B->succs()) {
-        auto SuccIt = BlockEntryStates.find(Successor);
-        LifetimeLattice OldSuccEntryState = (SuccIt != BlockEntryStates.end())
-                                                ? SuccIt->second
-                                                : LifetimeLattice{};
-        LifetimeLattice NewSuccEntryState =
-            OldSuccEntryState.join(ExitState, LifetimeFact);
-        // Enqueue the successor if its entry state has changed.
-        // TODO(opt): Consider changing 'join' to report a change if !=
-        // comparison is found expensive.
-        if (SuccIt == BlockEntryStates.end() ||
-            NewSuccEntryState != OldSuccEntryState) {
-          BlockEntryStates[Successor] = NewSuccEntryState;
-          Worklist.enqueueBlock(Successor);
-        }
-      }
-    }
-  }
-
-  void dump() const {
-    llvm::dbgs() << "==========================================\n";
-    llvm::dbgs() << "       Dataflow results:\n";
-    llvm::dbgs() << "==========================================\n";
-    const CFGBlock &B = Cfg.getExit();
-    getExitState(&B).dump(llvm::dbgs());
-  }
-
-  LifetimeLattice getEntryState(const CFGBlock *B) const {
-    return BlockEntryStates.lookup(B);
-  }
-
-  LifetimeLattice getExitState(const CFGBlock *B) const {
-    return BlockExitStates.lookup(B);
+private:
+  LoanSet getLoans(Lattice L, OriginID OID) {
+    if (auto *Loans = L.Origins.lookup(OID))
+      return *Loans;
+    return Factory.LoanSetFact.getEmptySet();
   }
 };
 
 // ========================================================================= //
-//  TODO: Analysing dataflow results and error reporting.
+//  TODO: 
+// - Modifying loan propagation to answer `LoanSet getLoans(Origin O, Point P)`
+// - Adding loan expiry analysis to answer `bool isExpired(Loan L, Point P)`
+// - Adding origin liveness analysis to answer `bool isLive(Origin O, Point P)`
+// - Using the above three to perform the final error reporting.
 // ========================================================================= //
 } // anonymous namespace
 
@@ -755,8 +775,9 @@ void runLifetimeSafetyAnalysis(const DeclContext &DC, const CFG &Cfg,
   ///    blocks; only Decls are visible.  Therefore, loans in a block that
   ///    never reach an Origin associated with a Decl can be safely dropped by
   ///    the analysis.
-  LifetimeDataflow Dataflow(Cfg, FactMgr, AC);
-  Dataflow.run();
-  DEBUG_WITH_TYPE("LifetimeDataflow", Dataflow.dump());
+  LifetimeFactory LifetimeFact;
+  LoanPropagationAnalysis LoanPropagation(Cfg, AC, FactMgr, LifetimeFact);
+  LoanPropagation.run();
+  DEBUG_WITH_TYPE("LifetimeLoanPropagation", LoanPropagation.dump());
 }
 } // namespace clang
diff --git a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
index 38dfdb98f08fc..0e98904ade86a 100644
--- a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
+++ b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -debug-only=LifetimeFacts,LifetimeDataflow -Wexperimental-lifetime-safety %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -mllvm -debug-only=LifetimeFacts,LifetimeLoanPropagation -Wexperimental-lifetime-safety %s 2>&1 | FileCheck %s
 // REQUIRES: asserts
 
 struct MyObj {
@@ -19,7 +19,7 @@ MyObj* return_local_addr() {
 // CHECK:   ReturnOfOrigin (OriginID: [[O_RET_VAL]])
 // CHECK:   Expire (LoanID: [[L_X]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_X]] contains Loan [[L_X]]
 // CHECK-DAG: Origin [[O_P]] contains Loan [[L_X]]
 // CHECK-DAG: Origin [[O_RET_VAL]] contains Loan [[L_X]]
@@ -47,7 +47,7 @@ MyObj* assign_and_return_local_addr() {
 // CHECK: ReturnOfOrigin (OriginID: [[O_PTR2_RVAL_2]])
 // CHECK: Expire (LoanID: [[L_Y]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_Y]] contains Loan [[L_Y]]
 // CHECK-DAG: Origin [[O_PTR1]] contains Loan [[L_Y]]
 // CHECK-DAG: Origin [[O_PTR2]] contains Loan [[L_Y]]
@@ -65,7 +65,7 @@ int return_int_val() {
   return x;
 }
 // CHECK-NEXT: End of Block
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK:  <empty>
 
 
@@ -79,7 +79,7 @@ void loan_expires_cpp() {
 // CHECK: AssignOrigin (DestID: [[O_POBJ:[0-9]+]], SrcID: [[O_ADDR_OBJ]])
 // CHECK: Expire (LoanID: [[L_OBJ]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_OBJ]] contains Loan [[L_OBJ]]
 // CHECK-DAG: Origin [[O_POBJ]] contains Loan [[L_OBJ]]
 
@@ -96,7 +96,7 @@ void loan_expires_trivial() {
 // CHECK-NEXT: End of Block
   // FIXME: Add check for Expire once trivial destructors are handled for expiration.
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_TRIVIAL_OBJ]] contains Loan [[L_TRIVIAL_OBJ]]
 // CHECK-DAG: Origin [[O_PTOBJ]] contains Loan [[L_TRIVIAL_OBJ]]
 
@@ -119,7 +119,7 @@ void conditional(bool condition) {
   // CHECK: AssignOrigin (DestID: [[O_P_RVAL:[0-9]+]], SrcID: [[O_P]])
   // CHECK: AssignOrigin (DestID: [[O_Q:[0-9]+]], SrcID: [[O_P_RVAL]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_A]] contains Loan [[L_A]]
 // CHECK-DAG: Origin [[O_ADDR_B]] contains Loan [[L_B]]
 // CHECK-DAG: Origin [[O_P]] contains Loan [[L_A]]
@@ -163,7 +163,7 @@ void pointers_in_a_cycle(bool condition) {
 }
 // At the end of the analysis, the origins for the po...
[truncated]

[llvmbot]

@llvm/pr-subscribers-clang-analysis

Author: Utkarsh Saxena (usx95)

Changes

Refactored the lifetime safety analysis to use a generic dataflow framework with a policy-based design.

Changes

• Introduced a generic DataflowAnalysis template class that can be specialized for different analyses
• Renamed LifetimeLattice to LoanPropagationLattice to better reflect its purpose
• Created a LoanPropagationAnalysis class that inherits from the generic framework
• Moved transfer functions from the standalone Transferer class into the analysis class
• Restructured the code to separate the dataflow engine from the specific analysis logic
• Updated debug output and test expectations to use the new class names

Motivation

In order to add more analyses, e.g. loan expiry and origin liveness, the previous implementation would have separate, nearly identical dataflow runners for each analysis. This change creates a single, reusable component, which will make it much simpler to add subsequent analyses without repeating boilerplate code.

This is quite close to the existing dataflow framework!

---

Patch is 22.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/148222.diff

2 Files Affected:

• (modified) clang/lib/Analysis/LifetimeSafety.cpp (+188-167)
• (modified) clang/test/Sema/warn-lifetime-safety-dataflow.cpp (+15-15)

diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp
index bf67bea6c9933..99bd7bc36faf5 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/clang/lib/Analysis/LifetimeSafety.cpp
@@ -496,7 +496,130 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
 };
 
 // ========================================================================= //
-//                              The Dataflow Lattice
+//                         Generic Dataflow Analysis
+// ========================================================================= //
+/// A generic, policy-based driver for forward dataflow analyses. It combines
+/// the dataflow runner and the transferer logic into a single class hierarchy.
+///
+/// The derived class is expected to provide:
+/// - A `Lattice` type.
+/// - `const char *getAnalysisName() const`
+/// - `Lattice getInitialState();` The initial state at the function entry.
+/// - `Lattice join(Lattice, Lattice);` Merges states from multiple CFG paths.
+/// - `Lattice transfer(Lattice, const FactType&);` Defines how a single
+///   lifetime-relevant `Fact` transforms the lattice state. Only overloads
+///   for facts relevant to the analysis need to be implemented.
+///
+/// \tparam Derived The CRTP derived class that implements the specific
+/// analysis.
+/// \tparam LatticeType The lattice type used by the analysis.
+/// TODO: Maybe use the dataflow framework! The framework might need changes
+/// to support the current comparison done at block-entry.
+template <typename Derived, typename LatticeType> class DataflowAnalysis {
+public:
+  using Lattice = LatticeType;
+
+private:
+  const CFG &Cfg;
+  AnalysisDeclContext &AC;
+
+  llvm::DenseMap<const CFGBlock *, Lattice> BlockEntryStates;
+  llvm::DenseMap<const CFGBlock *, Lattice> BlockExitStates;
+
+protected:
+  FactManager &AllFacts;
+
+  explicit DataflowAnalysis(const CFG &C, AnalysisDeclContext &AC,
+                            FactManager &F)
+      : Cfg(C), AC(AC), AllFacts(F) {}
+
+public:
+  void run() {
+    Derived &D = static_cast<Derived &>(*this);
+    llvm::TimeTraceScope Time(D.getAnalysisName());
+
+    ForwardDataflowWorklist Worklist(Cfg, AC);
+    const CFGBlock *Entry = &Cfg.getEntry();
+    BlockEntryStates[Entry] = D.getInitialState();
+    Worklist.enqueueBlock(Entry);
+
+    while (const CFGBlock *B = Worklist.dequeue()) {
+      Lattice EntryState = getEntryState(B);
+      Lattice ExitState = transferBlock(B, EntryState);
+      BlockExitStates[B] = ExitState;
+
+      for (const CFGBlock *Successor : B->succs()) {
+        auto SuccIt = BlockEntryStates.find(Successor);
+        Lattice OldSuccEntryState = (SuccIt != BlockEntryStates.end())
+                                        ? SuccIt->second
+                                        : D.getInitialState();
+        Lattice NewSuccEntryState = D.join(OldSuccEntryState, ExitState);
+        // Enqueue the successor if its entry state has changed.
+        // TODO(opt): Consider changing 'join' to report a change if !=
+        // comparison is found expensive.
+        if (SuccIt == BlockEntryStates.end() ||
+            NewSuccEntryState != OldSuccEntryState) {
+          BlockEntryStates[Successor] = NewSuccEntryState;
+          Worklist.enqueueBlock(Successor);
+        }
+      }
+    }
+  }
+
+  Lattice getEntryState(const CFGBlock *B) const {
+    return BlockEntryStates.lookup(B);
+  }
+
+  Lattice getExitState(const CFGBlock *B) const {
+    return BlockExitStates.lookup(B);
+  }
+
+  void dump() const {
+    const Derived *D = static_cast<const Derived *>(this);
+    llvm::dbgs() << "==========================================\n";
+    llvm::dbgs() << "   " << D->getAnalysisName() << " results:\n";
+    llvm::dbgs() << "==========================================\n";
+    const CFGBlock &B = Cfg.getExit();
+    getExitState(&B).dump(llvm::dbgs());
+  }
+
+private:
+  /// Computes the exit state of a block by applying all its facts sequentially
+  /// to a given entry state.
+  /// TODO: We might need to store intermediate states per-fact in the block for
+  /// later analysis.
+  Lattice transferBlock(const CFGBlock *Block, Lattice EntryState) {
+    Lattice BlockState = EntryState;
+    for (const Fact *F : AllFacts.getFacts(Block)) {
+      BlockState = transferFact(BlockState, F);
+    }
+    return BlockState;
+  }
+
+  Lattice transferFact(Lattice In, const Fact *F) {
+    Derived *d = static_cast<Derived *>(this);
+    switch (F->getKind()) {
+    case Fact::Kind::Issue:
+      return d->transfer(In, *F->getAs<IssueFact>());
+    case Fact::Kind::Expire:
+      return d->transfer(In, *F->getAs<ExpireFact>());
+    case Fact::Kind::AssignOrigin:
+      return d->transfer(In, *F->getAs<AssignOriginFact>());
+    case Fact::Kind::ReturnOfOrigin:
+      return d->transfer(In, *F->getAs<ReturnOfOriginFact>());
+    }
+    llvm_unreachable("Unknown fact kind");
+  }
+
+public:
+  Lattice transfer(Lattice In, const IssueFact &) { return In; }
+  Lattice transfer(Lattice In, const ExpireFact &) { return In; }
+  Lattice transfer(Lattice In, const AssignOriginFact &) { return In; }
+  Lattice transfer(Lattice In, const ReturnOfOriginFact &) { return In; }
+};
+
+// ========================================================================= //
+//                          Loan Propagation Analysis
 // ========================================================================= //
 
 // Using LLVM's immutable collections is efficient for dataflow analysis
@@ -517,10 +640,10 @@ struct LifetimeFactory {
   }
 };
 
-/// LifetimeLattice represents the state of our analysis at a given program
-/// point. It is an immutable object, and all operations produce a new
+/// LoanPropagationLattice represents the state of our analysis at a given
+/// program point. It is an immutable object, and all operations produce a new
 /// instance rather than modifying the existing one.
-struct LifetimeLattice {
+struct LoanPropagationLattice {
   /// The map from an origin to the set of loans it contains.
   /// The lattice has a finite height: An origin's loan set is bounded by the
   /// total number of loans in the function.
@@ -528,61 +651,16 @@ struct LifetimeLattice {
   /// not expressions, because expressions are not visible across blocks.
   OriginLoanMap Origins = OriginLoanMap(nullptr);
 
-  explicit LifetimeLattice(const OriginLoanMap &S) : Origins(S) {}
-  LifetimeLattice() = default;
+  explicit LoanPropagationLattice(const OriginLoanMap &S) : Origins(S) {}
+  LoanPropagationLattice() = default;
 
-  bool operator==(const LifetimeLattice &Other) const {
+  bool operator==(const LoanPropagationLattice &Other) const {
     return Origins == Other.Origins;
   }
-  bool operator!=(const LifetimeLattice &Other) const {
+  bool operator!=(const LoanPropagationLattice &Other) const {
     return !(*this == Other);
   }
 
-  LoanSet getLoans(OriginID OID) const {
-    if (auto *Loans = Origins.lookup(OID))
-      return *Loans;
-    return LoanSet(nullptr);
-  }
-
-  /// Computes the union of two lattices by performing a key-wise join of
-  /// their OriginLoanMaps.
-  // TODO(opt): This key-wise join is a performance bottleneck. A more
-  // efficient merge could be implemented using a Patricia Trie or HAMT
-  // instead of the current AVL-tree-based ImmutableMap.
-  // TODO(opt): Keep the state small by removing origins which become dead.
-  LifetimeLattice join(const LifetimeLattice &Other,
-                       LifetimeFactory &Factory) const {
-    /// Merge the smaller map into the larger one ensuring we iterate over the
-    /// smaller map.
-    if (Origins.getHeight() < Other.Origins.getHeight())
-      return Other.join(*this, Factory);
-
-    OriginLoanMap JoinedState = Origins;
-    // For each origin in the other map, union its loan set with ours.
-    for (const auto &Entry : Other.Origins) {
-      OriginID OID = Entry.first;
-      LoanSet OtherLoanSet = Entry.second;
-      JoinedState = Factory.OriginMapFactory.add(
-          JoinedState, OID, join(getLoans(OID), OtherLoanSet, Factory));
-    }
-    return LifetimeLattice(JoinedState);
-  }
-
-  LoanSet join(LoanSet a, LoanSet b, LifetimeFactory &Factory) const {
-    /// Merge the smaller set into the larger one ensuring we iterate over the
-    /// smaller set.
-    if (a.getHeight() < b.getHeight())
-      std::swap(a, b);
-    LoanSet Result = a;
-    for (LoanID LID : b) {
-      /// TODO(opt): Profiling shows that this loop is a major performance
-      /// bottleneck. Investigate using a BitVector to represent the set of
-      /// loans for improved join performance.
-      Result = Factory.LoanSetFact.add(Result, LID);
-    }
-    return Result;
-  }
-
   void dump(llvm::raw_ostream &OS) const {
     OS << "LifetimeLattice State:\n";
     if (Origins.isEmpty())
@@ -596,143 +674,85 @@ struct LifetimeLattice {
   }
 };
 
-// ========================================================================= //
-//                              The Transfer Function
-// ========================================================================= //
-class Transferer {
-  FactManager &AllFacts;
+class LoanPropagationAnalysis
+    : public DataflowAnalysis<LoanPropagationAnalysis, LoanPropagationLattice> {
+
   LifetimeFactory &Factory;
 
 public:
-  explicit Transferer(FactManager &F, LifetimeFactory &Factory)
-      : AllFacts(F), Factory(Factory) {}
+  LoanPropagationAnalysis(const CFG &C, AnalysisDeclContext &AC, FactManager &F,
+                          LifetimeFactory &Factory)
+      : DataflowAnalysis(C, AC, F), Factory(Factory) {}
 
-  /// Computes the exit state of a block by applying all its facts sequentially
-  /// to a given entry state.
-  /// TODO: We might need to store intermediate states per-fact in the block for
-  /// later analysis.
-  LifetimeLattice transferBlock(const CFGBlock *Block,
-                                LifetimeLattice EntryState) {
-    LifetimeLattice BlockState = EntryState;
-    llvm::ArrayRef<const Fact *> Facts = AllFacts.getFacts(Block);
+  using DataflowAnalysis<LoanPropagationAnalysis, Lattice>::transfer;
 
-    for (const Fact *F : Facts) {
-      BlockState = transferFact(BlockState, F);
+  const char *getAnalysisName() const { return "LoanPropagation"; }
+
+  Lattice getInitialState() { return Lattice{}; }
+
+  /// Computes the union of two lattices by performing a key-wise join of
+  /// their OriginLoanMaps.
+  // TODO(opt): This key-wise join is a performance bottleneck. A more
+  // efficient merge could be implemented using a Patricia Trie or HAMT
+  // instead of the current AVL-tree-based ImmutableMap.
+  // TODO(opt): Keep the state small by removing origins which become dead.
+  Lattice join(Lattice A, Lattice B) {
+    /// Merge the smaller map into the larger one ensuring we iterate over the
+    /// smaller map.
+    if (A.Origins.getHeight() < B.Origins.getHeight())
+      std::swap(A, B);
+
+    OriginLoanMap JoinedState = A.Origins;
+    // For each origin in the other map, union its loan set with ours.
+    for (const auto &Entry : B.Origins) {
+      OriginID OID = Entry.first;
+      LoanSet OtherLoanSet = Entry.second;
+      JoinedState = Factory.OriginMapFactory.add(
+          JoinedState, OID, join(getLoans(A, OID), OtherLoanSet));
     }
-    return BlockState;
+    return Lattice(JoinedState);
   }
 
-private:
-  LifetimeLattice transferFact(LifetimeLattice In, const Fact *F) {
-    switch (F->getKind()) {
-    case Fact::Kind::Issue:
-      return transfer(In, *F->getAs<IssueFact>());
-    case Fact::Kind::AssignOrigin:
-      return transfer(In, *F->getAs<AssignOriginFact>());
-    // Expire and ReturnOfOrigin facts don't modify the Origins and the State.
-    case Fact::Kind::Expire:
-    case Fact::Kind::ReturnOfOrigin:
-      return In;
-    }
-    llvm_unreachable("Unknown fact kind");
+  LoanSet join(LoanSet A, LoanSet B) {
+    if (A.getHeight() < B.getHeight())
+      std::swap(A, B);
+    for (LoanID L : B)
+      A = Factory.LoanSetFact.add(A, L);
+    return A;
   }
 
   /// A new loan is issued to the origin. Old loans are erased.
-  LifetimeLattice transfer(LifetimeLattice In, const IssueFact &F) {
+  Lattice transfer(Lattice In, const IssueFact &F) {
     OriginID OID = F.getOriginID();
     LoanID LID = F.getLoanID();
-    return LifetimeLattice(Factory.OriginMapFactory.add(
+    return LoanPropagationLattice(Factory.OriginMapFactory.add(
         In.Origins, OID, Factory.createLoanSet(LID)));
   }
 
   /// The destination origin's loan set is replaced by the source's.
   /// This implicitly "resets" the old loans of the destination.
-  LifetimeLattice transfer(LifetimeLattice InState, const AssignOriginFact &F) {
+  Lattice transfer(Lattice In, const AssignOriginFact &F) {
     OriginID DestOID = F.getDestOriginID();
     OriginID SrcOID = F.getSrcOriginID();
-    LoanSet SrcLoans = InState.getLoans(SrcOID);
-    return LifetimeLattice(
-        Factory.OriginMapFactory.add(InState.Origins, DestOID, SrcLoans));
+    LoanSet SrcLoans = getLoans(In, SrcOID);
+    return LoanPropagationLattice(
+        Factory.OriginMapFactory.add(In.Origins, DestOID, SrcLoans));
   }
-};
 
-// ========================================================================= //
-//                              Dataflow analysis
-// ========================================================================= //
-
-/// Drives the intra-procedural dataflow analysis.
-///
-/// Orchestrates the analysis by iterating over the CFG using a worklist
-/// algorithm. It computes a fixed point by propagating the LifetimeLattice
-/// state through each block until the state no longer changes.
-/// TODO: Maybe use the dataflow framework! The framework might need changes
-/// to support the current comparison done at block-entry.
-class LifetimeDataflow {
-  const CFG &Cfg;
-  AnalysisDeclContext &AC;
-  LifetimeFactory LifetimeFact;
-
-  Transferer Xfer;
-
-  /// Stores the merged analysis state at the entry of each CFG block.
-  llvm::DenseMap<const CFGBlock *, LifetimeLattice> BlockEntryStates;
-  /// Stores the analysis state at the exit of each CFG block, after the
-  /// transfer function has been applied.
-  llvm::DenseMap<const CFGBlock *, LifetimeLattice> BlockExitStates;
-
-public:
-  LifetimeDataflow(const CFG &C, FactManager &FS, AnalysisDeclContext &AC)
-      : Cfg(C), AC(AC), Xfer(FS, LifetimeFact) {}
-
-  void run() {
-    llvm::TimeTraceScope TimeProfile("Lifetime Dataflow");
-    ForwardDataflowWorklist Worklist(Cfg, AC);
-    const CFGBlock *Entry = &Cfg.getEntry();
-    BlockEntryStates[Entry] = LifetimeLattice{};
-    Worklist.enqueueBlock(Entry);
-    while (const CFGBlock *B = Worklist.dequeue()) {
-      LifetimeLattice EntryState = getEntryState(B);
-      LifetimeLattice ExitState = Xfer.transferBlock(B, EntryState);
-      BlockExitStates[B] = ExitState;
-
-      for (const CFGBlock *Successor : B->succs()) {
-        auto SuccIt = BlockEntryStates.find(Successor);
-        LifetimeLattice OldSuccEntryState = (SuccIt != BlockEntryStates.end())
-                                                ? SuccIt->second
-                                                : LifetimeLattice{};
-        LifetimeLattice NewSuccEntryState =
-            OldSuccEntryState.join(ExitState, LifetimeFact);
-        // Enqueue the successor if its entry state has changed.
-        // TODO(opt): Consider changing 'join' to report a change if !=
-        // comparison is found expensive.
-        if (SuccIt == BlockEntryStates.end() ||
-            NewSuccEntryState != OldSuccEntryState) {
-          BlockEntryStates[Successor] = NewSuccEntryState;
-          Worklist.enqueueBlock(Successor);
-        }
-      }
-    }
-  }
-
-  void dump() const {
-    llvm::dbgs() << "==========================================\n";
-    llvm::dbgs() << "       Dataflow results:\n";
-    llvm::dbgs() << "==========================================\n";
-    const CFGBlock &B = Cfg.getExit();
-    getExitState(&B).dump(llvm::dbgs());
-  }
-
-  LifetimeLattice getEntryState(const CFGBlock *B) const {
-    return BlockEntryStates.lookup(B);
-  }
-
-  LifetimeLattice getExitState(const CFGBlock *B) const {
-    return BlockExitStates.lookup(B);
+private:
+  LoanSet getLoans(Lattice L, OriginID OID) {
+    if (auto *Loans = L.Origins.lookup(OID))
+      return *Loans;
+    return Factory.LoanSetFact.getEmptySet();
   }
 };
 
 // ========================================================================= //
-//  TODO: Analysing dataflow results and error reporting.
+//  TODO: 
+// - Modifying loan propagation to answer `LoanSet getLoans(Origin O, Point P)`
+// - Adding loan expiry analysis to answer `bool isExpired(Loan L, Point P)`
+// - Adding origin liveness analysis to answer `bool isLive(Origin O, Point P)`
+// - Using the above three to perform the final error reporting.
 // ========================================================================= //
 } // anonymous namespace
 
@@ -755,8 +775,9 @@ void runLifetimeSafetyAnalysis(const DeclContext &DC, const CFG &Cfg,
   ///    blocks; only Decls are visible.  Therefore, loans in a block that
   ///    never reach an Origin associated with a Decl can be safely dropped by
   ///    the analysis.
-  LifetimeDataflow Dataflow(Cfg, FactMgr, AC);
-  Dataflow.run();
-  DEBUG_WITH_TYPE("LifetimeDataflow", Dataflow.dump());
+  LifetimeFactory LifetimeFact;
+  LoanPropagationAnalysis LoanPropagation(Cfg, AC, FactMgr, LifetimeFact);
+  LoanPropagation.run();
+  DEBUG_WITH_TYPE("LifetimeLoanPropagation", LoanPropagation.dump());
 }
 } // namespace clang
diff --git a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
index 38dfdb98f08fc..0e98904ade86a 100644
--- a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
+++ b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -debug-only=LifetimeFacts,LifetimeDataflow -Wexperimental-lifetime-safety %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -mllvm -debug-only=LifetimeFacts,LifetimeLoanPropagation -Wexperimental-lifetime-safety %s 2>&1 | FileCheck %s
 // REQUIRES: asserts
 
 struct MyObj {
@@ -19,7 +19,7 @@ MyObj* return_local_addr() {
 // CHECK:   ReturnOfOrigin (OriginID: [[O_RET_VAL]])
 // CHECK:   Expire (LoanID: [[L_X]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_X]] contains Loan [[L_X]]
 // CHECK-DAG: Origin [[O_P]] contains Loan [[L_X]]
 // CHECK-DAG: Origin [[O_RET_VAL]] contains Loan [[L_X]]
@@ -47,7 +47,7 @@ MyObj* assign_and_return_local_addr() {
 // CHECK: ReturnOfOrigin (OriginID: [[O_PTR2_RVAL_2]])
 // CHECK: Expire (LoanID: [[L_Y]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_Y]] contains Loan [[L_Y]]
 // CHECK-DAG: Origin [[O_PTR1]] contains Loan [[L_Y]]
 // CHECK-DAG: Origin [[O_PTR2]] contains Loan [[L_Y]]
@@ -65,7 +65,7 @@ int return_int_val() {
   return x;
 }
 // CHECK-NEXT: End of Block
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK:  <empty>
 
 
@@ -79,7 +79,7 @@ void loan_expires_cpp() {
 // CHECK: AssignOrigin (DestID: [[O_POBJ:[0-9]+]], SrcID: [[O_ADDR_OBJ]])
 // CHECK: Expire (LoanID: [[L_OBJ]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_OBJ]] contains Loan [[L_OBJ]]
 // CHECK-DAG: Origin [[O_POBJ]] contains Loan [[L_OBJ]]
 
@@ -96,7 +96,7 @@ void loan_expires_trivial() {
 // CHECK-NEXT: End of Block
   // FIXME: Add check for Expire once trivial destructors are handled for expiration.
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_TRIVIAL_OBJ]] contains Loan [[L_TRIVIAL_OBJ]]
 // CHECK-DAG: Origin [[O_PTOBJ]] contains Loan [[L_TRIVIAL_OBJ]]
 
@@ -119,7 +119,7 @@ void conditional(bool condition) {
   // CHECK: AssignOrigin (DestID: [[O_P_RVAL:[0-9]+]], SrcID: [[O_P]])
   // CHECK: AssignOrigin (DestID: [[O_Q:[0-9]+]], SrcID: [[O_P_RVAL]])
 }
-// CHECK: Dataflow results:
+// CHECK: LoanPropagation results:
 // CHECK-DAG: Origin [[O_ADDR_A]] contains Loan [[L_A]]
 // CHECK-DAG: Origin [[O_ADDR_B]] contains Loan [[L_B]]
 // CHECK-DAG: Origin [[O_P]] contains Loan [[L_A]]
@@ -163,7 +163,7 @@ void pointers_in_a_cycle(bool condition) {
 }
 // At the end of the analysis, the origins for the po...
[truncated]

[Xazax-hun]

Overall looks good to me. One question for the future: sometimes it might be better to run certain analyses side by side in the same worklist rather than running them sequentially after each other. Do you think there are some analysis steps that will be beneficial to be combined in that way?