NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
With https://reviews.llvm.org/D44120 we can find the `CXXBindTemporaryExpr` and
`MaterializeTemporaryExpr` that correspond to the temporary object that is
being returned from a function into its caller. These expressions are on the
caller side because it is the caller that's responsible for managing the
lifetime of such temporary.
In order to find those expressions, we look at the call site's CFG element,
notice that it is a `CFGValueTypedCall` that was added in
https://reviews.llvm.org/D44120, and take the relevant construction context
information from there.
This patch does not yet affect temporaries that were returned from
conservatively evaluated functions.
The idea was initially described in the last part of
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html and addresses
most of the concerns from https://reviews.llvm.org/D42876.
Repository:
rC Clang
https://reviews.llvm.org/D44124
Files:
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,7 +1,10 @@
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify %s
// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -DMOVES -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused -std=c++11 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DTEMPORARIES -DMOVES -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_checkInlined(bool);
namespace pr17001_call_wrong_destructor {
bool x;
@@ -63,6 +66,8 @@
~C() { if (after) *after = this; }
operator bool() const { return x; }
+
+ static C make(C **after, C **before) { return C(false, after, before); }
};
void f1() {
@@ -180,3 +185,146 @@
1 / x; // no-warning
}
} // end namespace maintain_original_object_address_on_move
+
+namespace maintain_address_of_copies {
+class C;
+
+struct AddressVector {
+ C *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(C *c) {
+ buf[len] = c;
+ ++len;
+ }
+};
+
+class C {
+ AddressVector &v;
+
+public:
+ C(AddressVector &v) : v(v) { v.push(this); }
+ ~C() { v.push(this); }
+
+#ifdef MOVES
+ C(C &&c) : v(c.v) { v.push(this); }
+#endif
+
+ // Note how return-statements prefer move-constructors when available.
+ C(const C &c) : v(c.v) {
+#ifdef MOVES
+ clang_analyzer_checkInlined(false); // no-warning
+#else
+ v.push(this);
+#endif
+ } // no-warning
+
+ static C make(AddressVector &v) { return C(v); }
+};
+
+void f1() {
+ AddressVector v;
+ {
+ C c = C(v);
+ }
+ // Create temporary, perform elidable move into variable,
+ // destroy the temporary, destroy the variable.
+ clang_analyzer_eval(v.len == 4);
+ clang_analyzer_eval(v.buf[0] == v.buf[2]);
+ clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#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
+}
+
+void f2() {
+ AddressVector v;
+ {
+ const C &c = C::make(v);
+ }
+ // Create temporary within make(), perform elidable move within make()
+ // and lifetime-extend it into 'c', destroy the temporary, destroy the
+ // moved-from object.
+ clang_analyzer_eval(v.len == 4);
+ clang_analyzer_eval(v.buf[0] == v.buf[2]);
+ clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#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
+}
+
+void f3() {
+ AddressVector v;
+ {
+ C &&c = C::make(v);
+ }
+ // Create temporary within make(), perform elidable move within make()
+ // and lifetime-extend it into 'c', destroy the temporary, destroy the
+ // moved-from object.
+ clang_analyzer_eval(v.len == 4);
+ clang_analyzer_eval(v.buf[0] == v.buf[2]);
+ clang_analyzer_eval(v.buf[1] == v.buf[3]);
+#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
+}
+
+C doubleMake(AddressVector &v) {
+ return C::make(v);
+}
+
+void f4() {
+ AddressVector v;
+ {
+ C c = doubleMake(v);
+ }
+ // 1. Construct the original temporary within make(),
+ // 2. Construct return value of make() (elidable move) and lifetime-extend
+ // it into the return value constructor argument within doubleMake(),
+ // 3. Destroy temporary within make(),
+ // 4. Construct return value of doubleMake() (elidable move) and
+ // lifetime-extend it into the variable 'c' constructor argument,
+ // 5. Destroy return value of make(),
+ // 6. Construct variable 'c' (elidable move),
+ // 7. Destroy return value of doubleMake(),
+ // 8. Destroy variable 'c'.
+ clang_analyzer_eval(v.len == 8);
+ clang_analyzer_eval(v.buf[0] == v.buf[2]);
+ clang_analyzer_eval(v.buf[1] == v.buf[4]);
+ clang_analyzer_eval(v.buf[3] == v.buf[6]);
+ clang_analyzer_eval(v.buf[5] == v.buf[7]);
+#ifdef TEMPORARIES
+ // expected-warning@-6{{TRUE}}
+ // expected-warning@-6{{TRUE}}
+ // expected-warning@-6{{TRUE}}
+ // expected-warning@-6{{TRUE}}
+ // expected-warning@-6{{TRUE}}
+#else
+ // expected-warning@-12{{UNKNOWN}}
+ // expected-warning@-12{{UNKNOWN}}
+ // expected-warning@-12{{UNKNOWN}}
+ // expected-warning@-12{{UNKNOWN}}
+ // expected-warning@-12{{UNKNOWN}}
+#endif
+}
+} // end namespace maintain_address_of_copies
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -197,16 +197,19 @@
return MRMgr.getCXXTempObjectRegion(CE, LCtx);
}
case ConstructionContext::ReturnedValueKind: {
- // TODO: We should construct into a CXXBindTemporaryExpr or a
- // MaterializeTemporaryExpr around the call-expression on the previous
- // stack frame. Currently we re-bind the temporary to the correct region
- // later, but that's not semantically correct. This of course does not
- // apply when we're in the top frame. But if we are in an inlined
- // function, we should be able to take the call-site CFG element,
- // and it should contain (but right now it wouldn't) some sort of
- // construction context that'd give us the right temporary expression.
+ // The temporary is to be managed by the parent stack frame.
+ // So build it in the parent stack frame if we're not in the
+ // top frame of the analysis.
+ // TODO: What exactly happens when we are? Does the temporary object live
+ // long enough in the region store in this case? Would checkers think
+ // that this object immediately goes out of scope?
+ const LocationContext *TempLCtx = LCtx;
+ if (const LocationContext *CallerLCtx =
+ LCtx->getCurrentStackFrame()->getParent()) {
+ TempLCtx = CallerLCtx;
+ }
CallOpts.IsTemporaryCtorOrDtor = true;
- return MRMgr.getCXXTempObjectRegion(CE, LCtx);
+ return MRMgr.getCXXTempObjectRegion(CE, TempLCtx);
}
}
}
@@ -262,32 +265,52 @@
assert(C || getCurrentCFGElement().getAs<CFGStmt>());
const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
+ bool IsReturnedIntoParentStackFrame = false;
const CXXBindTemporaryExpr *BTE = nullptr;
const MaterializeTemporaryExpr *MTE = nullptr;
switch (CE->getConstructionKind()) {
case CXXConstructExpr::CK_Complete: {
Target = getRegionForConstructedObject(CE, Pred, CC, CallOpts);
- // In case of temporary object construction, extract data necessary for
- // destruction and lifetime extension.
- if (const auto *TCC =
- dyn_cast_or_null<TemporaryObjectConstructionContext>(CC)) {
- assert(CallOpts.IsTemporaryCtorOrDtor);
- assert(!CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion);
- if (AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG()) {
- BTE = TCC->getCXXBindTemporaryExpr();
- MTE = TCC->getMaterializedTemporaryExpr();
- if (!BTE) {
- // FIXME: lifetime extension for temporaries without destructors
- // is not implemented yet.
- MTE = nullptr;
+ if (CC) {
+ // In case of temporary object construction, extract data necessary for
+ // destruction and lifetime extension.
+ const auto *TCC = dyn_cast<TemporaryObjectConstructionContext>(CC);
+
+ // If the temporary is being returned from the function, it will be
+ // destroyed or lifetime-extended in the caller stack frame.
+ if (const auto *RCC = dyn_cast<ReturnedValueConstructionContext>(CC)) {
+ const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
+ assert(SFC);
+ if (SFC->getParent()) {
+ IsReturnedIntoParentStackFrame = true;
+ const CFGElement &CallElem =
+ (*SFC->getCallSiteBlock())[SFC->getIndex()];
+ if (auto VTCElem = CallElem.getAs<CFGValueTypedCall>()) {
+ TCC = cast<TemporaryObjectConstructionContext>(
+ VTCElem->getConstructionContext());
+ }
}
- 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.
- BTE = nullptr;
+ }
+
+ if (TCC) {
+ assert(CallOpts.IsTemporaryCtorOrDtor);
+ assert(!CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion);
+ if (AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG()) {
+ BTE = TCC->getCXXBindTemporaryExpr();
+ MTE = TCC->getMaterializedTemporaryExpr();
+ if (!BTE) {
+ // FIXME: lifetime extension for temporaries without destructors
+ // is not implemented yet.
+ MTE = nullptr;
+ }
+ 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.
+ BTE = nullptr;
+ }
}
}
}
@@ -385,13 +408,19 @@
State = State->bindDefault(loc::MemRegionVal(Target), ZeroVal, LCtx);
}
+ // Set up destruction and lifetime extension information.
+ const LocationContext *TempLCtx =
+ IsReturnedIntoParentStackFrame
+ ? LCtx->getCurrentStackFrame()->getParent()
+ : LCtx;
+
if (BTE) {
- State = addInitializedTemporary(State, BTE, LCtx,
+ State = addInitializedTemporary(State, BTE, TempLCtx,
cast<CXXTempObjectRegion>(Target));
}
if (MTE) {
- State = addTemporaryMaterialization(State, MTE, LCtx,
+ State = addTemporaryMaterialization(State, MTE, TempLCtx,
cast<CXXTempObjectRegion>(Target));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
