https://github.com/usx95 updated
https://github.com/llvm/llvm-project/pull/153661
>From 4576fa72454847f64a62ad0d403816cf4ad59505 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <u...@google.com>
Date: Thu, 14 Aug 2025 06:57:44 +0000
Subject: [PATCH] [LifetimeSafety] Track view types/gsl::Pointer.
---
clang/lib/Analysis/LifetimeSafety.cpp | 108 ++++++++++++------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 +-
.../Sema/warn-lifetime-safety-dataflow.cpp | 20 ++++
.../unittests/Analysis/LifetimeSafetyTest.cpp | 53 +++++----
4 files changed, 128 insertions(+), 61 deletions(-)
diff --git a/clang/lib/Analysis/LifetimeSafety.cpp
b/clang/lib/Analysis/LifetimeSafety.cpp
index c2e6dd74d0758..b318b6754531c 100644
--- a/clang/lib/Analysis/LifetimeSafety.cpp
+++ b/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,15 @@ 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 flushBlock(const CFGBlock *CurrentBlock) {
+ FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
+ CurrentBlockFacts.clear();
}
void VisitDeclStmt(const DeclStmt *DS) {
@@ -445,7 +432,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 +499,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 +521,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 +555,60 @@ 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) {}
+ ~FactGeneratorBlockRAII() { FG.flushBlock(CurBlock); }
+
+ private:
+ FactGeneratorVisitor &FG;
+ const CFGBlock *CurBlock;
+ };
+
+ FactGeneratorVisitor FG;
+ AnalysisDeclContext &AC;
+ llvm::DenseSet<const Stmt *> VisitedStmts;
+};
+
// ========================================================================= //
// Generic Dataflow Analysis
// ========================================================================= //
@@ -1116,8 +1152,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
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp
b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 0b94b1044f072..1b66d83df5171 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2905,6 +2905,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ bool EnableLifetimeSafetyAnalysis = S.getLangOpts().EnableLifetimeSafety;
+
// Force that certain expressions appear as CFGElements in the CFG. This
// is used to speed up various analyses.
// FIXME: This isn't the right factoring. This is here for initial
@@ -2912,11 +2914,10 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
// expect to always be CFGElements and then fill in the BuildOptions
// appropriately. This is essentially a layering violation.
if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis ||
- P.enableConsumedAnalysis) {
+ P.enableConsumedAnalysis || EnableLifetimeSafetyAnalysis) {
// Unreachable code analysis and thread safety require a linearized CFG.
AC.getCFGBuildOptions().setAllAlwaysAdd();
- }
- else {
+ } else {
AC.getCFGBuildOptions()
.setAlwaysAdd(Stmt::BinaryOperatorClass)
.setAlwaysAdd(Stmt::CompoundAssignOperatorClass)
@@ -2927,7 +2928,6 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
.setAlwaysAdd(Stmt::UnaryOperatorClass);
}
- bool EnableLifetimeSafetyAnalysis = S.getLangOpts().EnableLifetimeSafety;
// Install the logical handler.
std::optional<LogicalErrorHandler> LEH;
if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
diff --git a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
b/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
index bcde9adf25ca5..42fe64da6014b 100644
--- a/clang/test/Sema/warn-lifetime-safety-dataflow.cpp
+++ b/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
+}
+
diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
b/clang/unittests/Analysis/LifetimeSafetyTest.cpp
index c8d88b4ea2277..13e5832d70050 100644
--- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp
+++ b/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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
