Skip to content

Commit 2f329e7

Browse files
usx95usx95
committed
[LifetimeSafety] Track view types/gsl::Pointer.
1 parent 673750f commit 2f329e7

File tree

2 files changed

+107
-55
lines changed

2 files changed

+107
-55
lines changed

‎clang/lib/Analysis/LifetimeSafety.cpp‎

Lines changed: 75 additions & 34 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"
@@ -387,25 +388,16 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
387388
using Base = ConstStmtVisitor<FactGenerator>;
388389

389390
public:
390-
FactGenerator(FactManager &FactMgr, AnalysisDeclContext &AC)
391-
: FactMgr(FactMgr), AC(AC) {}
391+
FactGenerator(FactManager &FactMgr) : FactMgr(FactMgr) {}
392392

393-
void run() {
394-
llvm::TimeTraceScope TimeProfile("FactGenerator");
395-
// Iterate through the CFG blocks in reverse post-order to ensure that
396-
// initializations and destructions are processed in the correct sequence.
397-
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
398-
CurrentBlockFacts.clear();
399-
for (unsigned I = 0; I < Block->size(); ++I) {
400-
const CFGElement &Element = Block->Elements[I];
401-
if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
402-
Visit(CS->getStmt());
403-
else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
404-
Element.getAs<CFGAutomaticObjDtor>())
405-
handleDestructor(*DtorOpt);
406-
}
407-
FactMgr.addBlockFacts(Block, CurrentBlockFacts);
408-
}
393+
void startBlock(const CFGBlock *Block) {
394+
CurrentBlock = Block;
395+
CurrentBlockFacts.clear();
396+
}
397+
398+
void endBlock() {
399+
FactMgr.addBlockFacts(CurrentBlock, CurrentBlockFacts);
400+
startBlock(nullptr);
409401
}
410402

411403
void VisitDeclStmt(const DeclStmt *DS) {
@@ -425,7 +417,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
425417
void VisitImplicitCastExpr(const ImplicitCastExpr *ICE) {
426418
if (!hasOrigin(ICE->getType()))
427419
return;
428-
Visit(ICE->getSubExpr());
429420
// An ImplicitCastExpr node itself gets an origin, which flows from the
430421
// origin of its sub-expression (after stripping its own parens/casts).
431422
// TODO: Consider if this is actually useful in practice. Alternatively, we
@@ -493,18 +484,6 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
493484
Base::VisitCXXFunctionalCastExpr(FCE);
494485
}
495486

496-
private:
497-
// Check if a type has an origin.
498-
bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
499-
500-
template <typename Destination, typename Source>
501-
void addAssignOriginFact(const Destination &D, const Source &S) {
502-
OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
503-
OriginID SrcOID = FactMgr.getOriginMgr().get(S);
504-
CurrentBlockFacts.push_back(
505-
FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
506-
}
507-
508487
void handleDestructor(const CFGAutomaticObjDtor &DtorOpt) {
509488
/// TODO: Also handle trivial destructors (e.g., for `int`
510489
/// variables) which will never have a CFGAutomaticObjDtor node.
@@ -527,6 +506,18 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
527506
}
528507
}
529508

509+
private:
510+
// Check if a type has an origin.
511+
bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); }
512+
513+
template <typename Destination, typename Source>
514+
void addAssignOriginFact(const Destination &D, const Source &S) {
515+
OriginID DestOID = FactMgr.getOriginMgr().getOrCreate(D);
516+
OriginID SrcOID = FactMgr.getOriginMgr().get(S);
517+
CurrentBlockFacts.push_back(
518+
FactMgr.createFact<AssignOriginFact>(DestOID, SrcOID));
519+
}
520+
530521
/// Checks if the expression is a `void("__lifetime_test_point_...")` cast.
531522
/// If so, creates a `TestPointFact` and returns true.
532523
bool VisitTestPoint(const CXXFunctionalCastExpr *FCE) {
@@ -549,10 +540,59 @@ class FactGenerator : public ConstStmtVisitor<FactGenerator> {
549540
}
550541

551542
FactManager &FactMgr;
552-
AnalysisDeclContext &AC;
543+
const CFGBlock *CurrentBlock = nullptr;
553544
llvm::SmallVector<Fact *> CurrentBlockFacts;
554545
};
555546

547+
class FactGeneratorDriver : public RecursiveASTVisitor<FactGeneratorDriver> {
548+
public:
549+
FactGeneratorDriver(FactGenerator &FG, AnalysisDeclContext &AC)
550+
: FG(FG), AC(AC) {}
551+
bool shouldTraversePostOrder() const { return true; }
552+
void run() {
553+
llvm::TimeTraceScope TimeProfile("FactGenerator");
554+
// Iterate through the CFG blocks in reverse post-order to ensure that
555+
// initializations and destructions are processed in the correct sequence.
556+
for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) {
557+
FactGeneratorBlockRAII BlockGenerator(FG, Block);
558+
for (const CFGElement &Element : *Block) {
559+
if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>())
560+
TraverseStmt(const_cast<Stmt *>(CS->getStmt()));
561+
else if (std::optional<CFGAutomaticObjDtor> DtorOpt =
562+
Element.getAs<CFGAutomaticObjDtor>())
563+
FG.handleDestructor(*DtorOpt);
564+
}
565+
}
566+
}
567+
568+
bool TraverseStmt(Stmt *S) {
569+
// Avoid re-visiting nodes to not create duplicate facts.
570+
if (!S || !VisitedStmts.insert(S).second)
571+
return true;
572+
return RecursiveASTVisitor::TraverseStmt(S);
573+
}
574+
575+
bool VisitStmt(Stmt *S) {
576+
FG.Visit(S);
577+
return true; // Continue traversing to children.
578+
}
579+
580+
private:
581+
struct FactGeneratorBlockRAII {
582+
FactGeneratorBlockRAII(FactGenerator &FG, const CFGBlock *Block) : FG(FG) {
583+
FG.startBlock(Block);
584+
}
585+
~FactGeneratorBlockRAII() { FG.endBlock(); }
586+
587+
private:
588+
FactGenerator FG;
589+
};
590+
591+
FactGenerator &FG;
592+
AnalysisDeclContext &AC;
593+
llvm::DenseSet<const Stmt *> VisitedStmts;
594+
};
595+
556596
// ========================================================================= //
557597
// Generic Dataflow Analysis
558598
// ========================================================================= //
@@ -1096,8 +1136,9 @@ void LifetimeSafetyAnalysis::run() {
10961136
DEBUG_WITH_TYPE("PrintCFG", Cfg.dump(AC.getASTContext().getLangOpts(),
10971137
/*ShowColors=*/true));
10981138

1099-
FactGenerator FactGen(*FactMgr, AC);
1100-
FactGen.run();
1139+
FactGenerator Generator(*FactMgr);
1140+
FactGeneratorDriver Driver(Generator, AC);
1141+
Driver.run();
11011142
DEBUG_WITH_TYPE("LifetimeFacts", FactMgr->dump(Cfg, AC));
11021143

11031144
/// 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)