Re: r327345 - [analyzer] Destroy and lifetime-extend inlined function return values properly.
Hmm, indeed, sorry. This was fixed independently by Pavel Labath in r327491 (this code had moved before the fix). On 13/03/2018 11:34 PM, Mikael Holmén wrote: Hi Artem, This commit gives the following warning: ../tools/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:283:23: error: unused variable 'RCC' [-Werror,-Wunused-variable] if (const auto *RCC = dyn_cast(CC)) { ^ 1 error generated. Regards, Mikael On 03/13/2018 12:22 AM, Artem Dergachev via cfe-commits wrote: Author: dergachev Date: Mon Mar 12 16:22:35 2018 New Revision: 327345 URL: http://llvm.org/viewvc/llvm-project?rev=327345&view=rev Log: [analyzer] Destroy and lifetime-extend inlined function return values properly. This patch uses the newly added CFGCXXRecordTypedCall element at the call site of the caller to construct the return value within the callee directly into the caller's stack frame. This way it is also capable of populating the temporary destructor and lifetime extension maps for the temporary, which allows temporary destructors and lifetime extension to work correctly. This patch does not affect temporaries that were returned from conservatively evaluated functions. Differential Revision: https://reviews.llvm.org/D44124 Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp cfe/trunk/test/Analysis/lifetime-extension.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=327345&r1=327344&r2=327345&view=diff == --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Mon Mar 12 16:22:35 2018 @@ -197,16 +197,19 @@ ExprEngine::getRegionForConstructedObjec 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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->getP
Re: r327345 - [analyzer] Destroy and lifetime-extend inlined function return values properly.
Hi Artem, This commit gives the following warning: ../tools/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:283:23: error: unused variable 'RCC' [-Werror,-Wunused-variable] if (const auto *RCC = dyn_cast(CC)) { ^ 1 error generated. Regards, Mikael On 03/13/2018 12:22 AM, Artem Dergachev via cfe-commits wrote: Author: dergachev Date: Mon Mar 12 16:22:35 2018 New Revision: 327345 URL: http://llvm.org/viewvc/llvm-project?rev=327345&view=rev Log: [analyzer] Destroy and lifetime-extend inlined function return values properly. This patch uses the newly added CFGCXXRecordTypedCall element at the call site of the caller to construct the return value within the callee directly into the caller's stack frame. This way it is also capable of populating the temporary destructor and lifetime extension maps for the temporary, which allows temporary destructors and lifetime extension to work correctly. This patch does not affect temporaries that were returned from conservatively evaluated functions. Differential Revision: https://reviews.llvm.org/D44124 Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp cfe/trunk/test/Analysis/lifetime-extension.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=327345&r1=327344&r2=327345&view=diff == --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Mon Mar 12 16:22:35 2018 @@ -197,16 +197,19 @@ ExprEngine::getRegionForConstructedObjec 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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 (
r327345 - [analyzer] Destroy and lifetime-extend inlined function return values properly.
Author: dergachev Date: Mon Mar 12 16:22:35 2018 New Revision: 327345 URL: http://llvm.org/viewvc/llvm-project?rev=327345&view=rev Log: [analyzer] Destroy and lifetime-extend inlined function return values properly. This patch uses the newly added CFGCXXRecordTypedCall element at the call site of the caller to construct the return value within the callee directly into the caller's stack frame. This way it is also capable of populating the temporary destructor and lifetime extension maps for the temporary, which allows temporary destructors and lifetime extension to work correctly. This patch does not affect temporaries that were returned from conservatively evaluated functions. Differential Revision: https://reviews.llvm.org/D44124 Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp cfe/trunk/test/Analysis/lifetime-extension.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp?rev=327345&r1=327344&r2=327345&view=diff == --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Mon Mar 12 16:22:35 2018 @@ -197,16 +197,19 @@ ExprEngine::getRegionForConstructedObjec 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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 @@ void ExprEngine::VisitCXXConstructExpr(c 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 RTCElem = CallElem.getAs()) { +TCC = cast( +RTCElem->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 - /