Skip to content

Commit 440c3fe

Browse files
usx95usx95
committed
[LifetimeSafety] Track view types/gsl::Pointer.
1 parent 92a91f7 commit 440c3fe

File tree

2 files changed

+111
-57
lines changed

2 files changed

+111
-57
lines changed

‎clang/lib/Analysis/LifetimeSafety.cpp‎

Lines changed: 79 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "clang/Analysis/Analyses/LifetimeSafety.h"
99
#include "clang/AST/Decl.h"
1010
#include "clang/AST/Expr.h"
11+
#include "clang/AST/RecursiveASTVisitor.h"
1112
#include "clang/AST/StmtVisitor.h"
1213
#include "clang/AST/Type.h"
1314
#include "clang/Analysis/Analyses/PostOrderCFGView.h"
@@ -403,29 +404,20 @@ class FactManager {
403404
llvm::BumpPtrAllocator FactAllocator;
404405
};
405406

406-
class FactGenerator : public ConstStmtVisitor<FactGenerator> {
407-
using Base = ConstStmtVisitor<FactGenerator>;
407+
class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> {
408+
using Base = ConstStmtVisitor<FactGeneratorVisitor>;
408409

409410
public:
410-
FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
411-
: FactMgr(FactMgr), AC(AC) {}
411+
FactGeneratorVisitor(FactManager &FactMgr) : FactMgr(FactMgr) {}
412412

413-
void run() {
414-
llvm::TimeTraceScope TimeProfile("FactGenerator");
415-
// Iterate through the CFG blocks in reverse post-order to ensure that
416-
// initializations and destructions are processed in the correct sequence.
417-
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
418-
CurrentBlockFacts.clear();
419-
for (unsigned I = 0; I < Block->size(); ++I) {
420-
const CFGElement &Element = Block->Elements[I];
421-
if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
422-
Visit(CS->getStmt());
423-
else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
424-
Element.getAs<CFGAutomaticObjDtor>())
425-
handleDestructor(*DtorOpt);
426-
}
427-
FactMgr.addBlockFacts(Block, CurrentBlockFacts);
428-
}
413+
void startBlock(const CFGBlock *Block) {
414+
CurrentBlock = Block;
415+
CurrentBlockFacts.clear();
416+
}
417+
418+
void endBlock() {
419+
FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
420+
startBlock(nullptr);
429421
}
430422

431423
void VisitDeclStmt(const DeclStmt *DS) {
@@ -445,7 +437,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
445437
void VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
446438
if (!hasOrigin(ICE->getType()))
447439
return;
448-
Visit(ICE->getSubExpr());
449440
// An ImplicitCastExpr node itself gets an origin, which flows from the
450441
// origin of its sub-expression (after stripping its own parens/casts).
451442
// TODO: Consider if this is actually useful in practice. Alternatively, we
@@ -513,18 +504,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
513504
Base::VisitCXXFunctionalCastExpr(FCE);
514505
}
515506

516-
private:
517-
// Check if a type has an origin.
518-
bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
519-
520-
template <typename Destination, typename Source>
521-
void addAssignOriginFact(const Destination &D, const Source &S) {
522-
OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
523-
OriginID SrcOID = FactMgr.getOriginMgr().get(S);
524-
CurrentBlockFacts.push_back(
525-
FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
526-
}
527-
528507
void handleDestructor(const CFGAutomaticObjDtor &DtorOpt) {
529508
/// TODO: Also handle trivial destructors (e.g., for `int`
530509
/// variables) which will never have a CFGAutomaticObjDtor node.
@@ -547,6 +526,18 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
547526
}
548527
}
549528

529+
private:
530+
// Check if a type has an origin.
531+
bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
532+
533+
template <typename Destination, typename Source>
534+
void addAssignOriginFact(const Destination &D, const Source &S) {
535+
OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
536+
OriginID SrcOID = FactMgr.getOriginMgr().get(S);
537+
CurrentBlockFacts.push_back(
538+
FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
539+
}
540+
550541
/// Checks if the expression is a `void("__lifetime_test_point_...")` cast.
551542
/// If so, creates a `TestPointFact` and returns true.
552543
bool VisitTestPoint(const CXXFunctionalCastExpr *FCE) {
@@ -569,10 +560,62 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
569560
}
570561

571562
FactManager &FactMgr;
572-
AnalysisDeclContext &AC;
563+
const CFGBlock *CurrentBlock = nullptr;
573564
llvm::SmallVector<Fact *> CurrentBlockFacts;
574565
};
575566

567+
class FactGenerator : public RecursiveASTVisitor<FactGenerator> {
568+
public:
569+
FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
570+
: FG(FactMgr), AC(AC) {}
571+
572+
bool shouldTraversePostOrder() const { return true; }
573+
574+
void run() {
575+
llvm::TimeTraceScope TimeProfile("FactGenerator");
576+
// Iterate through the CFG blocks in reverse post-order to ensure that
577+
// initializations and destructions are processed in the correct sequence.
578+
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
579+
FactGeneratorBlockRAII BlockGenerator(FG, Block);
580+
for (const CFGElement &Element : *Block) {
581+
if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
582+
TraverseStmt(const_cast<Stmt *>(CS->getStmt()));
583+
else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
584+
Element.getAs<CFGAutomaticObjDtor>())
585+
FG.handleDestructor(*DtorOpt);
586+
}
587+
}
588+
}
589+
590+
bool TraverseStmt(Stmt *S) {
591+
// Avoid re-visiting nodes to not create duplicate facts.
592+
if (!S || !VisitedStmts.insert(S).second)
593+
return true;
594+
return RecursiveASTVisitor::TraverseStmt(S);
595+
}
596+
597+
bool VisitStmt(Stmt *S) {
598+
FG.Visit(S);
599+
return true; // Continue traversing to children.
600+
}
601+
602+
private:
603+
struct FactGeneratorBlockRAII {
604+
FactGeneratorBlockRAII(FactGeneratorVisitor &FG, const CFGBlock *Block)
605+
: FG(FG) {
606+
FG.startBlock(Block);
607+
}
608+
~FactGeneratorBlockRAII() { FG.endBlock(); }
609+
610+
private:
611+
FactGeneratorVisitor &FG;
612+
};
613+
614+
FactGeneratorVisitor FG;
615+
AnalysisDeclContext &AC;
616+
llvm::DenseSet<const Stmt *> VisitedStmts;
617+
};
618+
576619
// ========================================================================= //
577620
// Generic Dataflow Analysis
578621
// ========================================================================= //
@@ -1116,8 +1159,8 @@ void LifetimeSafetyAnalysis::run() {
11161159
DEBUG_WITH_TYPE("PrintCFG", Cfg.dump(AC.getASTContext().getLangOpts(),
11171160
/*ShowColors=*/true));
11181161

1119-
FactGenerator FactGen(*FactMgr, AC);
1120-
FactGen.run();
1162+
FactGenerator FG(*FactMgr, AC);
1163+
FG.run();
11211164
DEBUG_WITH_TYPE("LifetimeFacts", FactMgr->dump(Cfg, AC));
11221165

11231166
/// TODO(opt): Consider optimizing individual blocks before running the

‎clang/unittests/Analysis/LifetimeSafetyTest.cpp‎

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "clang/ASTMatchers/ASTMatchers.h"
1212
#include "clang/Testing/TestAST.h"
1313
#include "llvm/ADT/StringMap.h"
14+
#include "llvm/Testing/Support/Error.h"
1415
#include "gmock/gmock.h"
1516
#include "gtest/gtest.h"
1617
#include <optional>
@@ -20,6 +21,7 @@ namespace clang::lifetimes::internal {
2021
namespace {
2122

2223
using namespace ast_matchers;
24+
using ::testing::SizeIs;
2325
using ::testing::UnorderedElementsAreArray;
2426

2527
// A helper class to run the full lifetime analysis on a piece of code
@@ -96,21 +98,18 @@ class LifetimeTestHelper {
9698
return OID;
9799
}
98100

99-
std::optional<LoanID> getLoanForVar(llvm::StringRef VarName) {
101+
std::vector<LoanID> getLoansForVar(llvm::StringRef VarName) {
100102
auto *VD = findDecl<VarDecl>(VarName);
101-
if (!VD)
102-
return std::nullopt;
103+
if (!VD) {
104+
ADD_FAILURE() << "No VarDecl found for '" << VarName << "'";
105+
return {};
106+
}
103107
std::vector<LoanID> LID = Analysis.getLoanIDForVar(VD);
104108
if (LID.empty()) {
105109
ADD_FAILURE() << "Loan for '" << VarName << "' not found.";
106-
return std::nullopt;
107-
}
108-
// TODO: Support retrieving more than one loans to a var.
109-
if (LID.size() > 1) {
110-
ADD_FAILURE() << "More than 1 loans found for '" << VarName;
111-
return std::nullopt;
110+
return {};
112111
}
113-
return LID[0];
112+
return LID;
114113
}
115114

116115
std::optional<LoanSet> getLoansAtPoint(OriginID OID,
@@ -121,13 +120,12 @@ class LifetimeTestHelper {
121120
return Analysis.getLoansAtPoint(OID, PP);
122121
}
123122

124-
std::optional<llvm::DenseSet<LoanID>>
123+
std::optional<std::vector<LoanID>>
125124
getExpiredLoansAtPoint(llvm::StringRef Annotation) {
126125
ProgramPoint PP = Runner.getProgramPoint(Annotation);
127126
if (!PP)
128127
return std::nullopt;
129-
auto Expired = Analysis.getExpiredLoansAtPoint(PP);
130-
return llvm::DenseSet<LoanID>{Expired.begin(), Expired.end()};
128+
return Analysis.getExpiredLoansAtPoint(PP);
131129
}
132130

133131
private:
@@ -197,12 +195,13 @@ MATCHER_P2(HasLoansToImpl, LoanVars, Annotation, "") {
197195

198196
std::vector<LoanID> ExpectedLoans;
199197
for (const auto &LoanVar : LoanVars) {
200-
std::optional<LoanID> ExpectedLIDOpt = Info.Helper.getLoanForVar(LoanVar);
201-
if (!ExpectedLIDOpt) {
198+
std::vector<LoanID> ExpectedLIDs = Info.Helper.getLoansForVar(LoanVar);
199+
if (ExpectedLIDs.empty()) {
202200
*result_listener << "could not find loan for var '" << LoanVar << "'";
203201
return false;
204202
}
205-
ExpectedLoans.push_back(*ExpectedLIDOpt);
203+
ExpectedLoans.insert(ExpectedLoans.end(), ExpectedLIDs.begin(),
204+
ExpectedLIDs.end());
206205
}
207206

208207
return ExplainMatchResult(UnorderedElementsAreArray(ExpectedLoans),
@@ -221,17 +220,17 @@ MATCHER_P(AreExpiredAt, Annotation, "") {
221220
<< Annotation << "'";
222221
return false;
223222
}
224-
std::vector<LoanID> ActualExpiredLoans(ActualExpiredSetOpt->begin(),
225-
ActualExpiredSetOpt->end());
223+
std::vector<LoanID> ActualExpiredLoans = *ActualExpiredSetOpt;
226224
std::vector<LoanID> ExpectedExpiredLoans;
227225
for (const auto &VarName : Info.LoanVars) {
228-
auto LoanIDOpt = Helper.getLoanForVar(VarName);
229-
if (!LoanIDOpt) {
226+
auto LoanIDs = Helper.getLoansForVar(VarName);
227+
if (LoanIDs.empty()) {
230228
*result_listener << "could not find a loan for variable '" << VarName
231229
<< "'";
232230
return false;
233231
}
234-
ExpectedExpiredLoans.push_back(*LoanIDOpt);
232+
ExpectedExpiredLoans.insert(ExpectedExpiredLoans.end(), LoanIDs.begin(),
233+
LoanIDs.end());
235234
}
236235
return ExplainMatchResult(UnorderedElementsAreArray(ExpectedExpiredLoans),
237236
ActualExpiredLoans, result_listener);
@@ -730,5 +729,17 @@ TEST_F(LifetimeAnalysisTest, ReassignedPointerThenOriginalExpires) {
730729
EXPECT_THAT(LoansTo({"s1", "s2"}), AreExpiredAt("p_after_s1_expires"));
731730
}
732731

732+
TEST_F(LifetimeAnalysisTest, NoDuplicateLoansForImplicitCastToConst) {
733+
SetupTest(R"(
734+
void target() {
735+
MyObj a;
736+
const MyObj* p = &a;
737+
const MyObj* q = &a;
738+
POINT(at_end);
739+
}
740+
)");
741+
EXPECT_THAT(Helper->getLoansForVar("a"), SizeIs(2));
742+
}
743+
733744
} // anonymous namespace
734745
} // namespace clang::lifetimes::internal

0 commit comments

Comments
 (0)