Revert "[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression (#91879)" #94597
+42
−103
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
…ult init expression (llvm#91879)" This depends on llvm#92527 which needs to be reverted due to llvm#92527 (comment). This reverts commit 905b402.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: None (bgra8) ChangesThis depends on #92527 which This reverts commit 905b402. Full diff: https://github.com/llvm/llvm-project/pull/94597.diff 6 Files Affected:
diff --git a/clang/lib/AST/ParentMap.cpp b/clang/lib/AST/ParentMap.cpp
index 534793b837bbb..3d6a1cc84c7b1 100644
--- a/clang/lib/AST/ParentMap.cpp
+++ b/clang/lib/AST/ParentMap.cpp
@@ -97,22 +97,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 02317257c2740..64e6155de090c 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);
@@ -2258,10 +2254,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);
@@ -2431,40 +2433,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/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index fb5ca199b3fc6..d8c77e3e0a5cd 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5608,7 +5608,6 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
InitializationContext.emplace(Loc, Field, CurContext);
Expr *Init = nullptr;
- bool HasRewrittenInit = false;
bool NestedDefaultChecking = isCheckingDefaultArgumentOrInitializer();
bool InLifetimeExtendingContext = isInLifetimeExtendingContext();
@@ -5658,7 +5657,6 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
isa_and_present<ExprWithCleanups>(Field->getInClassInitializer());
if (V.HasImmediateCalls || InLifetimeExtendingContext ||
ContainsAnyTemporaries) {
- HasRewrittenInit = true;
ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
CurContext};
ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
@@ -5702,7 +5700,7 @@ ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
return CXXDefaultInitExpr::Create(Context, InitializationContext->Loc,
Field, InitializationContext->Context,
- HasRewrittenInit ? Init : nullptr);
+ Init);
}
// DR1351:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 290d96611d466..197d673107285 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1971,45 +1971,33 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet Tmp;
StmtNodeBuilder Bldr2(PreVisit, Tmp, *currBldrCtx);
- bool HasRewrittenInit = 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();
- HasRewrittenInit = DefE->hasRewrittenInit();
- } else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S)) {
+ else if (const auto *DefE = dyn_cast<CXXDefaultInitExpr>(S))
ArgE = DefE->getExpr();
- HasRewrittenInit = DefE->hasRewrittenInit();
- } else
+ else
llvm_unreachable("unknown constant wrapper kind");
- if (HasRewrittenInit) {
- 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/Analysis/cxx-uninitialized-object.cpp b/clang/test/Analysis/cxx-uninitialized-object.cpp
index aee0dae15fbfa..e3fa8ae8d7f29 100644
--- a/clang/test/Analysis/cxx-uninitialized-object.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object.cpp
@@ -1114,27 +1114,27 @@ void fCXX11MemberInitTest1() {
CXX11MemberInitTest1();
}
-#ifdef PEDANTIC
struct CXX11MemberInitTest2 {
struct RecordType {
- int a; // expected-note {{uninitialized field 'this->a'}}
- int b; // expected-note {{uninitialized field 'this->b'}}
+ // TODO: we'd expect the note: {{uninitialized field 'this->rec.a'}}
+ int a; // no-note
+ // TODO: we'd expect the note: {{uninitialized field 'this->rec.b'}}
+ int b; // no-note
RecordType(int) {}
};
- RecordType rec = RecordType(int()); // expected-warning {{2 uninitialized fields}}
+ RecordType rec = RecordType(int());
int dontGetFilteredByNonPedanticMode = 0;
CXX11MemberInitTest2() {}
};
void fCXX11MemberInitTest2() {
+ // TODO: we'd expect the warning: {{2 uninitializeds field}}
CXX11MemberInitTest2(); // no-warning
}
-#endif // PEDANTIC
-
//===----------------------------------------------------------------------===//
// "Esoteric" primitive type tests.
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/lifetime-extended-regions.cpp b/clang/test/Analysis/lifetime-extended-regions.cpp
index 524f4e0c400d1..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 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() {
|
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 depends on #92527 which
needs to be reverted due to
#92527 (comment).
This reverts commit 905b402.