NoQ updated this revision to Diff 135994.
NoQ marked 3 inline comments as done.
NoQ added a comment.
Fix cleanup node generation logic.
https://reviews.llvm.org/D43666
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -900,7 +900,13 @@
class C {
public:
- ~C() { glob = 1; }
+ ~C() {
+ glob = 1;
+ clang_analyzer_checkInlined(true);
+#ifdef TEMPORARY_DTORS
+ // expected-warning@-2{{TRUE}}
+#endif
+ }
};
C get();
@@ -913,12 +919,11 @@
// temporaries: the return value of get() and the elidable copy constructor
// of that return value into is(). According to the CFG, we need to cleanup
// both of them depending on whether the temporary corresponding to the
- // return value of get() was initialized. However, for now we don't track
- // temporaries returned from functions, so we take the wrong branch.
+ // return value of get() was initialized. However, we didn't track
+ // temporaries returned from functions, so we took the wrong branch.
coin && is(get()); // no-crash
- // FIXME: We should have called the destructor, i.e. should be TRUE,
- // at least when we inline temporary destructors.
- clang_analyzer_eval(glob == 1); // expected-warning{{UNKNOWN}}
+ // FIXME: Should be true once we inline all destructors.
+ clang_analyzer_eval(glob); // expected-warning{{UNKNOWN}}
}
} // namespace dont_forget_destructor_around_logical_op
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1089,6 +1089,34 @@
}
}
+void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
+ ExplodedNodeSet &PreVisit,
+ ExplodedNodeSet &Dst) {
+ // This is a fallback solution in case we didn't have a construction
+ // context when we were constructing the temporary. Otherwise the map should
+ // have been populated there.
+ if (!getAnalysisManager().options.includeTemporaryDtorsInCFG()) {
+ // In case we don't have temporary destructors in the CFG, do not mark
+ // the initialization - we would otherwise never clean it up.
+ Dst = PreVisit;
+ return;
+ }
+ StmtNodeBuilder StmtBldr(PreVisit, Dst, *currBldrCtx);
+ for (ExplodedNode *Node : PreVisit) {
+ ProgramStateRef State = Node->getState();
+ const auto &Key = std::make_pair(BTE, Node->getStackFrame());
+
+ if (!State->contains<InitializedTemporaries>(Key)) {
+ // FIXME: Currently the state might also already contain the marker due to
+ // incorrect handling of temporaries bound to default parameters; for
+ // those, we currently skip the CXXBindTemporaryExpr but rely on adding
+ // temporary destructor nodes.
+ State = State->set<InitializedTemporaries>(Key, nullptr);
+ }
+ StmtBldr.generateNode(BTE, Node, State);
+ }
+}
+
namespace {
class CollectReachableSymbolsCallback final : public SymbolVisitor {
InvalidatedSymbols Symbols;
@@ -1251,7 +1279,9 @@
Bldr.takeNodes(Pred);
ExplodedNodeSet PreVisit;
getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
- getCheckerManager().runCheckersForPostStmt(Dst, PreVisit, S, *this);
+ ExplodedNodeSet Next;
+ VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), PreVisit, Next);
+ getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
Bldr.addNodes(Dst);
break;
}
@@ -2194,13 +2224,13 @@
Pred->getLocationContext(),
Pred->getStackFrame()->getParent()));
- // FIXME: We currently assert that temporaries are clear, as lifetime extended
- // temporaries are not always modelled correctly. In some cases when we
- // materialize the temporary, we do createTemporaryRegionIfNeeded(), and
- // the region changes, and also the respective destructor becomes automatic
- // from temporary. So for now clean up the state manually before asserting.
- // Ideally, the code above the assertion should go away, but the assertion
- // should remain.
+ // FIXME: We currently cannot assert that temporaries are clear, because
+ // lifetime extended temporaries are not always modelled correctly. In some
+ // cases when we materialize the temporary, we do
+ // createTemporaryRegionIfNeeded(), and the region changes, and also the
+ // respective destructor becomes automatic from temporary. So for now clean up
+ // the state manually before asserting. Ideally, the code above the assertion
+ // should go away, but the assertion should remain.
{
ExplodedNodeSet CleanUpTemporaries;
NodeBuilder Bldr(Pred, CleanUpTemporaries, BC);
@@ -2217,9 +2247,12 @@
LC = LC->getParent();
}
if (State != Pred->getState()) {
- Bldr.generateNode(Pred->getLocation(), State, Pred);
- assert(CleanUpTemporaries.size() <= 1);
- Pred = CleanUpTemporaries.empty() ? Pred : *CleanUpTemporaries.begin();
+ Pred = Bldr.generateNode(Pred->getLocation(), State, Pred);
+ if (!Pred) {
+ // The node with clean temporaries already exists. We might have reached
+ // it on a path on which we initialize different temporaries.
+ return;
+ }
}
}
assert(areInitializedTemporariesClear(Pred->getState(),
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -448,6 +448,10 @@
ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
+ ExplodedNodeSet &PreVisit,
+ ExplodedNodeSet &Dst);
+
void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
