[clang] 90e0dd1 - Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#117437)"
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 3 06:59:03 PST 2025
Author: Hans Wennborg
Date: 2025-02-03T15:52:04+01:00
New Revision: 90e0dd15ff070b5b4b1bb068cdade7f5b5e6ccec
URL: https://github.com/llvm/llvm-project/commit/90e0dd15ff070b5b4b1bb068cdade7f5b5e6ccec
DIFF: https://github.com/llvm/llvm-project/commit/90e0dd15ff070b5b4b1bb068cdade7f5b5e6ccec.diff
LOG: Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#117437)"
This caused assertion failures:
clang/lib/Analysis/CFG.cpp:822:
void (anonymous namespace)::CFGBuilder::appendStmt(CFGBlock *, const Stmt *):
Assertion `!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S' failed.
See comment on the PR.
This reverts commit 44aa618ef67d302f5ab77cc591fb3434fe967a2e.
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 30364a747f9c194..a220e57d0b32229 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -219,10 +219,6 @@ 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 580613b2618fb64..e62e71bf5a51459 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -13,7 +13,6 @@
#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"
@@ -104,22 +103,6 @@ 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 6bba0e38af630de..304bbb2b422c61d 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -556,10 +556,6 @@ 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);
@@ -2265,10 +2261,16 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
asc, ExternallyDestructed);
case Stmt::CXXDefaultArgExprClass:
- return VisitCXXDefaultArgExpr(cast<CXXDefaultArgExpr>(S), asc);
-
case Stmt::CXXDefaultInitExprClass:
- return VisitCXXDefaultInitExpr(cast<CXXDefaultInitExpr>(S), asc);
+ // 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);
case Stmt::CXXBindTemporaryExprClass:
return VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), asc);
@@ -2438,40 +2440,6 @@ 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 3b1f716f8dea187..dd81c8e0a3d5437 100644
--- a/clang/lib/Analysis/ReachableCode.cpp
+++ b/clang/lib/Analysis/ReachableCode.cpp
@@ -454,12 +454,11 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) {
return isDeadRoot;
}
-// 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) {
+// 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) {
// The coroutine statement, co_return, co_await, or co_yield.
- const Stmt *TargetStmt = nullptr;
+ const Stmt *CoroStmt = 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;
@@ -468,27 +467,32 @@ static bool isDeadStmtInOneOf(const Stmt *DeadStmt, const CFGBlock *Block) {
const Stmt *S = CS->getStmt();
if (S == DeadStmt)
AfterDeadStmt = true;
- if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
- TargetStmt = S;
+ if (AfterDeadStmt &&
+ // For simplicity, we only check simple coroutine statements.
+ (llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) {
+ CoroStmt = S;
break;
}
}
- if (!TargetStmt)
+ if (!CoroStmt)
return false;
struct Checker : DynamicRecursiveASTVisitor {
const Stmt *DeadStmt;
- bool IsSubStmtOfTargetStmt = false;
- Checker(const Stmt *S) : DeadStmt(S) { ShouldVisitImplicitCode = true; }
+ bool CoroutineSubStmt = false;
+ Checker(const Stmt *S) : DeadStmt(S) {
+ // Statements captured in the CFG can be implicit.
+ ShouldVisitImplicitCode = true;
+ }
bool VisitStmt(Stmt *S) override {
if (S == DeadStmt)
- IsSubStmtOfTargetStmt = true;
+ CoroutineSubStmt = true;
return true;
}
};
Checker checker(DeadStmt);
- checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
- return checker.IsSubStmtOfTargetStmt;
+ checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
+ return checker.CoroutineSubStmt;
}
static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -499,12 +503,7 @@ 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.
- //
- // 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);
+ return !isInCoroutineStmt(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 95c985e049060c7..ba4aaa94b90ffd5 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5570,10 +5570,8 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
/*SkipImmediateInvocations=*/NestedDefaultChecking))
return ExprError();
- Expr *RewrittenExpr = (Init == Param->getDefaultArg() ? nullptr : Init);
return CXXDefaultArgExpr::Create(Context, InitializationContext->Loc, Param,
- RewrittenExpr,
- InitializationContext->Context);
+ Init, InitializationContext->Context);
}
static FieldDecl *FindFieldDeclInstantiationPattern(const ASTContext &Ctx,
@@ -5691,11 +5689,10 @@ 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,
- RewrittenInit);
+ Init);
}
// DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 5851f7d5eb024c4..2b1872f8386aad1 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1987,45 +1987,33 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet Tmp;
StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
- bool HasRebuiltInit = false;
- const Expr *ArgE = nullptr;
- if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S)) {
+ const Expr *ArgE;
+ if (const auto *DefE = dyn_cast<CXXDefaultArgExpr>(S))
ArgE = DefE->getExpr();
- HasRebuiltInit = DefE->hasRewrittenInit();
- } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
+ else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
ArgE = DefE->getExpr();
- HasRebuiltInit = DefE->hasRewrittenInit();
- } else
+ else
llvm_unreachable("unknown constant wrapper kind");
- 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));
+ bool IsTemporary = false;
+ if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(ArgE)) {
+ ArgE = MTE->getSubExpr();
+ IsTemporary = true;
+ }
- Bldr2.generateNode(S, I, State);
- }
+ 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);
}
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 fa6d747556dd8c2..b59fa3778192f99 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
+// CHECK-NEXT: `-CXXDefaultInitExpr {{.*}} 'int' contains-errors has rewritten init
// 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 02a1210d9af925b..4458ad294af7cb0 100644
--- a/clang/test/Analysis/lifetime-extended-regions.cpp
+++ b/clang/test/Analysis/lifetime-extended-regions.cpp
@@ -121,10 +121,11 @@ 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]+}}} }}
- // The lifetime of object bound to reference members of aggregates,
- // that are created from default member initializer was extended.
+ // 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]+}}} }}
RefAggregate defaultInitExtended{i};
- clang_analyzer_dump(defaultInitExtended.ry); // expected-warning-re {{&lifetime_extended_object{Composite, defaultInitExtended, S{{[0-9]+}}} }}
+ clang_analyzer_dump(defaultInitExtended.ry); // expected-warning {{Unknown }}
}
void lambda() {
diff --git a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
index 37824c16f4f054c..8e428c0ef042791 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'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 1
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 2
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} 'a' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
// CHECK-NEXT: IntegerLiteral {{.*}} 'int' 3
// CHECK-NEXT: CXXCtorInitializer Field {{.*}} '_' 'int'
-// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int'
+// CHECK-NEXT: CXXDefaultInitExpr {{.*}} 'int' has rewritten init
// 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 8c23822dc16e0a5..e6f5bc5ef8e1275 100644
--- a/clang/test/SemaCXX/warn-unreachable.cpp
+++ b/clang/test/SemaCXX/warn-unreachable.cpp
@@ -414,42 +414,3 @@ 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