Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -708,6 +708,19 @@ 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. Index: lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngine.cpp +++ lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -65,6 +65,17 @@ REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries, InitializedTemporariesMap) +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,14 +266,25 @@ const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments( CommaLHSs, Adjustments); + 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); + auto Key = std::make_pair(MT, LC->getCurrentStackFrame()); + if (const CXXTempObjectRegion *const *TRPtr = + State->get(Key)) { + FoundOriginalMaterializationRegion = true; + TR = *TRPtr; + State = State->remove(Key); + } + + if (!TR) { + 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) TR = MRMgr.getCXXTempObjectRegion(Init, LC); @@ -287,32 +309,35 @@ } } - // 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); + 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); + } } // The result expression would now point to the correct sub-region of the @@ -320,8 +345,10 @@ // 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 +383,29 @@ 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 +487,25 @@ } } +static void printTemporaryMaterializatinosForContext( + 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); + if (Value) + Out << " : " << Value; + Out << NL; + } +} + static void printCXXNewAllocatorValuesForContext(raw_ostream &Out, ProgramStateRef State, const char *NL, @@ -468,6 +537,14 @@ }); } + if (!State->get().isEmpty()) { + Out << Sep << "Temporaries to be materialized:" << NL; + + LCtx->dumpStack(Out, "", NL, Sep, [&](const LocationContext *LC) { + printTemporaryMaterializatinosForContext(Out, State, NL, Sep, LC); + }); + } + if (!State->get().isEmpty()) { Out << Sep << "operator new() allocator return values:" << NL; @@ -578,6 +655,10 @@ 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 +2189,12 @@ 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 +2219,9 @@ 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()); Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -230,6 +230,7 @@ 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 @@ 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 @@ cast(Target)); } + if (MTE) { + State = addTemporaryMaterialization(State, MTE, LCtx, + cast(Target)); + } + Bldr.generateNode(CE, *I, State, /*tag=*/nullptr, ProgramPoint::PreStmtKind); } Index: test/Analysis/lifetime-extension.cpp =================================================================== --- test/Analysis/lifetime-extension.cpp +++ 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,142 @@ 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; + bool x; + +public: + 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(1, &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(1, &after, &before) : C(2, &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 + } +} +} // 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 Index: test/Analysis/temporaries.cpp =================================================================== --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -895,6 +895,33 @@ } } // 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 {