r326236 - [analyzer] Introduce correct lifetime extension behavior in simple cases.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 27 11:47:49 PST 2018
Author: dergachev
Date: Tue Feb 27 11:47:49 2018
New Revision: 326236
URL: http://llvm.org/viewvc/llvm-project?rev=326236&view=rev
Log:
[analyzer] Introduce correct lifetime extension behavior in simple cases.
This patch uses the reference to MaterializeTemporaryExpr stored in the
construction context since r326014 in order to model that expression correctly.
When modeling MaterializeTemporaryExpr, instead of copying the raw memory
contents from the sub-expression's rvalue to a completely new temporary region,
that we conjure up for the lack of better options, we now have the better
option to recall the region into which the object was originally constructed
and declare that region to be the value of the expression, which is semantically
correct.
This only works when the construction context is available, which is worked on
independently.
The temporary region's liveness (in the sense of removeDeadBindings) is extended
until the MaterializeTemporaryExpr is resolved, in order to keep the store
bindings around, because it wouldn't be referenced from anywhere else in the
program state.
Differential Revision: https://reviews.llvm.org/D43497
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/test/Analysis/lifetime-extension.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=326236&r1=326235&r2=326236&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h Tue Feb 27 11:47:49 2018
@@ -708,6 +708,19 @@ private:
const LocationContext *FromLC,
const LocationContext *ToLC);
+ /// Store the region of a C++ temporary object corresponding to a
+ /// CXXBindTemporaryExpr for later destruction.
+ static ProgramStateRef addTemporaryMaterialization(
+ ProgramStateRef State, const MaterializeTemporaryExpr *MTE,
+ const LocationContext *LC, const CXXTempObjectRegion *R);
+
+ /// Check if all temporary materialization regions are clear for the given
+ /// context range (including FromLC, not including ToLC).
+ /// This is useful for assertions.
+ static bool areTemporaryMaterializationsClear(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.
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=326236&r1=326235&r2=326236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Tue Feb 27 11:47:49 2018
@@ -65,6 +65,17 @@ typedef llvm::ImmutableMap<std::pair<con
REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
InitializedTemporariesMap)
+typedef llvm::ImmutableMap<std::pair<const MaterializeTemporaryExpr *,
+ const StackFrameContext *>,
+ const CXXTempObjectRegion *>
+ TemporaryMaterializationMap;
+
+// Keeps track of temporaries that will need to be materialized later.
+// The StackFrameContext assures that nested calls due to inlined recursive
+// functions do not interfere.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(TemporaryMaterializations,
+ TemporaryMaterializationMap)
+
typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
const LocationContext *>,
SVal>
@@ -255,17 +266,35 @@ ExprEngine::createTemporaryRegionIfNeede
const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
CommaLHSs, Adjustments);
+ // Take the region for Init, i.e. for the whole object. If we do not remember
+ // the region in which the object originally was constructed, come up with
+ // a new temporary region out of thin air and copy the contents of the object
+ // (which are currently present in the Environment, because Init is an rvalue)
+ // into that region. This is not correct, but it is better than nothing.
+ bool FoundOriginalMaterializationRegion = false;
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
dyn_cast<MaterializeTemporaryExpr>(Result)) {
- StorageDuration SD = MT->getStorageDuration();
- // If this object is bound to a reference with static storage duration, we
- // put it in a different region to prevent "address leakage" warnings.
- if (SD == SD_Static || SD == SD_Thread)
- TR = MRMgr.getCXXStaticTempObjectRegion(Init);
- }
- if (!TR)
+ auto Key = std::make_pair(MT, LC->getCurrentStackFrame());
+ if (const CXXTempObjectRegion *const *TRPtr =
+ State->get<TemporaryMaterializations>(Key)) {
+ FoundOriginalMaterializationRegion = true;
+ TR = *TRPtr;
+ assert(TR);
+ State = State->remove<TemporaryMaterializations>(Key);
+ } else {
+ StorageDuration SD = MT->getStorageDuration();
+ // If this object is bound to a reference with static storage duration, we
+ // put it in a different region to prevent "address leakage" warnings.
+ if (SD == SD_Static || SD == SD_Thread) {
+ TR = MRMgr.getCXXStaticTempObjectRegion(Init);
+ } else {
+ TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+ }
+ }
+ } else {
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
+ }
SVal Reg = loc::MemRegionVal(TR);
SVal BaseReg = Reg;
@@ -287,32 +316,35 @@ ExprEngine::createTemporaryRegionIfNeede
}
}
- // What remains is to copy the value of the object to the new region.
- // FIXME: In other words, what we should always do is copy value of the
- // Init expression (which corresponds to the bigger object) to the whole
- // temporary region TR. However, this value is often no longer present
- // in the Environment. If it has disappeared, we instead invalidate TR.
- // Still, what we can do is assign the value of expression Ex (which
- // corresponds to the sub-object) to the TR's sub-region Reg. At least,
- // values inside Reg would be correct.
- SVal InitVal = State->getSVal(Init, LC);
- if (InitVal.isUnknown()) {
- InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
- currBldrCtx->blockCount());
- State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
-
- // Then we'd need to take the value that certainly exists and bind it over.
- if (InitValWithAdjustments.isUnknown()) {
- // Try to recover some path sensitivity in case we couldn't
- // compute the value.
- InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
- Result, LC, InitWithAdjustments->getType(),
- currBldrCtx->blockCount());
+ if (!FoundOriginalMaterializationRegion) {
+ // What remains is to copy the value of the object to the new region.
+ // FIXME: In other words, what we should always do is copy value of the
+ // Init expression (which corresponds to the bigger object) to the whole
+ // temporary region TR. However, this value is often no longer present
+ // in the Environment. If it has disappeared, we instead invalidate TR.
+ // Still, what we can do is assign the value of expression Ex (which
+ // corresponds to the sub-object) to the TR's sub-region Reg. At least,
+ // values inside Reg would be correct.
+ SVal InitVal = State->getSVal(Init, LC);
+ if (InitVal.isUnknown()) {
+ InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
+ currBldrCtx->blockCount());
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+
+ // Then we'd need to take the value that certainly exists and bind it
+ // over.
+ if (InitValWithAdjustments.isUnknown()) {
+ // Try to recover some path sensitivity in case we couldn't
+ // compute the value.
+ InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
+ Result, LC, InitWithAdjustments->getType(),
+ currBldrCtx->blockCount());
+ }
+ State =
+ State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
+ } else {
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
}
- State =
- State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
- } else {
- State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
}
// The result expression would now point to the correct sub-region of the
@@ -320,8 +352,10 @@ ExprEngine::createTemporaryRegionIfNeede
// correctly in case (Result == Init).
State = State->BindExpr(Result, LC, Reg);
- // Notify checkers once for two bindLoc()s.
- State = processRegionChange(State, TR, LC);
+ if (!FoundOriginalMaterializationRegion) {
+ // Notify checkers once for two bindLoc()s.
+ State = processRegionChange(State, TR, LC);
+ }
return State;
}
@@ -356,6 +390,29 @@ bool ExprEngine::areInitializedTemporari
return true;
}
+ProgramStateRef ExprEngine::addTemporaryMaterialization(
+ ProgramStateRef State, const MaterializeTemporaryExpr *MTE,
+ const LocationContext *LC, const CXXTempObjectRegion *R) {
+ const auto &Key = std::make_pair(MTE, LC->getCurrentStackFrame());
+ assert(!State->contains<TemporaryMaterializations>(Key));
+ return State->set<TemporaryMaterializations>(Key, R);
+}
+
+bool ExprEngine::areTemporaryMaterializationsClear(
+ ProgramStateRef State, const LocationContext *FromLC,
+ const LocationContext *ToLC) {
+ const LocationContext *LC = FromLC;
+ while (LC != ToLC) {
+ assert(LC && "ToLC must be a parent of FromLC!");
+ for (auto I : State->get<TemporaryMaterializations>())
+ if (I.first.second == LC)
+ return false;
+
+ LC = LC->getParent();
+ }
+ return true;
+}
+
ProgramStateRef
ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
const CXXNewExpr *CNE,
@@ -437,6 +494,24 @@ static void printInitializedTemporariesF
}
}
+static void printTemporaryMaterializationsForContext(
+ raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
+ const LocationContext *LC) {
+ PrintingPolicy PP =
+ LC->getAnalysisDeclContext()->getASTContext().getPrintingPolicy();
+ for (auto I : State->get<TemporaryMaterializations>()) {
+ std::pair<const MaterializeTemporaryExpr *, const LocationContext *> Key =
+ I.first;
+ const MemRegion *Value = I.second;
+ if (Key.second != LC)
+ continue;
+ Out << '(' << Key.second << ',' << Key.first << ") ";
+ Key.first->printPretty(Out, nullptr, PP);
+ assert(Value);
+ Out << " : " << Value << NL;
+ }
+}
+
static void printCXXNewAllocatorValuesForContext(raw_ostream &Out,
ProgramStateRef State,
const char *NL,
@@ -468,6 +543,14 @@ void ExprEngine::printState(raw_ostream
});
}
+ if (!State->get<TemporaryMaterializations>().isEmpty()) {
+ Out << Sep << "Temporaries to be materialized:" << NL;
+
+ LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) {
+ printTemporaryMaterializationsForContext(Out, State, NL, Sep, LC);
+ });
+ }
+
if (!State->get<CXXNewAllocatorValues>().isEmpty()) {
Out << Sep << "operator new() allocator return values:" << NL;
@@ -578,6 +661,10 @@ void ExprEngine::removeDead(ExplodedNode
if (I.second)
SymReaper.markLive(I.second);
+ for (auto I : CleanedState->get<TemporaryMaterializations>())
+ if (I.second)
+ SymReaper.markLive(I.second);
+
for (auto I : CleanedState->get<CXXNewAllocatorValues>()) {
if (SymbolRef Sym = I.second.getAsSymbol())
SymReaper.markLive(Sym);
@@ -2108,11 +2195,12 @@ void ExprEngine::processEndOfFunction(No
Pred->getStackFrame()->getParent()));
// FIXME: We currently assert that temporaries are clear, as lifetime extended
- // temporaries are not modelled correctly. 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.
+ // 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);
@@ -2137,6 +2225,9 @@ void ExprEngine::processEndOfFunction(No
assert(areInitializedTemporariesClear(Pred->getState(),
Pred->getLocationContext(),
Pred->getStackFrame()->getParent()));
+ assert(areTemporaryMaterializationsClear(Pred->getState(),
+ Pred->getLocationContext(),
+ Pred->getStackFrame()->getParent()));
PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
StateMgr.EndPath(Pred->getState());
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=326236&r1=326235&r2=326236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Tue Feb 27 11:47:49 2018
@@ -230,6 +230,7 @@ void ExprEngine::VisitCXXConstructExpr(c
const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
const CXXBindTemporaryExpr *BTE = nullptr;
+ const MaterializeTemporaryExpr *MTE = nullptr;
switch (CE->getConstructionKind()) {
case CXXConstructExpr::CK_Complete: {
@@ -237,8 +238,13 @@ void ExprEngine::VisitCXXConstructExpr(c
if (CC && AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG() &&
!CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion &&
CallOpts.IsTemporaryCtorOrDtor) {
- // May as well be a ReturnStmt.
- BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+ MTE = CC->getMaterializedTemporary();
+ if (!MTE || MTE->getStorageDuration() == SD_FullExpression) {
+ // If the temporary is lifetime-extended, don't save the BTE,
+ // because we don't need a temporary destructor, but an automatic
+ // destructor. The cast may fail because it may as well be a ReturnStmt.
+ BTE = dyn_cast<CXXBindTemporaryExpr>(CC->getTriggerStmt());
+ }
}
break;
}
@@ -339,6 +345,11 @@ void ExprEngine::VisitCXXConstructExpr(c
cast<CXXTempObjectRegion>(Target));
}
+ if (MTE) {
+ State = addTemporaryMaterialization(State, MTE, LCtx,
+ cast<CXXTempObjectRegion>(Target));
+ }
+
Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
ProgramPoint::PreStmtKind);
}
Modified: cfe/trunk/test/Analysis/lifetime-extension.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/lifetime-extension.cpp?rev=326236&r1=326235&r2=326236&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/lifetime-extension.cpp (original)
+++ cfe/trunk/test/Analysis/lifetime-extension.cpp Tue Feb 27 11:47:49 2018
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true -DTEMPORARIES -verify %s
void clang_analyzer_eval(bool);
@@ -38,9 +39,157 @@ void f() {
const int &y = A().j[1]; // no-crash
const int &z = (A().j[1], A().j[0]); // no-crash
- // FIXME: All of these should be TRUE, but constructors aren't inlined.
- clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 3);
+ clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning at -4{{TRUE}}
+ // expected-warning at -4{{TRUE}}
+ // expected-warning at -4{{TRUE}}
+#else
+ // expected-warning at -8{{UNKNOWN}}
+ // expected-warning at -8{{UNKNOWN}}
+ // expected-warning at -8{{UNKNOWN}}
+#endif
}
} // end namespace pr19539_crash_on_destroying_an_integer
+
+namespace maintain_original_object_address_on_lifetime_extension {
+class C {
+ C **after, **before;
+
+public:
+ bool x;
+
+ C(bool x, C **after, C **before) : x(x), after(after), before(before) {
+ *before = this;
+ }
+
+ // Don't track copies in our tests.
+ C(const C &c) : x(c.x), after(nullptr), before(nullptr) {}
+
+ ~C() { if (after) *after = this; }
+
+ operator bool() const { return x; }
+};
+
+void f1() {
+ C *after, *before;
+ {
+ const C &c = C(true, &after, &before);
+ }
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning at -2{{TRUE}}
+#else
+ // expected-warning at -4{{UNKNOWN}}
+#endif
+}
+
+void f2() {
+ C *after, *before;
+ C c = C(1, &after, &before);
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning at -2{{TRUE}}
+#else
+ // expected-warning at -4{{UNKNOWN}}
+#endif
+}
+
+void f3(bool coin) {
+ C *after, *before;
+ {
+ const C &c = coin ? C(true, &after, &before) : C(false, &after, &before);
+ }
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning at -2{{TRUE}}
+#else
+ // expected-warning at -4{{UNKNOWN}}
+#endif
+}
+
+void f4(bool coin) {
+ C *after, *before;
+ {
+ // no-crash
+ const C &c = C(coin, &after, &before) ?: C(false, &after, &before);
+ }
+ // FIXME: Add support for lifetime extension through binary conditional
+ // operator. Ideally also add support for the binary conditional operator in
+ // C++. Because for now it calls the constructor for the condition twice.
+ if (coin) {
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning at -2{{The left operand of '==' is a garbage value}}
+#else
+ // expected-warning at -4{{UNKNOWN}}
+#endif
+ } else {
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // Seems to work at the moment, but also seems accidental.
+ // Feel free to break.
+ // expected-warning at -4{{TRUE}}
+#else
+ // expected-warning at -6{{UNKNOWN}}
+#endif
+ }
+}
+
+void f5() {
+ C *after, *before;
+ {
+ const bool &x = C(true, &after, &before).x; // no-crash
+ }
+ // FIXME: Should be TRUE. Should not warn about garbage value.
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning at -2{{The left operand of '==' is a garbage value}}
+#else
+ // expected-warning at -4{{UNKNOWN}}
+#endif
+}
+} // end namespace maintain_original_object_address_on_lifetime_extension
+
+namespace maintain_original_object_address_on_move {
+class C {
+ int *x;
+
+public:
+ C() : x(nullptr) {}
+ C(int *x) : x(x) {}
+ C(const C &c) = delete;
+ C(C &&c) : x(c.x) { c.x = nullptr; }
+ C &operator=(C &&c) {
+ x = c.x;
+ c.x = nullptr;
+ return *this;
+ }
+ ~C() {
+ // This was triggering the division by zero warning in f1() and f2():
+ // Because move-elision materialization was incorrectly causing the object
+ // to be relocated from one address to another before move, but destructor
+ // was operating on the old address, it was still thinking that 'x' is set.
+ if (x)
+ *x = 0;
+ }
+};
+
+void f1() {
+ int x = 1;
+ // &x is replaced with nullptr in move-constructor before the temporary dies.
+ C c = C(&x);
+ // Hence x was not set to 0 yet.
+ 1 / x; // no-warning
+}
+void f2() {
+ int x = 1;
+ C c;
+ // &x is replaced with nullptr in move-assignment before the temporary dies.
+ c = C(&x);
+ // Hence x was not set to 0 yet.
+ 1 / x; // no-warning
+}
+} // end namespace maintain_original_object_address_on_move
Modified: cfe/trunk/test/Analysis/temporaries.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/temporaries.cpp?rev=326236&r1=326235&r2=326236&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/temporaries.cpp (original)
+++ cfe/trunk/test/Analysis/temporaries.cpp Tue Feb 27 11:47:49 2018
@@ -895,6 +895,33 @@ void test_ternary_temporary_with_copy(in
}
} // namespace test_match_constructors_and_destructors
+namespace dont_forget_destructor_around_logical_op {
+int glob;
+
+class C {
+public:
+ ~C() { glob = 1; }
+};
+
+C get();
+
+bool is(C);
+
+
+void test(int coin) {
+ // Here temporaries are being cleaned up after && is evaluated. There are two
+ // 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.
+ 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}}
+}
+} // namespace dont_forget_destructor_around_logical_op
+
#if __cplusplus >= 201103L
namespace temporary_list_crash {
class C {
More information about the cfe-commits
mailing list