r327345 - [analyzer] Destroy and lifetime-extend inlined function return values properly.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 14 12:41:03 PDT 2018
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<ReturnedValueConstructionContext>(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<CFGStmt>());
>> 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<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 RTCElem = CallElem.getAs<CFGCXXRecordTypedCall>()) {
>> + TCC = cast<TemporaryObjectConstructionContext>(
>> + 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
>> - // 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 @@ void ExprEngine::VisitCXXConstructExpr(c
>> 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));
>> }
>>
>> Modified: cfe/trunk/test/Analysis/lifetime-extension.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/lifetime-extension.cpp?rev=327345&r1=327344&r2=327345&view=diff
>> ==============================================================================
>>
>> --- cfe/trunk/test/Analysis/lifetime-extension.cpp (original)
>> +++ cfe/trunk/test/Analysis/lifetime-extension.cpp Mon Mar 12
>> 16:22:35 2018
>> @@ -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 @@ public:
>> ~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,154 @@ void f2() {
>> 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);
>> + }
>> + // 0. Create the original temporary and lifetime-extend it into
>> variable 'c'
>> + // construction argument.
>> + // 1. Construct variable 'c' (elidable copy/move).
>> + // 2. Destroy the temporary.
>> + // 3. Destroy variable 'c'.
>> + 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 at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> +#else
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> +#endif
>> +}
>> +
>> +void f2() {
>> + AddressVector v;
>> + {
>> + const C &c = C::make(v);
>> + }
>> + // 0. Construct the original temporary within make(),
>> + // 1. Construct the return value of make() (elidable copy/move) and
>> + // lifetime-extend it via reference 'c',
>> + // 2. Destroy the temporary within make(),
>> + // 3. Destroy the temporary lifetime-extended by 'c'.
>> + 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 at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> +#else
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> +#endif
>> +}
>> +
>> +void f3() {
>> + AddressVector v;
>> + {
>> + C &&c = C::make(v);
>> + }
>> + // 0. Construct the original temporary within make(),
>> + // 1. Construct the return value of make() (elidable copy/move) and
>> + // lifetime-extend it via reference 'c',
>> + // 2. Destroy the temporary within make(),
>> + // 3. Destroy the temporary lifetime-extended by 'c'.
>> + 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 at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> + // expected-warning at -4{{TRUE}}
>> +#else
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> + // expected-warning at -8{{UNKNOWN}}
>> +#endif
>> +}
>> +
>> +C doubleMake(AddressVector &v) {
>> + return C::make(v);
>> +}
>> +
>> +void f4() {
>> + AddressVector v;
>> + {
>> + C c = doubleMake(v);
>> + }
>> + // 0. Construct the original temporary within make(),
>> + // 1. Construct the return value of make() (elidable copy/move) and
>> + // lifetime-extend it into the return value constructor
>> argument within
>> + // doubleMake(),
>> + // 2. Destroy the temporary within make(),
>> + // 3. Construct the return value of doubleMake() (elidable
>> copy/move) and
>> + // lifetime-extend it into the variable 'c' constructor argument,
>> + // 4. Destroy the return value of make(),
>> + // 5. Construct variable 'c' (elidable copy/move),
>> + // 6. Destroy the return value of doubleMake(),
>> + // 7. 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 at -6{{TRUE}}
>> + // expected-warning at -6{{TRUE}}
>> + // expected-warning at -6{{TRUE}}
>> + // expected-warning at -6{{TRUE}}
>> + // expected-warning at -6{{TRUE}}
>> +#else
>> + // expected-warning at -12{{UNKNOWN}}
>> + // expected-warning at -12{{UNKNOWN}}
>> + // expected-warning at -12{{UNKNOWN}}
>> + // expected-warning at -12{{UNKNOWN}}
>> + // expected-warning at -12{{UNKNOWN}}
>> +#endif
>> +}
>> +} // end namespace maintain_address_of_copies
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
More information about the cfe-commits
mailing list