[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
via cfe-commits
cfe-commits at lists.llvm.org
Mon May 13 08:17:23 PDT 2024
https://github.com/yronglin updated https://github.com/llvm/llvm-project/pull/91879
>From 6969f06f39363deda92d473ec14a08663c71f3b1 Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH 1/2] [Analyzer][CFG] Correctly handle rebuilted default arg
and default init expression
Signed-off-by: yronglin <yronglin777 at gmail.com>
---
clang/lib/AST/ParentMap.cpp | 16 +++++++++
clang/lib/Analysis/CFG.cpp | 38 +++++++++++++++-----
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 35 ++++++------------
3 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index 3d6a1cc84c7b1..534793b837bbb 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -97,6 +97,22 @@ static void BuildParentMap(MapTy& M, Stmt* S,
BuildParentMap(M, SubStmt, OVMode);
}
break;
+ case Stmt::CXXDefaultArgExprClass:
+ if (auto *Arg = dyn_cast<CXXDefaultArgExpr>(S)) {
+ if (Arg->hasRewrittenInit()) {
+ M[Arg->getExpr()] = S;
+ BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+ }
+ break;
+ case Stmt::CXXDefaultInitExprClass:
+ if (auto *Init = dyn_cast<CXXDefaultInitExpr>(S)) {
+ if (Init->hasRewrittenInit()) {
+ M[Init->getExpr()] = S;
+ BuildParentMap(M, Init->getExpr(), OVMode);
+ }
+ }
+ break;
default:
for (Stmt *SubStmt : S->children()) {
if (SubStmt) {
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 64e6155de090c..2f3f3e92516ba 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,6 +556,8 @@ class CFGBuilder {
private:
// Visitors to walk an AST and construct the CFG.
+ CFGBlock *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Default, AddStmtChoice asc);
+ CFGBlock *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Default, AddStmtChoice asc);
CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc);
@@ -2254,16 +2256,10 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
asc, ExternallyDestructed);
case Stmt::CXXDefaultArgExprClass:
+ return VisitCXXDefaultArgExpr(cast<CXXDefaultArgExpr>(S), asc);
+
case Stmt::CXXDefaultInitExprClass:
- // FIXME: The expression inside a CXXDefaultArgExpr is owned by the
- // called function's declaration, not by the caller. If we simply add
- // this expression to the CFG, we could end up with the same Expr
- // appearing multiple times (PR13385).
- //
- // It's likewise possible for multiple CXXDefaultInitExprs for the same
- // expression to be used in the same function (through aggregate
- // initialization).
- return VisitStmt(S, asc);
+ return VisitCXXDefaultInitExpr(cast<CXXDefaultInitExpr>(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), asc);
@@ -2433,6 +2429,30 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
return B;
}
+CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg,
+ AddStmtChoice asc) {
+ if (Arg->hasRewrittenInit()) {
+ if (asc.alwaysAdd(*this, Arg)) {
+ autoCreateBlock();
+ appendStmt(Block, Arg);
+ }
+ return VisitStmt(Arg->getExpr(), asc);
+ }
+ return VisitStmt(Arg, asc);
+}
+
+CFGBlock *CFGBuilder::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Init,
+ AddStmtChoice asc) {
+ if (Init->hasRewrittenInit()) {
+ if (asc.alwaysAdd(*this, Init)) {
+ autoCreateBlock();
+ appendStmt(Block, Init);
+ }
+ return VisitStmt(Init->getExpr(), asc);
+ }
+ return VisitStmt(Init, asc);
+}
+
CFGBlock *CFGBuilder::VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc) {
if (asc.alwaysAdd(*this, ILE)) {
autoCreateBlock();
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 0b1edf3e5c96b..41cb4293a8451 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1964,11 +1964,8 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
case Stmt::CXXDefaultArgExprClass:
case Stmt::CXXDefaultInitExprClass: {
Bldr.takeNodes(Pred);
- ExplodedNodeSet PreVisit;
- getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
-
- ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ ExplodedNodeSet CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
const Expr *ArgE;
if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
- ArgE = MTE->getSubExpr();
- IsTemporary = true;
- }
-
- std::optional<SVal> ConstantVal = svalBuilder.getConstantVal(ArgE);
- if (!ConstantVal)
- ConstantVal = UnknownVal();
-
- const LocationContext *LCtx = Pred->getLocationContext();
- for (const auto I : PreVisit) {
- ProgramStateRef State = I->getState();
- State = State->BindExpr(S, LCtx, *ConstantVal);
- if (IsTemporary)
- State = createTemporaryRegionIfNeeded(State, LCtx,
- cast<Expr>(S),
- cast<Expr>(S));
- Bldr2.generateNode(S, I, State);
+ ExplodedNodeSet Tmp;
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
+
+ for (auto *I : CheckedSet) {
+ ProgramStateRef state = (*I).getState();
+ const LocationContext *LCtx = (*I).getLocationContext();
+ SVal Val = state->getSVal(ArgE, LCtx);
+ state = state->BindExpr(S, LCtx, Val);
+ Bldr2.generateNode(S, I, state);
}
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
>From d3403a3e1a0595b6fbb204f6990f23291ae22dbf Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Mon, 13 May 2024 23:15:22 +0800
Subject: [PATCH 2/2] Address review comments
Signed-off-by: yronglin <yronglin777 at gmail.com>
---
clang/lib/Analysis/CFG.cpp | 16 +++++-
clang/lib/Sema/SemaExpr.cpp | 4 +-
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 53 ++++++++++++++-----
.../Analysis/cxx-uninitialized-object.cpp | 13 ++---
.../Analysis/lifetime-extended-regions.cpp | 7 ++-
5 files changed, 68 insertions(+), 25 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 2f3f3e92516ba..02317257c2740 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,8 +556,10 @@ class CFGBuilder {
private:
// Visitors to walk an AST and construct the CFG.
- CFGBlock *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Default, AddStmtChoice asc);
- CFGBlock *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Default, AddStmtChoice asc);
+ CFGBlock *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Default,
+ AddStmtChoice asc);
+ CFGBlock *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Default,
+ AddStmtChoice asc);
CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc);
@@ -2438,6 +2440,11 @@ CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg,
}
return VisitStmt(Arg->getExpr(), asc);
}
+
+ // We can't add the default argument if it's not rewritten because the
+ // expression inside a CXXDefaultArgExpr is owned by the called function's
+ // declaration, not by the caller, we could end up with the same expression
+ // appearing multiple times.
return VisitStmt(Arg, asc);
}
@@ -2450,6 +2457,11 @@ CFGBlock *CFGBuilder::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Init,
}
return VisitStmt(Init->getExpr(), asc);
}
+
+ // We can't add the default initializer if it's not rewritten because multiple
+ // CXXDefaultInitExprs for the same sub-expression to be used in the same
+ // function (through aggregate initialization). we could end up with the same
+ // expression appearing multiple times.
return VisitStmt(Init, asc);
}
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index bb4b116fd73ca..eed328c716382 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5813,6 +5813,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
InitializationContext.emplace(Loc, Field, CurContext);
Expr *Init = nullptr;
+ bool HasRewrittenInit = false;
bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
@@ -5862,6 +5863,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
isa_and_present<ExprWithCleanups>(Field->getInClassInitializer());
if (V.HasImmediateCalls || InLifetimeExtendingContext ||
ContainsAnyTemporaries) {
+ HasRewrittenInit = true;
ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
CurContext};
ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
@@ -5905,7 +5907,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
Field, InitializationContext->Context,
- Init);
+ HasRewrittenInit ? Init : nullptr);
}
// DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 41cb4293a8451..525239101471e 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1967,23 +1967,52 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet CheckedSet;
getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
+ ExplodedNodeSet Tmp;
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
+
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
+ HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
ArgE = DefE->getExpr();
- else
+ HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
+ if (HasRewrittenInit) {
+ for (auto *I : CheckedSet) {
+ ProgramStateRef state = (*I).getState();
+ const LocationContext *LCtx = (*I).getLocationContext();
+ SVal Val = state->getSVal(ArgE, LCtx);
+ state = state->BindExpr(S, LCtx, Val);
+ Bldr2.generateNode(S, I, state);
+ }
+ } else {
+ // If it's not rewritten, the contents of these expressions are not
+ // actually part of the current function, so we fall back to constant
+ // evaluation.
+ bool IsTemporary = false;
+ if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+ }
- for (auto *I : CheckedSet) {
- ProgramStateRef state = (*I).getState();
- const LocationContext *LCtx = (*I).getLocationContext();
- SVal Val = state->getSVal(ArgE, LCtx);
- state = state->BindExpr(S, LCtx, Val);
- Bldr2.generateNode(S, I, state);
+ std::optional<SVal> ConstantVal = svalBuilder.getConstantVal(ArgE);
+ if (!ConstantVal)
+ ConstantVal = UnknownVal();
+
+ const LocationContext *LCtx = Pred->getLocationContext();
+ for (auto *I : CheckedSet) {
+ ProgramStateRef State = I->getState();
+ State = State->BindExpr(S, LCtx, *ConstantVal);
+ if (IsTemporary)
+ State = createTemporaryRegionIfNeeded(State, LCtx, cast<Expr>(S),
+ cast<Expr>(S));
+
+ Bldr2.generateNode(S, I, State);
+ }
}
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
diff --git a/clang/test/Analysis/cxx-uninitialized-object.cpp b/clang/test/Analysis/cxx-uninitialized-object.cpp
index e3fa8ae8d7f29..c62fd1c312056 100644
--- a/clang/test/Analysis/cxx-uninitialized-object.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -std=c++14 -verify %s \
+// RUN: %clang_analyze_cc1 -std=c++14 -verify=expected,pedantic %s \
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=optin.cplusplus.UninitializedObject \
// RUN: -analyzer-config optin.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
@@ -1114,17 +1114,16 @@ void fCXX11MemberInitTest1() {
CXX11MemberInitTest1();
}
+#ifdef PEDANTIC
struct CXX11MemberInitTest2 {
struct RecordType {
- // TODO: we'd expect the note: {{uninitialized field 'this->rec.a'}}
- int a; // no-note
- // TODO: we'd expect the note: {{uninitialized field 'this->rec.b'}}
- int b; // no-note
+ int a; // expected-note {{uninitialized field 'this->a'}}
+ int b; // expected-note {{uninitialized field 'this->b'}}
RecordType(int) {}
};
- RecordType rec = RecordType(int());
+ RecordType rec = RecordType(int()); // expected-warning {{2 uninitialized fields at the end of the constructor call}}
int dontGetFilteredByNonPedanticMode = 0;
CXX11MemberInitTest2() {}
@@ -1135,6 +1134,8 @@ void fCXX11MemberInitTest2() {
CXX11MemberInitTest2(); // no-warning
}
+#endif // PEDANTIC
+
//===----------------------------------------------------------------------===//
// "Esoteric" primitive type tests.
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/lifetime-extended-regions.cpp b/clang/test/Analysis/lifetime-extended-regions.cpp
index 4458ad294af7c..524f4e0c400d1 100644
--- a/clang/test/Analysis/lifetime-extended-regions.cpp
+++ b/clang/test/Analysis/lifetime-extended-regions.cpp
@@ -121,11 +121,10 @@ void aggregateWithReferences() {
clang_analyzer_dump(viaReference.rx); // expected-warning-re {{&lifetime_extended_object{int, viaReference, S{{[0-9]+}}} }}
clang_analyzer_dump(viaReference.ry); // expected-warning-re {{&lifetime_extended_object{Composite, viaReference, S{{[0-9]+}}} }}
- // FIXME: clang currently support extending lifetime of object bound to reference members of aggregates,
- // that are created from default member initializer. But CFG and ExprEngine need to be updated to address this change.
- // The following expect warning: {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
+ // The lifetime lifetime of object bound to reference members of aggregates,
+ // that are created from default member initializer was extended.
RefAggregate defaultInitExtended{i};
- clang_analyzer_dump(defaultInitExtended.ry); // expected-warning {{Unknown }}
+ clang_analyzer_dump(defaultInitExtended.ry); // expected-warning-re {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
}
void lambda() {
More information about the cfe-commits
mailing list