From 308e27ee9df861a219f62acb5452969fd161cecc Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Tue, 27 Feb 2018 19:47:49 +0000 Subject: [PATCH] [analyzer] Introduce correct lifetime extension behavior in simple cases. This patch uses the reference to MaterializeTemporaryExpr stored in the construction context since r326014 in order to model that expression correctly. When modeling MaterializeTemporaryExpr, instead of copying the raw memory contents from the sub-expression's rvalue to a completely new temporary region, that we conjure up for the lack of better options, we now have the better option to recall the region into which the object was originally constructed and declare that region to be the value of the expression, which is semantically correct. This only works when the construction context is available, which is worked on independently. 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. Differential Revision: https://reviews.llvm.org/D43497 llvm-svn: 326236 --- .../StaticAnalyzer/Core/PathSensitive/ExprEngine.h | 13 ++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 169 ++++++++++++++++----- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 15 +- clang/test/Analysis/lifetime-extension.cpp | 159 ++++++++++++++++++- clang/test/Analysis/temporaries.cpp | 27 ++++ 5 files changed, 337 insertions(+), 46 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index ce30827..0269953 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -708,6 +708,19 @@ private: 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. diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 8808e95..9f71b2f 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -65,6 +65,17 @@ typedef llvm::ImmutableMap, + 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, SVal> @@ -255,17 +266,35 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments( CommaLHSs, Adjustments); + // Take the region for Init, i.e. for the whole object. If we do not remember + // the region in which the object originally was constructed, come up with + // a new temporary region out of thin air and copy the contents of the object + // (which are currently present in the Environment, because Init is an rvalue) + // into that region. This is not correct, but it is better than nothing. + bool FoundOriginalMaterializationRegion = false; const TypedValueRegion *TR = nullptr; if (const MaterializeTemporaryExpr *MT = dyn_cast(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); - } - if (!TR) + auto Key = std::make_pair(MT, LC->getCurrentStackFrame()); + if (const CXXTempObjectRegion *const *TRPtr = + State->get(Key)) { + FoundOriginalMaterializationRegion = true; + TR = *TRPtr; + assert(TR); + State = State->remove(Key); + } else { + 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); + } else { + TR = MRMgr.getCXXTempObjectRegion(Init, LC); + } + } + } else { TR = MRMgr.getCXXTempObjectRegion(Init, LC); + } SVal Reg = loc::MemRegionVal(TR); SVal BaseReg = Reg; @@ -287,32 +316,35 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, } } - // 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(), 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()); + 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(), 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(), InitValWithAdjustments, LC, false); + } else { + State = State->bindLoc(BaseReg.castAs(), InitVal, LC, false); } - State = - State->bindLoc(Reg.castAs(), InitValWithAdjustments, LC, false); - } else { - State = State->bindLoc(BaseReg.castAs(), InitVal, LC, false); } // The result expression would now point to the correct sub-region of the @@ -320,8 +352,10 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, // 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 +390,29 @@ bool ExprEngine::areInitializedTemporariesClear(ProgramStateRef State, 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(Key)); + return State->set(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()) + if (I.first.second == LC) + return false; + + LC = LC->getParent(); + } + return true; +} + ProgramStateRef ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE, @@ -437,6 +494,24 @@ static void printInitializedTemporariesForContext(raw_ostream &Out, } } +static void printTemporaryMaterializationsForContext( + 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()) { + std::pair Key = + I.first; + const MemRegion *Value = I.second; + if (Key.second != LC) + continue; + Out << '(' << Key.second << ',' << Key.first << ") "; + Key.first->printPretty(Out, nullptr, PP); + assert(Value); + Out << " : " << Value << NL; + } +} + static void printCXXNewAllocatorValuesForContext(raw_ostream &Out, ProgramStateRef State, const char *NL, @@ -468,6 +543,14 @@ void ExprEngine::printState(raw_ostream &Out, ProgramStateRef State, }); } + if (!State->get().isEmpty()) { + Out << Sep << "Temporaries to be materialized:" << NL; + + LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) { + printTemporaryMaterializationsForContext(Out, State, NL, Sep, LC); + }); + } + if (!State->get().isEmpty()) { Out << Sep << "operator new() allocator return values:" << NL; @@ -578,6 +661,10 @@ void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out, if (I.second) SymReaper.markLive(I.second); + for (auto I : CleanedState->get()) + if (I.second) + SymReaper.markLive(I.second); + for (auto I : CleanedState->get()) { if (SymbolRef Sym = I.second.getAsSymbol()) SymReaper.markLive(Sym); @@ -2108,11 +2195,12 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC, 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 +2225,9 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC, 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()); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index 9c95507..2c1e858 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -230,6 +230,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr; const CXXBindTemporaryExpr *BTE = nullptr; + const MaterializeTemporaryExpr *MTE = nullptr; switch (CE->getConstructionKind()) { case CXXConstructExpr::CK_Complete: { @@ -237,8 +238,13 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, if (CC && AMgr.getAnalyzerOptions().includeTemporaryDtorsInCFG() && !CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion && CallOpts.IsTemporaryCtorOrDtor) { - // May as well be a ReturnStmt. - BTE = dyn_cast(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(CC->getTriggerStmt()); + } } break; } @@ -339,6 +345,11 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, cast(Target)); } + if (MTE) { + State = addTemporaryMaterialization(State, MTE, LCtx, + cast(Target)); + } + Bldr.generateNode(CE, *I, State, /*tag=*/nullptr, ProgramPoint::PreStmtKind); } diff --git a/clang/test/Analysis/lifetime-extension.cpp b/clang/test/Analysis/lifetime-extension.cpp index 5e3c5dd..93605fc 100644 --- a/clang/test/Analysis/lifetime-extension.cpp +++ b/clang/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,157 @@ void f() { 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; + +public: + bool x; + + 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(true, &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(true, &after, &before) : C(false, &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 + } +} + +void f5() { + C *after, *before; + { + const bool &x = C(true, &after, &before).x; // no-crash + } + // FIXME: Should be TRUE. Should not warn about garbage value. + clang_analyzer_eval(after == before); +#ifdef TEMPORARIES + // expected-warning@-2{{The left operand of '==' is a garbage value}} +#else + // expected-warning@-4{{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 diff --git a/clang/test/Analysis/temporaries.cpp b/clang/test/Analysis/temporaries.cpp index 2587e4e..48a1c57 100644 --- a/clang/test/Analysis/temporaries.cpp +++ b/clang/test/Analysis/temporaries.cpp @@ -895,6 +895,33 @@ void test_ternary_temporary_with_copy(int coin) { } } // namespace test_match_constructors_and_destructors +namespace dont_forget_destructor_around_logical_op { +int glob; + +class C { +public: + ~C() { glob = 1; } +}; + +C get(); + +bool is(C); + + +void test(int coin) { + // Here temporaries are being cleaned up after && is evaluated. There are two + // temporaries: the return value of get() and the elidable copy constructor + // of that return value into is(). According to the CFG, we need to cleanup + // both of them depending on whether the temporary corresponding to the + // return value of get() was initialized. However, for now we don't track + // temporaries returned from functions, so we take the wrong branch. + coin && is(get()); // no-crash + // FIXME: We should have called the destructor, i.e. should be TRUE, + // at least when we inline temporary destructors. + clang_analyzer_eval(glob == 1); // expected-warning{{UNKNOWN}} +} +} // namespace dont_forget_destructor_around_logical_op + #if __cplusplus >= 201103L namespace temporary_list_crash { class C { -- 2.7.4