NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
This patch uses the reference to `MaterializeTemporaryExpr` stored in the
construction context since https://reviews.llvm.org/D43477 in order to model
that expression correctly, following the plan described in
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html
`MaterializeTemporaryExpr` is an expression that takes an rvalue object (or one
of its xvalue sub-objects) and transforms it into a reference to that object.
From the analyzer's point of view, the value of the sub-expression would be a
`NonLoc`, and the value of `MaterializeTemporaryExpr` would be a `Loc` that
this `NonLoc` is stored in.
For now this behavior has been achieved by creating a `Loc` out of thin air and
re-binding the `NonLoc` value into the newly created location. This, however,
is incorrect, because the constructor that has constructed the respective
`NonLoc` value was operating on a different memory region, so all of the
subsequent method calls, including the destructor, should have the exact same
"this" region that the constructor was using.
The problem with behaving correctly is that we've already forgot the original
storage. It is technically impossible to reliably recover it from the `NonLoc`.
The patch addresses it by relying on the `ConstructionContext` to inform the
constructor that lifetime extension is taking place, so that the constructor
could store the current "this" region in the program state for later use. The
region is uniquely determined by the `MaterializeTemporaryExpr` itself. Then
when time comes to model `MaterializeTemporaryExpr`, it looks up itself in the
program state to find the respective region, and avoids the hassle of creating
a new region and copying the value in case it does indeed find the existing
correct region.
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.
Situations where correct lifetime extension behavior is needed require
relatively awkward test cases, because most of the time you don't care about
the object's address as long as its contents are correct.
This patch is enough to assert that for every initialized temporary
(lifetime-extended or not), a destructor is called (with the correct address)
on all existing test cases(!). I added a new test case (with
`BinaryConditionalOperator`) on which this assertion still fails (due to lack
of construction context and other problems) - i've actually seen this operator
used on C++ objects. There are more cases that we don't handle yet; i'd see
what i can do about them.
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
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,142 @@
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;
+ bool x;
+
+public:
+ 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(1, &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(1, &after, &before) : C(2, &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
+ }
+}
+} // 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
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: 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,14 +266,25 @@
const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
CommaLHSs, Adjustments);
+ 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);
+ auto Key = std::make_pair(MT, LC->getCurrentStackFrame());
+ if (const CXXTempObjectRegion *const *TRPtr =
+ State->get<TemporaryMaterializations>(Key)) {
+ FoundOriginalMaterializationRegion = true;
+ TR = *TRPtr;
+ State = State->remove<TemporaryMaterializations>(Key);
+ }
+
+ if (!TR) {
+ 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)
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
@@ -287,41 +309,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());
- }
- State =
- State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
- } else {
- State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+ 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);
+ }
}
// 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 +383,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 +487,25 @@
}
}
+static void printTemporaryMaterializatinosForContext(
+ 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);
+ if (Value)
+ Out << " : " << Value;
+ Out << NL;
+ }
+}
+
static void printCXXNewAllocatorValuesForContext(raw_ostream &Out,
ProgramStateRef State,
const char *NL,
@@ -468,6 +537,14 @@
});
}
+ if (!State->get<TemporaryMaterializations>().isEmpty()) {
+ Out << Sep << "Temporaries to be materialized:" << NL;
+
+ LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) {
+ printTemporaryMaterializatinosForContext(Out, State, NL, Sep, LC);
+ });
+ }
+
if (!State->get<CXXNewAllocatorValues>().isEmpty()) {
Out << Sep << "operator new() allocator return values:" << NL;
@@ -578,6 +655,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 +2189,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 +2219,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: 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
