[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"
@@ -403,29 +404,17 @@ class FactManager {
   llvm::BumpPtrAllocator FactAllocator;
 };
 
-class FactGenerator : public ConstStmtVisitor<FactGenerator> {
-  using Base = ConstStmtVisitor<FactGenerator>;
+class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
+  using Base = ConstStmtVisitor<FactGeneratorVisitor>;
 
 public:
-  FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
-      : FactMgr(FactMgr), AC(AC) {}
+  FactGeneratorVisitor(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() { CurrentBlockFacts.clear(); }
+
+  void endBlock(const CFGBlock *CurrentBlock) {
+    FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
+    CurrentBlockFacts.clear();
   }
 
   void VisitDeclStmt(const DeclStmt *DS) {
@@ -445,7 +434,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
@@ -513,18 +501,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.
@@ -547,6 +523,18 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
     }
   }
 
+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.
   bool VisitTestPoint(const CXXFunctionalCastExpr *FCE) {
@@ -569,10 +557,62 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
   }
 
   FactManager &FactMgr;
-  AnalysisDeclContext &AC;
   llvm::SmallVector<Fact *> CurrentBlockFacts;
 };
 
+class FactGenerator : public RecursiveASTVisitor<FactGenerator> {
+public:
+  FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
+      : FG(FactMgr), 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(FactGeneratorVisitor &FG, const CFGBlock *Block)
+        : FG(FG), CurBlock(Block) {
+      FG.startBlock();
+    }
+    ~FactGeneratorBlockRAII() { FG.endBlock(CurBlock); }
+
+  private:
+    FactGeneratorVisitor &FG;
+    const CFGBlock *CurBlock;
+  };
+
+  FactGeneratorVisitor FG;
+  AnalysisDeclContext &AC;
+  llvm::DenseSet<const Stmt *> VisitedStmts;
+};
+
 // ========================================================================= //
 //                         Generic Dataflow Analysis
 // ========================================================================= //
@@ -1116,8 +1156,8 @@ void LifetimeSafetyAnalysis::run() {
   DEBUG_WITH_TYPE("PrintCFG", Cfg.dump(AC.getASTContext().getLangOpts(),
                                        /*ShowColors=*/true));
 
-  FactGenerator FactGen(*FactMgr, AC);
-  FactGen.run();
+  FactGenerator FG(*FactMgr, AC);
+  FG.run();
   DEBUG_WITH_TYPE("LifetimeFacts", FactMgr->dump(Cfg, AC));
 
   /// TODO(opt): Consider optimizing individual blocks before running the

clang/test/Sema/warn-lifetime-safety-dataflow.cpp

@@ -293,3 +293,23 @@ void pointer_indirection() {
   int *q = *pp;
 // CHECK:   AssignOrigin (Dest: {{[0-9]+}} (Decl: q), Src: {{[0-9]+}} (Expr: ImplicitCastExpr))
 }
+
+// CHECK-LABEL: Function: ternary_operator
+// FIXME: Propagate origins across ConditionalOperator.
+void ternary_operator() {
+  int a, b;
+  int *p;
+  p = (a > b) ? &a : &b;
+  // CHECK: Block B2:
+  // CHECK:   Issue (LoanID: [[L_A:[0-9]+]], ToOrigin: [[O_ADDR_A:[0-9]+]] (Expr: UnaryOperator))
+  // CHECK: End of Block
+  
+  // CHECK: Block B3:
+  // CHECK:   Issue (LoanID: [[L_B:[0-9]+]], ToOrigin: [[O_ADDR_A:[0-9]+]] (Expr: UnaryOperator))
+  // CHECK: End of Block
+  
+  // CHECK: Block B1:
+  // CHECK:   AssignOrigin (Dest: [[O_P:[0-9]+]] (Decl: p), Src: {{[0-9]+}} (Expr: ConditionalOperator))
+  // CHECK: End of Block
+}
+

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