Revert "Reapply "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression" (#127338)" #128205
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and default init expression" (llvm#127338)" This reverts commit d235b72.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (yronglin) ChangesThis reverts commit d235b72. Full diff: https://github.com/llvm/llvm-project/pull/128205.diff 10 Files Affected:
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
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: None (yronglin) ChangesThis reverts commit d235b72. Full diff: https://github.com/llvm/llvm-project/pull/128205.diff 10 Files Affected:
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
|
You can test this locally with the following command:git-clang-format --diff ab098a7ebf07227a371df95ce65bd4aa135dee9a 797ffc34ab4a41a8437bbf02f1fc52ac8b155c93 --extensions cpp -- 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.cppView the diff from clang-format here.diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 4a5eeb9551..5dee2e582e 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2013,8 +2013,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ProgramStateRef State = I->getState();
State = State->BindExpr(S, LCtx, *ConstantVal);
if (IsTemporary)
- State = createTemporaryRegionIfNeeded(State, LCtx,
- cast<Expr>(S),
+ State = createTemporaryRegionIfNeeded(State, LCtx, cast<Expr>(S),
cast<Expr>(S));
Bldr2.generateNode(S, I, State);
}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit d235b72.