NoQ updated this revision to Diff 133672.
NoQ marked an inline comment as done.
NoQ added a comment.
Assert that the destructors are cleaned up. This assertion is pretty important
because it says that we didn't miss any destructors for initialized temporaries
during analysis.
Disable tracking initialized temporaries when `cfg-temporary-dtors` is off - a
bug i introduced when moving the code, which was found by the newly added
assertion :)
https://reviews.llvm.org/D43104
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -779,3 +779,81 @@
}
} // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+class C {
+public:
+ int &x, &y;
+ C(int &_x, int &_y) : x(_x), y(_y) { ++x; }
+ C(const C &c): x(c.x), y(c.y) { ++x; }
+ ~C() { ++y; }
+};
+
+void test_simple_temporary() {
+ int x = 0, y = 0;
+ {
+ const C &c = C(x, y);
+ }
+ // One constructor and one destructor.
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+#else
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+ int x = 0, y = 0;
+ {
+ C c = C(x, y);
+ }
+ // Two constructors (temporary object expr and copy) and two destructors.
+ clang_analyzer_eval(x == 2);
+ clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+#else
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+ int x = 0, y = 0, z = 0, w = 0;
+ {
+ C c = coin ? C(x, y) : C(z, w);
+ }
+ // This time each branch contains an additional elidable copy constructor.
+ if (coin) {
+ clang_analyzer_eval(x == 3);
+ clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+#else
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+#endif
+ clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+ } else {
+ clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(z == 3);
+ clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+ // expected-warning@-3{{TRUE}}
+ // expected-warning@-3{{TRUE}}
+#else
+ // expected-warning@-6{{UNKNOWN}}
+ // expected-warning@-6{{UNKNOWN}}
+#endif
+ }
+}
+} // namespace test_match_constructors_and_destructors
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -332,6 +332,7 @@
// See if we have any stale C++ allocator values.
assert(areCXXNewAllocatorValuesClear(CEEState, calleeCtx, callerCtx));
+ assert(areInitializedTemporariesClear(CEEState, calleeCtx, callerCtx));
ExplodedNode *CEENode = G.getNode(Loc, CEEState, false, &isNew);
CEENode->addPredecessor(*I, G);
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -102,14 +102,15 @@
const MemRegion *
ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
ExplodedNode *Pred,
+ const ConstructionContext *CC,
EvalCallOptions &CallOpts) {
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
// See if we're constructing an existing region by looking at the
// current construction context.
- if (auto CC = getCurrentCFGElement().getAs<CFGConstructor>()) {
+ if (CC) {
if (const Stmt *TriggerStmt = CC->getTriggerStmt()) {
if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(TriggerStmt)) {
if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
@@ -217,10 +218,20 @@
// the entire array).
EvalCallOptions CallOpts;
+ auto C = getCurrentCFGElement().getAs<CFGConstructor>();
+ const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
+
+ const CXXBindTemporaryExpr *BTE = nullptr;
switch (CE->getConstructionKind()) {
case CXXConstructExpr::CK_Complete: {
- Target = getRegionForConstructedObject(CE, Pred, CallOpts);
+ Target = getRegionForConstructedObject(CE, Pred, CC, CallOpts);
+ if (CC && AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG() &&
+ !CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion &&
+ CallOpts.IsTemporaryCtorOrDtor) {
+ // May as well be a ReturnStmt.
+ BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+ }
break;
}
case CXXConstructExpr::CK_VirtualBase:
@@ -288,17 +299,18 @@
ExplodedNodeSet DstPreVisit;
getCheckerManager().runCheckersForPreStmt(DstPreVisit, Pred, CE, *this);
+ // FIXME: Is it possible and/or useful to do this before PreStmt?
ExplodedNodeSet PreInitialized;
{
StmtNodeBuilder Bldr(DstPreVisit, PreInitialized, *currBldrCtx);
- if (CE->requiresZeroInitialization()) {
- // Type of the zero doesn't matter.
- SVal ZeroVal = svalBuilder.makeZeroVal(getContext().CharTy);
-
- for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
- E = DstPreVisit.end();
- I != E; ++I) {
- ProgramStateRef State = (*I)->getState();
+ for (ExplodedNodeSet::iterator I = DstPreVisit.begin(),
+ E = DstPreVisit.end();
+ I != E; ++I) {
+ ProgramStateRef State = (*I)->getState();
+ if (CE->requiresZeroInitialization()) {
+ // Type of the zero doesn't matter.
+ SVal ZeroVal = svalBuilder.makeZeroVal(getContext().CharTy);
+
// FIXME: Once we properly handle constructors in new-expressions, we'll
// need to invalidate the region before setting a default value, to make
// sure there aren't any lingering bindings around. This probably needs
@@ -312,9 +324,15 @@
// since it's then possible to be initializing one part of a multi-
// dimensional array.
State = State->bindDefault(loc::MemRegionVal(Target), ZeroVal, LCtx);
- Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
- ProgramPoint::PreStmtKind);
}
+
+ if (BTE) {
+ State = addInitializedTemporary(State, BTE, LCtx,
+ cast<CXXTempObjectRegion>(Target));
+ }
+
+ Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
+ ProgramPoint::PreStmtKind);
}
}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -54,18 +54,21 @@
STATISTIC(NumTimesRetriedWithoutInlining,
"The # of times we re-evaluated a call without inlining");
-typedef std::pair<const CXXBindTemporaryExpr *, const StackFrameContext *>
- CXXBindTemporaryContext;
+typedef llvm::ImmutableMap<std::pair<const CXXBindTemporaryExpr *,
+ const StackFrameContext *>,
+ const CXXTempObjectRegion *>
+ InitializedTemporariesMap;
// Keeps track of whether CXXBindTemporaryExpr nodes have been evaluated.
// The StackFrameContext assures that nested calls due to inlined recursive
// functions do not interfere.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet,
- llvm::ImmutableSet<CXXBindTemporaryContext>)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
+ InitializedTemporariesMap)
typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
- const LocationContext *>, SVal>
- CXXNewAllocatorValuesMap;
+ const LocationContext *>,
+ SVal>
+ CXXNewAllocatorValuesMap;
// Keeps track of return values of various operator new() calls between
// evaluation of the inlined operator new(), through the constructor call,
@@ -323,6 +326,36 @@
return State;
}
+ProgramStateRef ExprEngine::addInitializedTemporary(
+ ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC, const CXXTempObjectRegion *R) {
+ const auto &Key = std::make_pair(BTE, LC->getCurrentStackFrame());
+ if (!State->contains<InitializedTemporaries>(Key)) {
+ return State->set<InitializedTemporaries>(Key, R);
+ }
+
+ // FIXME: Currently the state might 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. Otherwise, this branch should be unreachable.
+ return State;
+}
+
+bool ExprEngine::areInitializedTemporariesClear(ProgramStateRef State,
+ const LocationContext *FromLC,
+ const LocationContext *ToLC) {
+ const LocationContext *LC = FromLC;
+ do {
+ for (auto I : State->get<InitializedTemporaries>())
+ if (I.first.second == LC)
+ return false;
+
+ LC = LC->getParent();
+ assert(LC && "ToLC must be a parent of FromLC!");
+ } while (LC != ToLC);
+ return true;
+}
+
ProgramStateRef
ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
const CXXNewExpr *CNE,
@@ -390,11 +423,16 @@
const LocationContext *LC) {
PrintingPolicy PP =
LC->getAnalysisDeclContext()->getASTContext().getPrintingPolicy();
- for (auto I : State->get<InitializedTemporariesSet>()) {
- if (I.second != LC)
+ for (auto I : State->get<InitializedTemporaries>()) {
+ std::pair<const CXXBindTemporaryExpr *, const LocationContext *> Key =
+ I.first;
+ const MemRegion *Value = I.second;
+ if (Key.second != LC)
continue;
- Out << '(' << I.second << ',' << I.first << ") ";
- I.first->printPretty(Out, nullptr, PP);
+ Out << '(' << Key.second << ',' << Key.first << ") ";
+ Key.first->printPretty(Out, nullptr, PP);
+ if (Value)
+ Out << " : " << Value;
Out << NL;
}
}
@@ -422,7 +460,7 @@
const char *NL, const char *Sep,
const LocationContext *LCtx) {
if (LCtx) {
- if (!State->get<InitializedTemporariesSet>().isEmpty()) {
+ if (!State->get<InitializedTemporaries>().isEmpty()) {
Out << Sep << "Initialized temporaries:" << NL;
LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) {
@@ -536,6 +574,10 @@
const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr;
SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
+ for (auto I : CleanedState->get<InitializedTemporaries>())
+ if (I.second)
+ SymReaper.markLive(I.second);
+
for (auto I : CleanedState->get<CXXNewAllocatorValues>()) {
if (SymbolRef Sym = I.second.getAsSymbol())
SymReaper.markLive(Sym);
@@ -900,12 +942,18 @@
ExplodedNodeSet CleanDtorState;
StmtNodeBuilder StmtBldr(Pred, CleanDtorState, *currBldrCtx);
ProgramStateRef State = Pred->getState();
- if (State->contains<InitializedTemporariesSet>(
- std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()))) {
+ const MemRegion *MR = nullptr;
+ if (const CXXTempObjectRegion *const *MRPtr =
+ State->get<InitializedTemporaries>(std::make_pair(
+ D.getBindTemporaryExpr(), Pred->getStackFrame()))) {
// FIXME: Currently we insert temporary destructors for default parameters,
- // but we don't insert the constructors.
- State = State->remove<InitializedTemporariesSet>(
+ // but we don't insert the constructors, so the entry in
+ // InitializedTemporaries may be missing.
+ State = State->remove<InitializedTemporaries>(
std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()));
+ // *MRPtr may still be null when the construction context for the temporary
+ // was not implemented.
+ MR = *MRPtr;
}
StmtBldr.generateNode(D.getBindTemporaryExpr(), Pred, State);
@@ -916,12 +964,11 @@
ExplodedNode *CleanPred =
CleanDtorState.empty() ? Pred : *CleanDtorState.begin();
- // FIXME: Inlining of temporary destructors is not supported yet anyway, so
- // we just put a NULL region for now. This will need to be changed later.
EvalCallOptions CallOpts;
CallOpts.IsTemporaryCtorOrDtor = true;
- CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
- VisitCXXDestructor(varType, nullptr, D.getBindTemporaryExpr(),
+ if (!MR)
+ CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+ VisitCXXDestructor(varType, MR, D.getBindTemporaryExpr(),
/*IsBase=*/false, CleanPred, Dst, CallOpts);
}
@@ -932,7 +979,7 @@
const CFGBlock *DstT,
const CFGBlock *DstF) {
BranchNodeBuilder TempDtorBuilder(Pred, Dst, BldCtx, DstT, DstF);
- if (Pred->getState()->contains<InitializedTemporariesSet>(
+ if (Pred->getState()->contains<InitializedTemporaries>(
std::make_pair(BTE, Pred->getStackFrame()))) {
TempDtorBuilder.markInfeasible(false);
TempDtorBuilder.generateNode(Pred->getState(), true, Pred);
@@ -942,32 +989,6 @@
}
}
-void ExprEngine::VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
- ExplodedNodeSet &PreVisit,
- ExplodedNodeSet &Dst) {
- 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();
-
- if (!State->contains<InitializedTemporariesSet>(
- std::make_pair(BTE, Node->getStackFrame()))) {
- // FIXME: Currently the state might 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->add<InitializedTemporariesSet>(
- std::make_pair(BTE, Node->getStackFrame()));
- }
- StmtBldr.generateNode(BTE, Node, State);
- }
-}
-
namespace {
class CollectReachableSymbolsCallback final : public SymbolVisitor {
InvalidatedSymbols Symbols;
@@ -1130,9 +1151,7 @@
Bldr.takeNodes(Pred);
ExplodedNodeSet PreVisit;
getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
- ExplodedNodeSet Next;
- VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), PreVisit, Next);
- getCheckerManager().runCheckersForPostStmt(Dst, Next, S, *this);
+ getCheckerManager().runCheckersForPostStmt(Dst, PreVisit, S, *this);
Bldr.addNodes(Dst);
break;
}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -1197,9 +1197,8 @@
// destructors, though this could change in the future.
const CFGBlock *B = CalleeCtx->getCallSiteBlock();
CFGElement E = (*B)[CalleeCtx->getIndex()];
- assert(E.getAs<CFGImplicitDtor>() &&
+ assert(E.getAs<CFGImplicitDtor>() || E.getAs<CFGTemporaryDtor>() &&
"All other CFG elements should have exprs");
- assert(!E.getAs<CFGTemporaryDtor>() && "We don't handle temporaries yet");
SValBuilder &SVB = State->getStateManager().getSValBuilder();
const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CalleeCtx->getDecl());
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -448,10 +448,6 @@
ExplodedNode *Pred,
ExplodedNodeSet &Dst);
- void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
- ExplodedNodeSet &PreVisit,
- ExplodedNodeSet &Dst);
-
void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
@@ -696,8 +692,22 @@
/// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts.
const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
ExplodedNode *Pred,
+ const ConstructionContext *CC,
EvalCallOptions &CallOpts);
+ /// Store the region of a C++ temporary object corresponding to a
+ /// CXXBindTemporaryExpr for later destruction.
+ static ProgramStateRef addInitializedTemporary(
+ ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+ const LocationContext *LC, const CXXTempObjectRegion *R);
+
+ /// Check if all initialized temporary regions are clear for the given
+ /// context range (including FromLC, not including ToLC).
+ /// This is useful for assertions.
+ static bool areInitializedTemporariesClear(ProgramStateRef State,
+ const LocationContext *FromLC,
+ const LocationContext *ToLC);
+
/// Store the region returned by operator new() so that the constructor
/// that follows it knew what location to initialize. The value should be
/// cleared once the respective CXXNewExpr CFGStmt element is processed.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
