[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326246: [analyzer] Track temporaries without construction
contexts for destruction. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43666?vs=135994&id=136138#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43666
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -452,6 +452,10 @@
ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
+ ExplodedNodeSet &PreVisit,
+ ExplodedNodeSet &Dst);
+
void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
Index: cfe/trunk/test/Analysis/temporaries.cpp
===
--- cfe/trunk/test/Analysis/temporaries.cpp
+++ cfe/trunk/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: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/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(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(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(S), PreVisit, Next);
+ getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
Bldr.addNodes(Dst);
break;
}
@@ -2194,13 +2224,13 @@
Pred->getLocationContext(),
[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
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(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(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(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);
@@ -
[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2262 + assert(DidCacheOutOnCleanup || + areInitializedTemporariesClear(Pred->getState(), Pred->getLocationContext(), dcoughlin wrote: > It sounds like this means if we did cache out then there are places where the > initialized temporaries are not cleared (that is, we have extra gunk in the > state that we don't want). > > Is that true? If so, then this relaxation of the assertion doesn't seem right > to me. > > Do we need to introduce a new program point when calling `Bldr.generateNode` > on the cleaned up state (for example, with a new tag or a new program point > kind)? This would make it so that when we cache out when generating a node > for the state with the cleaned up temporaries we know that it is safe to > early return from processEndOfFunction(). It would be safe because we would > know that the other node (the one we cached out because of) has already had > its temporaries cleared and notified the checkers about the end of the > function, etc. > > > > > Yep, this is a bug and also it can be simplified a lot. https://reviews.llvm.org/D43666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2234
+ // should go away, but the assertion should remain, without the
+ // "DidCacheOutOnCleanup" part, of course.
+ bool DidCacheOutOnCleanup = false;
Can you add a comment explaining the criteria under which the
DidCacheOutOnCleanup part can be removed?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2257
+ } else {
+Pred = *CleanUpTemporaries.begin();
+ }
I realize this isn't new code in this patch, but can you use the result of
Bldr.generateNode() to determine whether we cached out and also to get the new
Pred? That seems cleaner than querying the ExplodedNodeSet.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2262
+ assert(DidCacheOutOnCleanup ||
+ areInitializedTemporariesClear(Pred->getState(),
Pred->getLocationContext(),
It sounds like this means if we did cache out then there are places where the
initialized temporaries are not cleared (that is, we have extra gunk in the
state that we don't want).
Is that true? If so, then this relaxation of the assertion doesn't seem right
to me.
Do we need to introduce a new program point when calling `Bldr.generateNode` on
the cleaned up state (for example, with a new tag or a new program point kind)?
This would make it so that when we cache out when generating a node for the
state with the cleaned up temporaries we know that it is safe to early return
from processEndOfFunction(). It would be safe because we would know that the
other node (the one we cached out because of) has already had its temporaries
cleared and notified the checkers about the end of the function, etc.
Repository:
rC Clang
https://reviews.llvm.org/D43666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
This brings back code that was doing so earlier (when we had no constructed
contexts at all) but was removed too early in https://reviews.llvm.org/D43104,
as discussed in https://reviews.llvm.org/D43497. This allows us to call the
correct destructor sequence in the end of the operator `&&` in the test, even
if not all of them are inlined, because we keep track of all temporaries we've
constructed in order to resolve situations when the number or nature of
temporaries is not known in compile-time.
The test case provided in https://reviews.llvm.org/D43497 was not entirely
correct: the global variable would anyway be invalidated by the other
destructor that we don't inline yet. Though, yeah, it needs to be fixed as
well, but it isn't addressed by the fix that i've been thinking of (this patch).
Cleanup process misbehaves when the node after the cleanup is caching out: in
this case we take the node that wasn't cleaned up and hit the assertion. This
was fixed as well. It's a bit disappointing that our process of constructing a
single exploded node is so complicated and error-prone.
Repository:
rC Clang
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(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(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(S), PreVisit, Next);
+ getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
Bldr.addNodes(Dst);
break;
}
@@ -2194,13 +2224,15 @@
Pred->getLocationContext(),
