[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
alanzhao1 wrote: FYI this patch messes up some diagnostics with `-Wunreachable-code`: https://godbolt.org/z/6TEdrx55d If the unreachable code is a constructor with a default parameter that is a builtin function, clang incorrectly highlights the call to the builtin instead of the call to the constructor. There's a similar issue if instead of a builtin the default parameter is the return value of another function which itself has default parameter that is a builtin: https://godbolt.org/z/ErarnbGbY This was originally observed in Chrome - see https://crbug.com/343231820. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin closed https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
yronglin wrote: Thanks for your review! The CI did passed(https://buildkite.com/llvm-project/github-pull-requests/builds/66630) but not shown on github for whatever reason. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/steakhal approved this pull request. LGTM, thanks! https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin updated
https://github.com/llvm/llvm-project/pull/91879
>From f7b2ae00eebb272e0e5e221608ec3a36146a5d21 Mon Sep 17 00:00:00 2001
From: yronglin
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH 1/5] [Analyzer][CFG] Correctly handle rebuilted default arg
and default init expression
Signed-off-by: yronglin
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode
*Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = fals
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1114,17 +1114,16 @@ void fCXX11MemberInitTest1() {
CXX11MemberInitTest1();
}
+#ifdef PEDANTIC
yronglin wrote:
done
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+}
+
+std::optional ConstantVal = svalBuilder.getConstantVal(ArgE);
+if (!ConstantVal)
+ ConstantVal = UnknownVal();
- std::optional 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(S),
-cast(S));
-Bldr2.generateNode(S, I, State);
+const LocationContext *LCtx = Pred->getLocationContext();
+for (auto *I : CheckedSet) {
+ ProgramStateRef State = I->getState();
+ State = State->BindExpr(S, LCtx, *ConstantVal);
yronglin wrote:
done
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+}
+
+std::optional ConstantVal = svalBuilder.getConstantVal(ArgE);
+if (!ConstantVal)
+ ConstantVal = UnknownVal();
yronglin wrote:
done
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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);
yronglin wrote:
done
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin updated
https://github.com/llvm/llvm-project/pull/91879
>From f7b2ae00eebb272e0e5e221608ec3a36146a5d21 Mon Sep 17 00:00:00 2001
From: yronglin
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH 1/4] [Analyzer][CFG] Correctly handle rebuilted default arg
and default init expression
Signed-off-by: yronglin
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode
*Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = fals
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -5905,7 +5907,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation
Loc, FieldDecl *Field) {
return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
Field, InitializationContext->Context,
- Init);
+ HasRewrittenInit ? Init : nullptr);
yronglin wrote:
@cor3ntin Could you please help confirm this change is make sense?
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin edited https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
yronglin wrote: @steakhal Thanks for your review and sorry for the late reply! https://github.com/llvm/llvm-project/pull/87933 has been revert due to regression, I've a new PR to reapply CWG1815(https://github.com/llvm/llvm-project/pull/92527). I'll continue cook this PR once https://github.com/llvm/llvm-project/pull/92527 merged. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -std=c++14 -verify %s \ +// RUN: %clang_analyze_cc1 -std=c++14 -verify=expected,pedantic %s \ yronglin wrote: It's used like the `pedantic-warning`, I'll remove this to address previous comments. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+}
+
+std::optional ConstantVal = svalBuilder.getConstantVal(ArgE);
+if (!ConstantVal)
+ ConstantVal = UnknownVal();
yronglin wrote:
Agree!
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1114,17 +1114,16 @@ void fCXX11MemberInitTest1() {
CXX11MemberInitTest1();
}
+#ifdef PEDANTIC
yronglin wrote:
Agree!
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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);
steakhal wrote:
```suggestion
for (ExplodedNode *N : CheckedSet) {
ProgramStateRef State = N->getState();
const LocationContext *LCtx = N->getLocationContext();
State = State->BindExpr(S, LCtx, State->getSVal(ArgE, LCtx));
Bldr2.generateNode(S, N, State);
```
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -std=c++14 -verify %s \ +// RUN: %clang_analyze_cc1 -std=c++14 -verify=expected,pedantic %s \ steakhal wrote: You introduced the `pedantic` verify prefix, but never actually used it. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
steakhal wrote:
I think I'd prefer the old name here. That is how one would find these in the
other places.
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1114,17 +1114,16 @@ void fCXX11MemberInitTest1() {
CXX11MemberInitTest1();
}
+#ifdef PEDANTIC
steakhal wrote:
I don't think you need to guard this section of code if you were using `//
pedantic-note {{...}}` and `// pedantic-warning {{...}}` in the guarded checks.
I also think that the `// TODO: we'd expect the warning: {{2 uninitializeds
field}}` comment refers to this new appearing report, so you should just drop
that comment from the `fCXX11MemberInitTest2` function.
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/steakhal commented: In general, this PR looks good to me. I only have some nits inline. If it didn't break anything, it should be good to go. https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+}
+
+std::optional ConstantVal = svalBuilder.getConstantVal(ArgE);
+if (!ConstantVal)
+ ConstantVal = UnknownVal();
steakhal wrote:
```suggestion
```
I'd just drop this and use `value_or()` at the only use-site instead.
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -1964,39 +1964,55 @@ 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 CheckedSet;
+ getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, S, *this);
ExplodedNodeSet Tmp;
- StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
+ StmtNodeBuilder Bldr2(CheckedSet, Tmp, *currBldrCtx);
- const Expr *ArgE;
- if (const auto *DefE = dyn_cast(S))
+ bool HasRewrittenInit = false;
+ const Expr *ArgE = nullptr;
+ if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else if (const auto *DefE = dyn_cast(S))
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else if (const auto *DefE = dyn_cast(S)) {
ArgE = DefE->getExpr();
- else
+HasRewrittenInit = DefE->hasRewrittenInit();
+ } else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
- if (const auto *MTE = dyn_cast(ArgE)) {
-ArgE = MTE->getSubExpr();
-IsTemporary = true;
- }
+ 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(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+}
+
+std::optional ConstantVal = svalBuilder.getConstantVal(ArgE);
+if (!ConstantVal)
+ ConstantVal = UnknownVal();
- std::optional 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(S),
-cast(S));
-Bldr2.generateNode(S, I, State);
+const LocationContext *LCtx = Pred->getLocationContext();
+for (auto *I : CheckedSet) {
+ ProgramStateRef State = I->getState();
+ State = State->BindExpr(S, LCtx, *ConstantVal);
steakhal wrote:
```suggestion
State = State->BindExpr(S, LCtx, ConstantVal.value_or(UnknownVal()));
```
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin updated
https://github.com/llvm/llvm-project/pull/91879
>From 6969f06f39363deda92d473ec14a08663c71f3b1 Mon Sep 17 00:00:00 2001
From: yronglin
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH 1/3] [Analyzer][CFG] Correctly handle rebuilted default arg
and default init expression
Signed-off-by: yronglin
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode
*Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = fals
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -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);
yronglin wrote:
I've explicit specific `HasRewrittenInit` in `BuildCXXDefaultInitExpr` when
construct `CXXDefaultInitExpr`. This solution is temporarily feasible, but it
may not be a good way.
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
Author: None (yronglin)
Changes
Depends on https://github.com/llvm/llvm-project/pull/87933
Clang now support the following:
- Extending lifetime of object bound to reference members of aggregates, that
are created from default member initializer.
- Rebuild `CXXDefaultArgExpr` and `CXXDefaultInitExpr` as needed where called
or constructed.
But CFG and ExprEngine need to be updated to address this change.
This PR add `CXXDefaultArgExpr` and `CXXDefaultInitExpr` into CFG, and correct
handle these expressions in ExprEngine
---
Full diff: https://github.com/llvm/llvm-project/pull/91879.diff
6 Files Affected:
- (modified) clang/lib/AST/ParentMap.cpp (+16)
- (modified) clang/lib/Analysis/CFG.cpp (+41-9)
- (modified) clang/lib/Sema/SemaExpr.cpp (+3-1)
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+40-24)
- (modified) clang/test/Analysis/cxx-uninitialized-object.cpp (+7-6)
- (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4)
``diff
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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..02317257c2740 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);
@@ -2254,16 +2258,10 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
asc, ExternallyDestructed);
case Stmt::CXXDefaultArgExprClass:
+ return VisitCXXDefaultArgExpr(cast(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(S), asc);
@@ -2433,6 +2431,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, AddStmtCho
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (yronglin)
Changes
Depends on https://github.com/llvm/llvm-project/pull/87933
Clang now support the following:
- Extending lifetime of object bound to reference members of aggregates, that
are created from default member initializer.
- Rebuild `CXXDefaultArgExpr` and `CXXDefaultInitExpr` as needed where called
or constructed.
But CFG and ExprEngine need to be updated to address this change.
This PR add `CXXDefaultArgExpr` and `CXXDefaultInitExpr` into CFG, and correct
handle these expressions in ExprEngine
---
Full diff: https://github.com/llvm/llvm-project/pull/91879.diff
6 Files Affected:
- (modified) clang/lib/AST/ParentMap.cpp (+16)
- (modified) clang/lib/Analysis/CFG.cpp (+41-9)
- (modified) clang/lib/Sema/SemaExpr.cpp (+3-1)
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+40-24)
- (modified) clang/test/Analysis/cxx-uninitialized-object.cpp (+7-6)
- (modified) clang/test/Analysis/lifetime-extended-regions.cpp (+3-4)
``diff
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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..02317257c2740 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);
@@ -2254,16 +2258,10 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
asc, ExternallyDestructed);
case Stmt::CXXDefaultArgExprClass:
+ return VisitCXXDefaultArgExpr(cast(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(S), asc);
@@ -2433,6 +2431,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 (
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin ready_for_review https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin updated
https://github.com/llvm/llvm-project/pull/91879
>From 6969f06f39363deda92d473ec14a08663c71f3b1 Mon Sep 17 00:00:00 2001
From: yronglin
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
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode
*Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = fals
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin edited https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin updated
https://github.com/llvm/llvm-project/pull/91879
>From 6969f06f39363deda92d473ec14a08663c71f3b1 Mon Sep 17 00:00:00 2001
From: yronglin
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH] [Analyzer][CFG] Correctly handle rebuilted default arg and
default init expression
Signed-off-by: yronglin
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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(S))
@@ -1978,25 +1975,15 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode
*Pred,
else
llvm_unreachable("unknown constant wrapper kind");
- bool IsTemporary = false;
-
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -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);
yronglin wrote:
Thanks a lot for your review!
> I think it'd be useful to keep some of the old comment here: we can't add the
> default argument if it's not rewritten because we could end up with the same
> expression appearing multiple times.
Agree 100%, keep the old comment here can make things more clear.
> Actually, are we safe from that even if the default argument is rewritten? Do
> we guarantee to recreate all subexpressions in that case?
Not exactly, The rebuild implementation is the following, it's ignores
`LambdaExpr`, `BlockExpr` and `CXXThisExpr`, And
`EnsureImmediateInvocationInDefaultArgs`’s name may need to be refine.
https://github.com/llvm/llvm-project/blob/78b3a00418ce6da0426a261a64a77608d0264fe5/clang/lib/Sema/SemaExpr.cpp#L5707-L5723
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -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);
zygoloid wrote:
Actually, are we safe from that even if the default argument is rewritten? Do
we guarantee to recreate all subexpressions in that case?
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
@@ -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);
zygoloid wrote:
I think it'd be useful to keep some of the old comment here: we can't add the
default argument if it's not rewritten because we could end up with the same
expression appearing multiple times.
https://github.com/llvm/llvm-project/pull/91879
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
github-actions[bot] wrote: :warning: C/C++ code formatter, clang-format found issues in your code. :warning: You can test this locally with the following command: ``bash git-clang-format --diff 23fe1fc6b78b7643a801ce3eb53d14b47b1dd0ff 6969f06f39363deda92d473ec14a08663c71f3b1 -- clang/lib/AST/ParentMap.cpp clang/lib/Analysis/CFG.cpp clang/lib/StaticAnalyzer/Core/ExprEngine.cpp `` View the diff from clang-format here. ``diff diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 2f3f3e9251..12a1634a31 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -556,8 +556,10 @@ public: 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); `` https://github.com/llvm/llvm-project/pull/91879 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (PR #91879)
https://github.com/yronglin created
https://github.com/llvm/llvm-project/pull/91879
Depends on https://github.com/llvm/llvm-project/pull/87933
Clang now support the following:
- Extending lifetime of object bound to reference members of aggregates, that
are created from default member initializer.
- Rebuild `CXXDefaultArgExpr` and `CXXDefaultInitExpr` as needed where called
or constructed.
But CFG and ExprEngine need to be updated to address this change.
This PR add `CXXDefaultArgExpr` and `CXXDefaultInitExpr` into CFG, and correct
handle these expressions in ExprEngine
>From 6969f06f39363deda92d473ec14a08663c71f3b1 Mon Sep 17 00:00:00 2001
From: yronglin
Date: Sun, 12 May 2024 14:42:09 +0800
Subject: [PATCH] [Analyzer][CFG] Correctly handle rebuilted default arg and
default init expression
Signed-off-by: yronglin
---
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(S)) {
+ if (Arg->hasRewrittenInit()) {
+M[Arg->getExpr()] = S;
+BuildParentMap(M, Arg->getExpr(), OVMode);
+ }
+}
+break;
+ case Stmt::CXXDefaultInitExprClass:
+if (auto *Init = dyn_cast(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(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(S), asc);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast(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().runCheckers
