https://github.com/yronglin created
https://github.com/llvm/llvm-project/pull/128205
This reverts commit d235b72178adc710bf704078fbe0cd687642f3e0.
>From 797ffc34ab4a41a8437bbf02f1fc52ac8b155c93 Mon Sep 17 00:00:00 2001
From: yronglin <yronglin...@gmail.com>
Date: Sat, 22 Feb 2025 01:16:59 +0800
Subject: [PATCH] Revert "Reapply "[Analyzer][CFG] Correctly handle rebuilt
default arg and default init expression" (#127338)"
This reverts commit d235b72178adc710bf704078fbe0cd687642f3e0.
---
clang/docs/ReleaseNotes.rst | 4 -
clang/lib/AST/ParentMap.cpp | 17 -----
clang/lib/Analysis/CFG.cpp | 54 +++----------
clang/lib/Analysis/ReachableCode.cpp | 37 +++++----
clang/lib/Sema/SemaExpr.cpp | 9 +--
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 56 ++++++--------
clang/test/AST/ast-dump-recovery.cpp | 2 +-
.../Analysis/lifetime-extended-regions.cpp | 7 +-
clang/test/SemaCXX/cxx2c-placeholder-vars.cpp | 8 +-
clang/test/SemaCXX/warn-unreachable.cpp | 75 -------------------
10 files changed, 61 insertions(+), 208 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 01c58295613d7..3792a235538fa 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -284,10 +284,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 580613b2618fb..e62e71bf5a514 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 c82dbc42fb9d8..3e144395cffc6 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);
@@ -2267,10 +2263,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);
@@ -2440,44 +2442,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()->IgnoreParens(), 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);
- }
-
- // Unlike CXXDefaultArgExpr::getExpr stripped off the top level FullExpr
and
- // ConstantExpr, CXXDefaultInitExpr::getExpr does not do this, so we don't
- // need to ignore ParenExprs, because the top level will not be a
ParenExpr.
- 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 3b1f716f8dea1..dd81c8e0a3d54 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 16226f1ae6550..316a59cb80e60 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5571,10 +5571,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,
@@ -5692,11 +5690,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 c3dcdc985a935..4a5eeb95511a0 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1990,45 +1990,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 fa6d747556dd8..b59fa3778192f 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 02a1210d9af92..4458ad294af7c 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 37824c16f4f05..8e428c0ef0427 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 c96f26e196451..e6f5bc5ef8e12 100644
--- a/clang/test/SemaCXX/warn-unreachable.cpp
+++ b/clang/test/SemaCXX/warn-unreachable.cpp
@@ -414,78 +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
-
-// This issue reported by the comments in
https://github.com/llvm/llvm-project/pull/117437.
-// All block-level expressions should have already been IgnoreParens()ed.
-namespace gh117437_ignore_parens_in_default_arg {
- class Location {
- public:
- static Location Current(int = __builtin_LINE());
- };
- class DOMMatrix;
- class BasicMember {
- public:
- BasicMember(DOMMatrix *);
- };
- template <typename> using Member = BasicMember;
- class ExceptionState {
- public:
- ExceptionState &ReturnThis();
- ExceptionState(Location);
- };
- class NonThrowableExceptionState : public ExceptionState {
- public:
- NonThrowableExceptionState(Location location = Location::Current())
- : ExceptionState(location) {}
- };
- class DOMMatrix {
- public:
- static DOMMatrix *
- Create(int *, ExceptionState & =
(NonThrowableExceptionState().ReturnThis()));
- };
- class CSSMatrixComponent {
- int CSSMatrixComponent_matrix;
- CSSMatrixComponent()
- : matrix_(DOMMatrix::Create(&CSSMatrixComponent_matrix)) {}
- Member<DOMMatrix> matrix_;
- };
-} // namespace gh117437_ignore_parens_in_default_arg
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
