Index: include/clang/Analysis/AnalysisContext.h =================================================================== --- include/clang/Analysis/AnalysisContext.h +++ include/clang/Analysis/AnalysisContext.h @@ -404,13 +404,14 @@ bool SynthesizeBodies; public: - AnalysisDeclContextManager(bool useUnoptimizedCFG = false, - bool addImplicitDtors = false, - bool addInitializers = false, - bool addTemporaryDtors = false, - bool synthesizeBodies = false, - bool addStaticInitBranches = false, - bool addCXXNewAllocator = true); + AnalysisDeclContextManager(bool useUnoptimizedCFG, + bool addImplicitDtors, + bool addInitializers, + bool addTemporaryDtors, + bool synthesizeBodies, + bool addStaticInitBranches, + bool addCXXNewAllocator, + bool useTemporaryTerminators); ~AnalysisDeclContextManager(); Index: include/clang/Analysis/CFG.h =================================================================== --- include/clang/Analysis/CFG.h +++ include/clang/Analysis/CFG.h @@ -735,6 +735,7 @@ bool AddTemporaryDtors; bool AddStaticInitBranches; bool AddCXXNewAllocator; + bool UseTemporaryTerminators; bool alwaysAdd(const Stmt *stmt) const { return alwaysAddMask[stmt->getStmtClass()]; @@ -755,7 +756,7 @@ PruneTriviallyFalseEdges(true), AddEHEdges(false), AddInitializers(false), AddImplicitDtors(false), AddTemporaryDtors(false), AddStaticInitBranches(false), - AddCXXNewAllocator(false) {} + AddCXXNewAllocator(false), UseTemporaryTerminators(false) {} }; /// \brief Provides a custom implementation of the iterator class to have the Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h =================================================================== --- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h +++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h @@ -193,6 +193,9 @@ /// \sa includeTemporaryDtorsInCFG Optional IncludeTemporaryDtorsInCFG; + /// \sa useTemporaryTerminatorsInCFG + Optional UseTemporaryTerminatorsInCFG; + /// \sa mayInlineCXXStandardLibrary Optional InlineCXXStandardLibrary; @@ -281,6 +284,15 @@ /// accepts the values "true" and "false". bool includeTemporaryDtorsInCFG(); + /// Returns whether or not the CFG should be built with the new temporary + /// terminator based algorithm to correctly handle the destructors for C++ + /// temporary objects. This new algorithm is more correct and should be + /// turned on by default, but it needs to bake for a while. + /// + /// This is controlled by the 'cfg-temporary-terminators' config option, which + /// accepts the values "true" and "false". + bool useTemporaryTerminatorsInCFG(); + /// Returns whether or not C++ standard library functions may be considered /// for inlining. /// Index: include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -96,6 +96,10 @@ void HandleBranch(const Stmt *Cond, const Stmt *Term, const CFGBlock *B, ExplodedNode *Pred); + /// Handle conditional logic for running destructors for temporary objects. + void HandleTemporaryCleanup(const CXXBindTemporaryExpr *BTE, + const CFGBlock *B, + ExplodedNode *Pred); /// Handle conditional logic for running static initializers. void HandleStaticInit(const DeclStmt *DS, const CFGBlock *B, ExplodedNode *Pred); Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -228,6 +228,15 @@ const CFGBlock *DstF) override; /// Called by CoreEngine. Used to processing branching behavior + /// for temporary object destructor calls. + void processTemporaryCleanup(const CXXBindTemporaryExpr *BTE, + NodeBuilderContext& BuilderCtx, + ExplodedNode *Pred, + ExplodedNodeSet &Dst, + const CFGBlock *DstT, + const CFGBlock *DstF) override; + + /// Called by CoreEngine. Used to processing branching behavior /// at static initalizers. void processStaticInitializer(const DeclStmt *DS, NodeBuilderContext& BuilderCtx, @@ -436,6 +445,12 @@ void CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME, ExplodedNode *Pred, ExplodedNodeSet &Dst); + + // Handle a C++ temporary object binding, potentially tracking object construction so we can + // call destructors correctly. + void BindCXXTemporaryObject(const CXXBindTemporaryExpr *BTE, + ExplodedNode *Pred, + ExplodedNodeSet &Dst); /// evalEagerlyAssumeBinOpBifurcation - Given the nodes in 'Src', eagerly assume symbolic /// expressions of the form 'x != 0' and generate new nodes (stored in Dst) Index: include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h @@ -73,6 +73,14 @@ const CFGBlock *DstF) = 0; /// Called by CoreEngine. Used to processing branching behavior + /// for temporary object destructor calls. + virtual void processTemporaryCleanup(const CXXBindTemporaryExpr *BTE, + NodeBuilderContext& BuilderCtx, + ExplodedNode *Pred, + ExplodedNodeSet &Dst, + const CFGBlock *DstT, + const CFGBlock *DstF) = 0; + /// Called by CoreEngine. Used to processing branching behavior /// at static initalizers. virtual void processStaticInitializer(const DeclStmt *DS, NodeBuilderContext& BuilderCtx, Index: lib/Analysis/AnalysisDeclContext.cpp =================================================================== --- lib/Analysis/AnalysisDeclContext.cpp +++ lib/Analysis/AnalysisDeclContext.cpp @@ -63,13 +63,15 @@ cfgBuildOptions.forcedBlkExprs = &forcedBlkExprs; } -AnalysisDeclContextManager::AnalysisDeclContextManager(bool useUnoptimizedCFG, - bool addImplicitDtors, - bool addInitializers, - bool addTemporaryDtors, - bool synthesizeBodies, - bool addStaticInitBranch, - bool addCXXNewAllocator) +AnalysisDeclContextManager::AnalysisDeclContextManager( + bool useUnoptimizedCFG, + bool addImplicitDtors, + bool addInitializers, + bool addTemporaryDtors, + bool synthesizeBodies, + bool addStaticInitBranch, + bool addCXXNewAllocator, + bool useTemporaryTerminators) : SynthesizeBodies(synthesizeBodies) { cfgBuildOptions.PruneTriviallyFalseEdges = !useUnoptimizedCFG; @@ -78,6 +80,7 @@ cfgBuildOptions.AddTemporaryDtors = addTemporaryDtors; cfgBuildOptions.AddStaticInitBranches = addStaticInitBranch; cfgBuildOptions.AddCXXNewAllocator = addCXXNewAllocator; + cfgBuildOptions.UseTemporaryTerminators = useTemporaryTerminators; } void AnalysisDeclContextManager::clear() { Index: lib/Analysis/CFG.cpp =================================================================== --- lib/Analysis/CFG.cpp +++ lib/Analysis/CFG.cpp @@ -3485,6 +3485,33 @@ return B; } +CXXBindTemporaryExpr* findBindTemporaryInFirstChildren(Stmt *E) { + if (!E->children().empty()) { + Stmt *FirstChild = *E->children(); + assert(FirstChild); + if (CXXBindTemporaryExpr *BTE = + findBindTemporaryInFirstChildren(FirstChild)) + return BTE; + } + + return dyn_cast(E); +} + +CXXBindTemporaryExpr* findLastBindTemporary(Stmt *E) { + if (CXXBindTemporaryExpr *BTE = dyn_cast(E)) + return BTE; + // When visiting children for destructors we want to visit them in reverse + // order that they will appear in the CFG. Because the CFG is built + // bottom-up, this means we visit them in their natural order, which + // reverses them in the CFG. + for (Stmt::child_range I = E->children(); I; ++I) { + if (Stmt *Child = *I) + if (CXXBindTemporaryExpr *BTE = findLastBindTemporary(Child)) + return BTE; //FIXME: TODO: store this and return it? test with multiple temporaries + } + return NULL; +} + CFGBlock *CFGBuilder::VisitBinaryOperatorForTemporaryDtors(BinaryOperator *E) { if (E->isLogicalOp()) { // Destructors for temporaries in LHS expression should be called after @@ -3503,10 +3530,24 @@ if (badCFG) return NULL; - // If RHS expression did produce destructors we need to connect created - // blocks to CFG in same manner as for binary operator itself. - CFGBlock *LHSBlock = createBlock(false); - LHSBlock->setTerminator(CFGTerminator(E, true)); + CFGBlock *LHSBlock; + if (BuildOpts.UseTemporaryTerminators) { + CXXBindTemporaryExpr *BTE = findLastBindTemporary(E->getRHS()); + assert(BTE); + /*CXXBindTemporaryExpr *BTE = findBindTemporaryInFirstChildren(E->getRHS()); + if (!BTE) { + //TODO: what do we do when we don't have a terminator for this block? + Block = ConfluenceBlock; + return ConfluenceBlock; + }*/ + LHSBlock = createBlock(false); + LHSBlock->setTerminator(BTE); + } else { + LHSBlock = createBlock(false); + // If RHS expression did produce destructors we need to connect created + // blocks to CFG in same manner as for binary operator itself. + LHSBlock->setTerminator(CFGTerminator(E, true)); + } // For binary operator LHS block is before RHS in list of predecessors // of ConfluenceBlock. @@ -3618,6 +3659,7 @@ } Block = createBlock(false); + //FIXME: use temporary terminator handling here Block->setTerminator(CFGTerminator(E, true)); assert(Block->getTerminator().isTemporaryDtorsBranch()); Index: lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- lib/Sema/AnalysisBasedWarnings.cpp +++ lib/Sema/AnalysisBasedWarnings.cpp @@ -1814,6 +1814,7 @@ AC.getCFGBuildOptions().AddImplicitDtors = true; AC.getCFGBuildOptions().AddTemporaryDtors = true; AC.getCFGBuildOptions().AddCXXNewAllocator = false; + AC.getCFGBuildOptions().UseTemporaryTerminators = false; // Force that certain expressions appear as CFGElements in the CFG. This // is used to speed up various analyses. Index: lib/StaticAnalyzer/Core/AnalysisManager.cpp =================================================================== --- lib/StaticAnalyzer/Core/AnalysisManager.cpp +++ lib/StaticAnalyzer/Core/AnalysisManager.cpp @@ -26,7 +26,9 @@ /*AddInitializers=*/true, Options.includeTemporaryDtorsInCFG(), Options.shouldSynthesizeBodies(), - Options.shouldConditionalizeStaticInitializers()), + Options.shouldConditionalizeStaticInitializers(), + /*AddCXXNewAllocator=*/true, + Options.useTemporaryTerminatorsInCFG()), Ctx(ctx), Diags(diags), LangOpts(lang), Index: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp =================================================================== --- lib/StaticAnalyzer/Core/AnalyzerOptions.cpp +++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp @@ -122,6 +122,12 @@ /* Default = */ false); } +bool AnalyzerOptions::useTemporaryTerminatorsInCFG() { + return getBooleanOption(UseTemporaryTerminatorsInCFG, + "cfg-temporary-terminators", + /* Default = */ false); +} + bool AnalyzerOptions::mayInlineCXXStandardLibrary() { return getBooleanOption(InlineCXXStandardLibrary, "c++-stdlib-inlining", Index: lib/StaticAnalyzer/Core/CoreEngine.cpp =================================================================== --- lib/StaticAnalyzer/Core/CoreEngine.cpp +++ lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -14,6 +14,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h" #include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/StmtCXX.h" #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" @@ -385,6 +386,10 @@ HandleBranch(cast(Term)->getCond(), Term, B, Pred); return; + case Stmt::CXXBindTemporaryExprClass: + HandleTemporaryCleanup(cast(Term), B, Pred); + return; + case Stmt::CXXForRangeStmtClass: HandleBranch(cast(Term)->getCond(), Term, B, Pred); return; @@ -450,6 +455,8 @@ Pred->State, Pred); } + + void CoreEngine::HandleBranch(const Stmt *Cond, const Stmt *Term, const CFGBlock * B, ExplodedNode *Pred) { assert(B->succ_size() == 2); @@ -461,6 +468,17 @@ enqueue(Dst); } +void CoreEngine::HandleTemporaryCleanup(const CXXBindTemporaryExpr *BTE, + const CFGBlock *B, + ExplodedNode *Pred) { + assert(B->succ_size() == 2); + NodeBuilderContext Ctx(*this, B, Pred); + ExplodedNodeSet Dst; + SubEng.processTemporaryCleanup(BTE, Ctx, Pred, Dst, + *(B->succ_begin()), *(B->succ_begin()+1)); + // Enqueue the new frontier onto the worklist. + enqueue(Dst); +} void CoreEngine::HandleStaticInit(const DeclStmt *DS, const CFGBlock *B, ExplodedNode *Pred) { Index: lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngine.cpp +++ lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -794,7 +794,6 @@ case Stmt::SizeOfPackExprClass: case Stmt::StringLiteralClass: case Stmt::ObjCStringLiteralClass: - case Stmt::CXXBindTemporaryExprClass: case Stmt::CXXPseudoDestructorExprClass: case Stmt::SubstNonTypeTemplateParmExprClass: case Stmt::CXXNullPtrLiteralExprClass: { @@ -985,6 +984,14 @@ break; } + case Stmt::CXXBindTemporaryExprClass: { + Bldr.takeNodes(Pred); + const CXXBindTemporaryExpr *BTE = cast(S); + BindCXXTemporaryObject(BTE, Pred, Dst); + Bldr.addNodes(Dst); + break; + } + case Stmt::CXXNewExprClass: { Bldr.takeNodes(Pred); ExplodedNodeSet PostVisit; Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -12,6 +12,7 @@ //===----------------------------------------------------------------------===// #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" +#include "PrettyStackTraceLocationContext.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/StmtCXX.h" #include "clang/Basic/PrettyStackTrace.h" @@ -22,6 +23,56 @@ using namespace clang; using namespace ento; +/// The GDM component containing a mapping from temporary object binding +/// statements to the count of temporaries created at that statement. +/// Statements might create multiple temporaries if they're contained in a loop +/// (e.g. for (;;) { if (createTemporary().foo()){} } ). +REGISTER_TRAIT_WITH_PROGRAMSTATE( + LiveTemporaries, + CLANG_ENTO_PROGRAMSTATE_MAP(const CXXBindTemporaryExpr*, unsigned)); + +void ExprEngine::BindCXXTemporaryObject(const CXXBindTemporaryExpr *BTE, + ExplodedNode *Pred, + ExplodedNodeSet &Dst) { + StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx); + ProgramStateRef state = Pred->getState(); + if (const unsigned *binding_count = state->get(BTE)) { + state = state->set(BTE, *binding_count + 1); + } else { + state = state->set(BTE, 1); + } + Bldr.generateNode(BTE, Pred, state); +} + +void ExprEngine::processTemporaryCleanup(const CXXBindTemporaryExpr *BTE, + NodeBuilderContext &BuilderCtx, + ExplodedNode *Pred, + clang::ento::ExplodedNodeSet &Dst, + const CFGBlock *DstT, + const CFGBlock *DstF) { + PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext()); + currBldrCtx = &BuilderCtx; + + ProgramStateRef State = Pred->getState(); + + bool needs_cleanup = false; + if (const unsigned *binding_count = State->get(BTE)) { + needs_cleanup = true; + if (*binding_count == 1) { + State = State->remove(BTE); + } else { + State = State->set(BTE, *binding_count - 1); + } + } + + BranchNodeBuilder builder(Pred, Dst, BuilderCtx, DstT, DstF); + + builder.generateNode(State, !needs_cleanup, Pred); + builder.markInfeasible(needs_cleanup); + + currBldrCtx = 0; +} + void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME, ExplodedNode *Pred, ExplodedNodeSet &Dst) { @@ -304,6 +355,16 @@ const LocationContext *LCtx = Pred->getLocationContext(); ProgramStateRef State = Pred->getState(); + /*if (const CXXBindTemporaryExpr *BTE = dyn_cast(S)) { + const unsigned *binding_count = State->get(BTE); + assert(binding_count && "Destroying an unregistered temporary"); + if (*binding_count == 1) { + State = State->remove(BTE); + } else { + State = State->set(BTE, *binding_count - 1); + } + }*/ + // FIXME: We need to run the same destructor on every element of the array. // This workaround will just run the first destructor (which will still // invalidate the entire array). @@ -325,6 +386,9 @@ Call->getSourceRange().getBegin(), "Error evaluating destructor"); + //FIXME: TODO: remove! + llvm::errs() << "OMG running destructor with stack trace "; CrashInfo.print(llvm::errs()); llvm::errs() << "\n"; + ExplodedNodeSet DstPreCall; getCheckerManager().runCheckersForPreCall(DstPreCall, Pred, *Call, *this); Index: test/Analysis/temporaries.cpp =================================================================== --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -193,8 +193,7 @@ (i == 4 || i == 4 || compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) || i != 4) { - // FIXME: This shouldn't cause a warning. - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no warning, unreachable code } } @@ -211,8 +210,36 @@ void testConsistencyNestedComplex(bool value) { if (value) { if (!value || !value || check(NoReturnDtor())) { - // FIXME: This shouldn't cause a warning. - clang_analyzer_eval(true); // expected-warning{{TRUE}} + clang_analyzer_eval(true); // no warning, unreachable code + } + } + } + + // PR16664 and PR18159 + void testConsistencyNestedComplexMidBranch(bool value) { + if (value) { + if (!value || !value || check(NoReturnDtor()) || value) { + clang_analyzer_eval(true); // no warning, unreachable code + } + } + } + + // PR16664 and PR18159 + void testConsistencyNestedComplexNestedBranch(bool value) { + if (value) { + if (!value || (!value || check(NoReturnDtor()) || value)) { + clang_analyzer_eval(true); // no warning, unreachable code + } + } + } + + // PR16664 and PR18159 + void testConsistencyNestedVariableModification(bool value) { + bool other = true; + if (value) { + if (!other || !value || (other = false) || check(NoReturnDtor()) + || !other) { + clang_analyzer_eval(true); // no warning, unreachable code } } }