This revision was automatically updated to reflect the committed changes.
Closed by commit rC326236: [analyzer] Introduce correct lifetime extension
behavior in simple cases. (authored by dergachev, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D43497
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/lifetime-extension.cpp
test/Analysis/temporaries.cpp
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -65,6 +65,17 @@
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 @@
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,41 +316,46 @@
}
}
- // 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
// newly created temporary region. Do this last in order to getSVal of Init
// 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 @@
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 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 @@
});
}
+ 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 @@
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 @@
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 @@
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());
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -230,15 +230,21 @@
const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
const CXXBindTemporaryExpr *BTE = nullptr;
+ const MaterializeTemporaryExpr *MTE = nullptr;
switch (CE->getConstructionKind()) {
case CXXConstructExpr::CK_Complete: {
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());
+ 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 @@
cast<CXXTempObjectRegion>(Target));
}
+ if (MTE) {
+ State = addTemporaryMaterialization(State, MTE, LCtx,
+ cast<CXXTempObjectRegion>(Target));
+ }
+
Bldr.generateNode(CE, *I, State, /*tag=*/nullptr,
ProgramPoint::PreStmtKind);
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -708,6 +708,19 @@
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.
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -895,6 +895,33 @@
}
} // 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 {
Index: test/Analysis/lifetime-extension.cpp
===================================================================
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -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 @@
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@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+#else
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-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@-2{{TRUE}}
+#else
+ // expected-warning@-4{{UNKNOWN}}
+#endif
+}
+
+void f2() {
+ C *after, *before;
+ C c = C(1, &after, &before);
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning@-2{{TRUE}}
+#else
+ // expected-warning@-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@-2{{TRUE}}
+#else
+ // expected-warning@-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@-2{{The left operand of '==' is a garbage value}}
+#else
+ // expected-warning@-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@-4{{TRUE}}
+#else
+ // expected-warning@-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@-2{{The left operand of '==' is a garbage value}}
+#else
+ // expected-warning@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
