[LifetimeSafety] Track view types/gsl::Pointer.

Author: usx95

clang/lib/Analysis/LifetimeSafety.cpp

@@ -8,6 +8,7 @@
 #include "clang/Analysis/Analyses/LifetimeSafety.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Analysis/Analyses/PostOrderCFGView.h"
@@ -387,25 +388,16 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
   using Base = ConstStmtVisitor<FactGenerator>;
 
 public:
-  FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
-      : FactMgr(FactMgr), AC(AC) {}
+  FactGenerator(FactManager &FactMgr) : FactMgr(FactMgr) {}
 
-  void run() {
-    llvm::TimeTraceScope TimeProfile("FactGenerator");
-    // Iterate through the CFG blocks in reverse post-order to ensure that
-    // initializations and destructions are processed in the correct sequence.
-    for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
-      CurrentBlockFacts.clear();
-      for (unsigned I = 0; I < Block->size(); ++I) {
-        const CFGElement &Element = Block->Elements[I];
-        if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
-          Visit(CS->getStmt());
-        else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
-                     Element.getAs<CFGAutomaticObjDtor>())
-          handleDestructor(*DtorOpt);
-      }
-      FactMgr.addBlockFacts(Block, CurrentBlockFacts);
-    }
+  void startBlock(const CFGBlock *Block) {
+    CurrentBlock = Block;
+    CurrentBlockFacts.clear();
+  }
+
+  void endBlock() {
+    FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
+    startBlock(nullptr);
   }
 
   void VisitDeclStmt(const DeclStmt *DS) {
@@ -425,7 +417,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
   void VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
     if (!hasOrigin(ICE->getType()))
       return;
-    Visit(ICE->getSubExpr());
     // An ImplicitCastExpr node itself gets an origin, which flows from the
     // origin of its sub-expression (after stripping its own parens/casts).
     // TODO: Consider if this is actually useful in practice. Alternatively, we
@@ -493,18 +484,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
     Base::VisitCXXFunctionalCastExpr(FCE);
   }
 
-private:
-  // Check if a type has an origin.
-  bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
-
-  template <typename Destination, typename Source>
-  void addAssignOriginFact(const Destination &D, const Source &S) {
-    OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
-    OriginID SrcOID = FactMgr.getOriginMgr().get(S);
-    CurrentBlockFacts.push_back(
-        FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
-  }
-
   void handleDestructor(const CFGAutomaticObjDtor &DtorOpt) {
     /// TODO: Also handle trivial destructors (e.g., for `int`
     /// variables) which will never have a CFGAutomaticObjDtor node.
@@ -526,6 +505,17 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
             L.ID, DtorOpt.getTriggerStmt()->getEndLoc()));
     }
   }
+private:
+  // Check if a type has an origin.
+  bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
+
+  template <typename Destination, typename Source>
+  void addAssignOriginFact(const Destination &D, const Source &S) {
+    OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
+    OriginID SrcOID = FactMgr.getOriginMgr().get(S);
+    CurrentBlockFacts.push_back(
+        FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
+  }
 
   /// Checks if the expression is a `void("__lifetime_test_point_...")` cast.
   /// If so, creates a `TestPointFact` and returns true.
@@ -549,10 +539,59 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
   }
 
   FactManager &FactMgr;
-  AnalysisDeclContext &AC;
+  const CFGBlock *CurrentBlock = nullptr;
   llvm::SmallVector<Fact *> CurrentBlockFacts;
 };
 
+class FactGeneratorDriver : public RecursiveASTVisitor<FactGeneratorDriver> {
+public:
+  FactGeneratorDriver(FactGenerator &FG, AnalysisDeclContext &AC)
+      : FG(FG), AC(AC) {}
+  bool shouldTraversePostOrder() const { return true; }
+  void run() {
+    llvm::TimeTraceScope TimeProfile("FactGenerator");
+    // Iterate through the CFG blocks in reverse post-order to ensure that
+    // initializations and destructions are processed in the correct sequence.
+    for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
+      FactGeneratorBlockRAII BlockGenerator(FG, Block);
+      for (const CFGElement &Element : *Block) {
+        if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
+          TraverseStmt(const_cast<Stmt *>(CS->getStmt()));
+        else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
+                     Element.getAs<CFGAutomaticObjDtor>())
+          FG.handleDestructor(*DtorOpt);
+      }
+    }
+  }
+
+  bool TraverseStmt(Stmt *S) {
+    // Avoid re-visiting nodes to not create duplicate facts.
+    if (!S || !VisitedStmts.insert(S).second)
+      return true;
+    return RecursiveASTVisitor::TraverseStmt(S);
+  }
+
+  bool VisitStmt(Stmt *S) {
+    FG.Visit(S);
+    return true; // Continue traversing to children.
+  }
+
+private:
+  struct FactGeneratorBlockRAII {
+    FactGeneratorBlockRAII(FactGenerator &FG, const CFGBlock *Block) : FG(FG) {
+      FG.startBlock(Block);
+    }
+    ~FactGeneratorBlockRAII() { FG.endBlock(); }
+
+  private:
+    FactGenerator FG;
+  };
+
+  FactGenerator &FG;
+  AnalysisDeclContext &AC;
+  llvm::DenseSet<const Stmt *> VisitedStmts;
+};
+
 // ========================================================================= //
 //                         Generic Dataflow Analysis
 // ========================================================================= //
@@ -1096,8 +1135,9 @@ void LifetimeSafetyAnalysis::run() {
   DEBUG_WITH_TYPE("PrintCFG", Cfg.dump(AC.getASTContext().getLangOpts(),
                                        /*ShowColors=*/true));
 
-  FactGenerator FactGen(*FactMgr, AC);
-  FactGen.run();
+  FactGenerator Generator(*FactMgr);
+  FactGeneratorDriver Driver(Generator, AC);
+  Driver.run();
   DEBUG_WITH_TYPE("LifetimeFacts", FactMgr->dump(Cfg, AC));
 
   /// TODO(opt): Consider optimizing individual blocks before running the

clang/unittests/Analysis/LifetimeSafetyTest.cpp

@@ -11,6 +11,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Testing/TestAST.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <optional>
@@ -20,6 +21,7 @@ namespace clang::lifetimes::internal {
 namespace {
 
 using namespace ast_matchers;
+using ::testing::SizeIs;
 using ::testing::UnorderedElementsAreArray;
 
 // A helper class to run the full lifetime analysis on a piece of code
@@ -96,21 +98,18 @@ class LifetimeTestHelper {
     return OID;
   }
 
-  std::optional<LoanID> getLoanForVar(llvm::StringRef VarName) {
+  std::vector<LoanID> getLoansForVar(llvm::StringRef VarName) {
     auto *VD = findDecl<VarDecl>(VarName);
-    if (!VD)
-      return std::nullopt;
+    if (!VD) {
+      ADD_FAILURE() << "No VarDecl found for '" << VarName << "'";
+      return {};
+    }
     std::vector<LoanID> LID = Analysis.getLoanIDForVar(VD);
     if (LID.empty()) {
       ADD_FAILURE() << "Loan for '" << VarName << "' not found.";
-      return std::nullopt;
-    }
-    // TODO: Support retrieving more than one loans to a var.
-    if (LID.size() > 1) {
-      ADD_FAILURE() << "More than 1 loans found for '" << VarName;
-      return std::nullopt;
+      return {};
     }
-    return LID[0];
+    return LID;
   }
 
   std::optional<LoanSet> getLoansAtPoint(OriginID OID,
@@ -121,13 +120,12 @@ class LifetimeTestHelper {
     return Analysis.getLoansAtPoint(OID, PP);
   }
 
-  std::optional<llvm::DenseSet<LoanID>>
+  std::optional<std::vector<LoanID>>
   getExpiredLoansAtPoint(llvm::StringRef Annotation) {
     ProgramPoint PP = Runner.getProgramPoint(Annotation);
     if (!PP)
       return std::nullopt;
-    auto Expired = Analysis.getExpiredLoansAtPoint(PP);
-    return llvm::DenseSet<LoanID>{Expired.begin(), Expired.end()};
+    return Analysis.getExpiredLoansAtPoint(PP);
   }
 
 private:
@@ -197,12 +195,13 @@ MATCHER_P2(HasLoansToImpl, LoanVars, Annotation, "") {
 
   std::vector<LoanID> ExpectedLoans;
   for (const auto &LoanVar : LoanVars) {
-    std::optional<LoanID> ExpectedLIDOpt = Info.Helper.getLoanForVar(LoanVar);
-    if (!ExpectedLIDOpt) {
+    std::vector<LoanID> ExpectedLIDs = Info.Helper.getLoansForVar(LoanVar);
+    if (ExpectedLIDs.empty()) {
       *result_listener << "could not find loan for var '" << LoanVar << "'";
       return false;
     }
-    ExpectedLoans.push_back(*ExpectedLIDOpt);
+    ExpectedLoans.insert(ExpectedLoans.end(), ExpectedLIDs.begin(),
+                         ExpectedLIDs.end());
   }
 
   return ExplainMatchResult(UnorderedElementsAreArray(ExpectedLoans),
@@ -221,17 +220,17 @@ MATCHER_P(AreExpiredAt, Annotation, "") {
                      << Annotation << "'";
     return false;
   }
-  std::vector<LoanID> ActualExpiredLoans(ActualExpiredSetOpt->begin(),
-                                         ActualExpiredSetOpt->end());
+  std::vector<LoanID> ActualExpiredLoans = *ActualExpiredSetOpt;
   std::vector<LoanID> ExpectedExpiredLoans;
   for (const auto &VarName : Info.LoanVars) {
-    auto LoanIDOpt = Helper.getLoanForVar(VarName);
-    if (!LoanIDOpt) {
+    auto LoanIDs = Helper.getLoansForVar(VarName);
+    if (LoanIDs.empty()) {
       *result_listener << "could not find a loan for variable '" << VarName
                        << "'";
       return false;
     }
-    ExpectedExpiredLoans.push_back(*LoanIDOpt);
+    ExpectedExpiredLoans.insert(ExpectedExpiredLoans.end(), LoanIDs.begin(),
+                                LoanIDs.end());
   }
   return ExplainMatchResult(UnorderedElementsAreArray(ExpectedExpiredLoans),
                             ActualExpiredLoans, result_listener);
@@ -730,5 +729,17 @@ TEST_F(LifetimeAnalysisTest, ReassignedPointerThenOriginalExpires) {
   EXPECT_THAT(LoansTo({"s1", "s2"}), AreExpiredAt("p_after_s1_expires"));
 }
 
+TEST_F(LifetimeAnalysisTest, NoDuplicateLoansForImplicitCastToConst) {
+  SetupTest(R"(
+    void target() {
+      MyObj a;
+      const MyObj* p = &a;
+      const MyObj* q = &a;
+      POINT(at_end);
+    }
+  )");
+  EXPECT_THAT(Helper->getLoansForVar("a"), SizeIs(2));
+}
+
 } // anonymous namespace
 } // namespace clang::lifetimes::internal