[clang] 44aa618 - [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#117437)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Feb 1 00:58:08 PST 2025
Author: yronglin
Date: 2025-02-01T16:58:05+08:00
New Revision: 44aa618ef67d302f5ab77cc591fb3434fe967a2e
URL: https://github.com/llvm/llvm-project/commit/44aa618ef67d302f5ab77cc591fb3434fe967a2e
DIFF: https://github.com/llvm/llvm-project/commit/44aa618ef67d302f5ab77cc591fb3434fe967a2e.diff
LOG: [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#117437)
Clang currently support extending lifetime of object bound to reference
members of aggregates, that are created from default member initializer.
This PR address this change and updaye CFG and ExprEngine.
This PR reapply https://github.com/llvm/llvm-project/pull/91879.
Fixes https://github.com/llvm/llvm-project/issues/93725.
---------
Signed-off-by: yronglin <yronglin777 at gmail.com>
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/AST/ParentMap.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/Analysis/ReachableCode.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
clang/test/AST/ast-dump-recovery.cpp
clang/test/Analysis/lifetime-extended-regions.cpp
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
clang/test/SemaCXX/warn-unreachable.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a220e57d0b3222..30364a747f9c19 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -219,6 +219,10 @@ Code Completion
Static Analyzer
---------------
+- Clang currently support extending lifetime of object bound to
+ reference members of aggregates in CFG and ExprEngine, that are
+ created from default member initializer.
+
New features
^^^^^^^^^^^^
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index e62e71bf5a5145..580613b2618fb6 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -13,6 +13,7 @@
#include "clang/AST/ParentMap.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/AST/StmtObjC.h"
#include "llvm/ADT/DenseMap.h"
@@ -103,6 +104,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 304bbb2b422c61..6bba0e38af630d 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,6 +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 *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc);
@@ -2261,16 +2265,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);
@@ -2440,6 +2438,40 @@ 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);
+ }
+
+ // 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);
+}
+
+CFGBlock *CFGBuilder::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Init,
+ AddStmtChoice asc) {
+ if (Init->hasRewrittenInit()) {
+ if (asc.alwaysAdd(*this, Init)) {
+ autoCreateBlock();
+ appendStmt(Block, 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);
+}
+
CFGBlock *CFGBuilder::VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc) {
if (asc.alwaysAdd(*this, ILE)) {
autoCreateBlock();
diff --git a/clang/lib/Analysis/ReachableCode.cpp b/clang/lib/Analysis/ReachableCode.cpp
index dd81c8e0a3d543..3b1f716f8dea18 100644
--- a/clang/lib/Analysis/ReachableCode.cpp
+++ b/clang/lib/Analysis/ReachableCode.cpp
@@ -454,11 +454,12 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) {
return isDeadRoot;
}
-// Check if the given `DeadStmt` is a coroutine statement and is a substmt of
-// the coroutine statement. `Block` is the CFGBlock containing the `DeadStmt`.
-static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
+// Check if the given `DeadStmt` is one of target statements or is a sub-stmt of
+// them. `Block` is the CFGBlock containing the `DeadStmt`.
+template <class... Ts>
+static bool isDeadStmtInOneOf(const Stmt *DeadStmt, const CFGBlock *Block) {
// The coroutine statement, co_return, co_await, or co_yield.
- const Stmt *CoroStmt = nullptr;
+ const Stmt *TargetStmt = nullptr;
// Find the first coroutine statement after the DeadStmt in the block.
bool AfterDeadStmt = false;
for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
@@ -467,32 +468,27 @@ static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
const Stmt *S = CS->getStmt();
if (S == DeadStmt)
AfterDeadStmt = true;
- if (AfterDeadStmt &&
- // For simplicity, we only check simple coroutine statements.
- (llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) {
- CoroStmt = S;
+ if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
+ TargetStmt = S;
break;
}
}
- if (!CoroStmt)
+ if (!TargetStmt)
return false;
struct Checker : DynamicRecursiveASTVisitor {
const Stmt *DeadStmt;
- bool CoroutineSubStmt = false;
- Checker(const Stmt *S) : DeadStmt(S) {
- // Statements captured in the CFG can be implicit.
- ShouldVisitImplicitCode = true;
- }
+ bool IsSubStmtOfTargetStmt = false;
+ Checker(const Stmt *S) : DeadStmt(S) { ShouldVisitImplicitCode = true; }
bool VisitStmt(Stmt *S) override {
if (S == DeadStmt)
- CoroutineSubStmt = true;
+ IsSubStmtOfTargetStmt = true;
return true;
}
};
Checker checker(DeadStmt);
- checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
- return checker.CoroutineSubStmt;
+ checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
+ return checker.IsSubStmtOfTargetStmt;
}
static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -503,7 +499,12 @@ static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
// Coroutine statements are never considered dead statements, because removing
// them may change the function semantic if it is the only coroutine statement
// of the coroutine.
- return !isInCoroutineStmt(S, Block);
+ //
+ // If the dead stmt is a sub-stmt of CXXDefaultInitExpr and CXXDefaultArgExpr,
+ // we would rather expect to find CXXDefaultInitExpr and CXXDefaultArgExpr as
+ // a valid dead stmt.
+ return !isDeadStmtInOneOf<CoreturnStmt, CoroutineSuspendExpr,
+ CXXDefaultArgExpr, CXXDefaultInitExpr>(S, Block);
}
const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ba4aaa94b90ffd..95c985e049060c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5570,8 +5570,10 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
/*SkipImmediateInvocations=*/NestedDefaultChecking))
return ExprError();
+ Expr *RewrittenExpr = (Init == Param->getDefaultArg() ? nullptr : Init);
return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
- Init, InitializationContext->Context);
+ RewrittenExpr,
+ InitializationContext->Context);
}
static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx,
@@ -5689,10 +5691,11 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
return ExprError();
}
Init = Res.get();
-
+ Expr *RewrittenInit =
+ (Init == Field->getInClassInitializer() ? nullptr : Init);
return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
Field, InitializationContext->Context,
- Init);
+ RewrittenInit);
}
// DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 2b1872f8386aad..5851f7d5eb024c 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1987,33 +1987,45 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet Tmp;
StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
+ bool HasRebuiltInit = 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))
+ HasRebuiltInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
ArgE = DefE->getExpr();
- else
+ HasRebuiltInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
- ArgE = MTE->getSubExpr();
- IsTemporary = true;
- }
+ if (HasRebuiltInit) {
+ for (auto *N : PreVisit) {
+ ProgramStateRef state = N->getState();
+ const LocationContext *LCtx = N->getLocationContext();
+ state = state->BindExpr(S, LCtx, state->getSVal(ArgE, LCtx));
+ Bldr2.generateNode(S, N, 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;
+ }
+
+ std::optional<SVal> ConstantVal = svalBuilder.getConstantVal(ArgE);
+ const LocationContext *LCtx = Pred->getLocationContext();
+ for (auto *I : PreVisit) {
+ ProgramStateRef State = I->getState();
+ State = State->BindExpr(S, LCtx, ConstantVal.value_or(UnknownVal()));
+ if (IsTemporary)
+ State = createTemporaryRegionIfNeeded(State, LCtx, cast<Expr>(S),
+ cast<Expr>(S));
- 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);
+ Bldr2.generateNode(S, I, State);
+ }
}
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
index b59fa3778192f9..fa6d747556dd8c 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -507,7 +507,7 @@ union U {
// CHECK-NEXT: `-DeclStmt {{.*}}
// CHECK-NEXT: `-VarDecl {{.*}} g 'U':'GH112560::U' listinit
// CHECK-NEXT: `-InitListExpr {{.*}} 'U':'GH112560::U' contains-errors field Field {{.*}} 'f' 'int'
-// CHECK-NEXT: `-CXXDefaultInitExpr {{.*}} 'int' contains-errors has rewritten init
+// CHECK-NEXT: `-CXXDefaultInitExpr {{.*}} 'int' contains-errors
// CHECK-NEXT: `-RecoveryExpr {{.*}} 'int' contains-errors
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
void foo() {
diff --git a/clang/test/Analysis/lifetime-extended-regions.cpp b/clang/test/Analysis/lifetime-extended-regions.cpp
index 4458ad294af7cb..02a1210d9af925 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 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() {
diff --git a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
index 8e428c0ef04279..37824c16f4f054 100644
--- a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
+++ b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
@@ -274,16 +274,16 @@ void f() {
// CHECK: ClassTemplateSpecializationDecl {{.*}} struct A definition
// CHECK: CXXConstructorDecl {{.*}} implicit used constexpr A 'void () noexcept'
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 1
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 2
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} 'a' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 3
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 4
// CHECK-NEXT: CompoundStmt {{.*}}
diff --git a/clang/test/SemaCXX/warn-unreachable.cpp b/clang/test/SemaCXX/warn-unreachable.cpp
index e6f5bc5ef8e127..8c23822dc16e0a 100644
--- a/clang/test/SemaCXX/warn-unreachable.cpp
+++ b/clang/test/SemaCXX/warn-unreachable.cpp
@@ -414,3 +414,42 @@ void tautological_compare(bool x, int y) {
calledFun();
}
+
+namespace test_rebuilt_default_arg {
+struct A {
+ explicit A(int = __builtin_LINE());
+};
+
+int h(int a) {
+ return 3;
+ A(); // expected-warning {{will never be executed}}
+}
+
+struct Temp {
+ Temp();
+ ~Temp();
+};
+
+struct B {
+ explicit B(const Temp &t = Temp());
+};
+int f(int a) {
+ return 3;
+ B(); // expected-warning {{will never be executed}}
+}
+} // namespace test_rebuilt_default_arg
+namespace test_rebuilt_default_init {
+
+struct A {
+ A();
+ ~A();
+};
+
+struct B {
+ const A &t = A();
+};
+int f(int a) {
+ return 3;
+ A{}; // expected-warning {{will never be executed}}
+}
+} // namespace test_rebuilt_default_init
More information about the cfe-commits
mailing list