NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
NoQ added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:68-71
typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
- const LocationContext *>, SVal>
- CXXNewAllocatorValuesMap;
+ const LocationContext *>,
+ SVal>
+ CXXNewAllocatorValuesMap;
----------------
For consistency.
When a temporary is constructed into a `CXXBindTemporaryExpr` (i.e. when it
needs a destructor), we can (since
https://reviews.llvm.org/D43056/https://reviews.llvm.org/D43062) pick up the
construction context and be sure that the temporary is initialized correctly.
Now, store the initialized temporary region in the program state with
`CXXBindTemporaryExpr` as its key, and when the time comes to call the
destructor, lookup the `CXXBindTemporaryExpr` in the `CFGTemporaryDtor`
element, then lookup the region in the program state.
For this purpose, the existing program state trait is used - the one that was
added by @klimek for making sure that the control flow for ternary operators
with temporaries is correct. Now the region is also stuffed into it.
Technically, it should be possible to reconstruct the region by having just the
temporary-expression and the location context. However, this would require
duplicating a bit of logic from the CFG.
When the this-region is available this way, give ourselves the privilege to
attempt inlining it. Of course, normal limitations of what we will or will not
inline will still apply.
Additionally, prevent the temporary from being garbage-collected from the
program state until the destructor call. I don't think
`SymRegion::isLiveRegion()` should be taught to query `ExprEngine` regarding
live temporaries - we should be just fine treating it as a regular program
state trait value liveness process. Also in this case `LiveVariables` analysis
isn't going to help much: temporaries definitely outlive their
`CXXBindTemporaryExpr`s sometimes. So i have an impression that this is the
right way to solve the liveness issue this time.
Repository:
rC Clang
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
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/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 &&
+ !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,21 @@
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;
+}
+
ProgramStateRef
ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
const CXXNewExpr *CNE,
@@ -390,11 +408,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 +445,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 +559,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 +927,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 +949,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 +964,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 +974,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 +1136,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,15 @@
/// 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);
+
/// 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
