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,6 +265,7 @@ assert(C || getCurrentCFGElement().getAs()); const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr; + bool IsReturnedIntoParentStackFrame = false; const CXXBindTemporaryExpr *BTE = nullptr; const MaterializeTemporaryExpr *MTE = nullptr; @@ -269,25 +273,44 @@ 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(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(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(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()) { + TCC = cast( + 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(Target)); } if (MTE) { - State = addTemporaryMaterialization(State, MTE, LCtx, + State = addTemporaryMaterialization(State, MTE, TempLCtx, cast(Target)); } 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