From 88bb563c4350ce67ebc435de046fa395e74c4670 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Fri, 15 Feb 2013 00:32:15 +0000 Subject: [PATCH] Re-apply "[analyzer] Model trivial copy/move ctors with an aggregate bind." ...after a host of optimizations related to the use of LazyCompoundVals (our implementation of aggregate binds). Originally applied in r173951. Reverted in r174069 because it was causing hangs. Re-applied in r174212. Reverted in r174265 because it was /still/ causing hangs. If this needs to be reverted again it will be punted to far in the future. llvm-svn: 175234 --- .../StaticAnalyzer/Core/PathSensitive/ExprEngine.h | 5 + clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 54 +++++++++-- .../Core/ExprEngineCallAndReturn.cpp | 35 +++++-- clang/test/Analysis/ctor-inlining.mm | 102 ++++++++++++++++++++- clang/test/Analysis/temporaries.cpp | 4 +- 5 files changed, 183 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index ba63620..598a915 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -44,6 +44,7 @@ namespace ento { class AnalysisManager; class CallEvent; class SimpleCall; +class CXXConstructorCall; class ExprEngine : public SubEngine { public: @@ -546,6 +547,10 @@ private: ExplodedNode *Pred); bool replayWithoutInlining(ExplodedNode *P, const LocationContext *CalleeLC); + + /// Models a trivial copy or move constructor call with a simple bind. + void performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred, + const CXXConstructorCall &Call); }; /// Traits for storing the call processing policy inside GDM. diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index 1f0c523..fdd50a6 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -49,6 +49,35 @@ void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME, Bldr.generateNode(ME, Pred, state->BindExpr(ME, LCtx, V)); } +void ExprEngine::performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred, + const CXXConstructorCall &Call) { + const CXXConstructExpr *CtorExpr = Call.getOriginExpr(); + assert(CtorExpr->getConstructor()->isCopyOrMoveConstructor()); + assert(CtorExpr->getConstructor()->isTrivial()); + + SVal ThisVal = Call.getCXXThisVal(); + const LocationContext *LCtx = Pred->getLocationContext(); + + ExplodedNodeSet Dst; + Bldr.takeNodes(Pred); + + SVal V = Call.getArgSVal(0); + + // Make sure the value being copied is not unknown. + if (const Loc *L = dyn_cast(&V)) + V = Pred->getState()->getSVal(*L); + + evalBind(Dst, CtorExpr, Pred, ThisVal, V, true); + + PostStmt PS(CtorExpr, LCtx); + for (ExplodedNodeSet::iterator I = Dst.begin(), E = Dst.end(); + I != E; ++I) { + ProgramStateRef State = (*I)->getState(); + State = bindReturnValue(Call, LCtx, State); + Bldr.generateNode(PS, State, *I); + } +} + void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ExplodedNode *Pred, ExplodedNodeSet &destNodes) { @@ -56,6 +85,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, ProgramStateRef State = Pred->getState(); const MemRegion *Target = 0; + bool IsArray = false; switch (CE->getConstructionKind()) { case CXXConstructExpr::CK_Complete: { @@ -79,6 +109,7 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, Target = State->getLValue(AT->getElementType(), getSValBuilder().makeZeroArrayIndex(), Base).getAsRegion(); + IsArray = true; } else { Target = State->getLValue(Var, LCtx).getAsRegion(); } @@ -148,14 +179,25 @@ void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE, getCheckerManager().runCheckersForPreCall(DstPreCall, DstPreVisit, *Call, *this); - ExplodedNodeSet DstInvalidated; - StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx); - for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end(); - I != E; ++I) - defaultEvalCall(Bldr, *I, *Call); + ExplodedNodeSet DstEvaluated; + StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx); + + if (CE->getConstructor()->isTrivial() && + CE->getConstructor()->isCopyOrMoveConstructor() && + !IsArray) { + // FIXME: Handle other kinds of trivial constructors as well. + for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end(); + I != E; ++I) + performTrivialCopy(Bldr, *I, *Call); + + } else { + for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end(); + I != E; ++I) + defaultEvalCall(Bldr, *I, *Call); + } ExplodedNodeSet DstPostCall; - getCheckerManager().runCheckersForPostCall(DstPostCall, DstInvalidated, + getCheckerManager().runCheckersForPostCall(DstPostCall, DstEvaluated, *Call, *this); getCheckerManager().runCheckersForPostStmt(destNodes, DstPostCall, CE, *this); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 54afe65..1d006b0 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -187,6 +187,23 @@ static bool wasDifferentDeclUsedForInlining(CallEventRef<> Call, return RuntimeCallee->getCanonicalDecl() != StaticDecl->getCanonicalDecl(); } +/// Returns true if the CXXConstructExpr \p E was intended to construct a +/// prvalue for the region in \p V. +/// +/// Note that we can't just test for rvalue vs. glvalue because +/// CXXConstructExprs embedded in DeclStmts and initializers are considered +/// rvalues by the AST, and the analyzer would like to treat them as lvalues. +static bool isTemporaryPRValue(const CXXConstructExpr *E, SVal V) { + if (E->isGLValue()) + return false; + + const MemRegion *MR = V.getAsRegion(); + if (!MR) + return false; + + return isa(MR); +} + /// The call exit is simulated with a sequence of nodes, which occur between /// CallExitBegin and CallExitEnd. The following operations occur between the /// two program points: @@ -247,13 +264,9 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) { svalBuilder.getCXXThis(CCE->getConstructor()->getParent(), calleeCtx); SVal ThisV = state->getSVal(This); - // If the constructed object is a prvalue, get its bindings. - // Note that we have to be careful here because constructors embedded - // in DeclStmts are not marked as lvalues. - if (!CCE->isGLValue()) - if (const MemRegion *MR = ThisV.getAsRegion()) - if (isa(MR)) - ThisV = state->getSVal(cast(ThisV)); + // If the constructed object is a temporary prvalue, get its bindings. + if (isTemporaryPRValue(CCE, ThisV)) + ThisV = state->getSVal(cast(ThisV)); state = state->BindExpr(CCE, callerCtx, ThisV); } @@ -692,7 +705,13 @@ ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call, } } } else if (const CXXConstructorCall *C = dyn_cast(&Call)){ - return State->BindExpr(E, LCtx, C->getCXXThisVal()); + SVal ThisV = C->getCXXThisVal(); + + // If the constructed object is a temporary prvalue, get its bindings. + if (isTemporaryPRValue(cast(E), ThisV)) + ThisV = State->getSVal(cast(ThisV)); + + return State->BindExpr(E, LCtx, ThisV); } // Conjure a symbol if the return value is unknown. diff --git a/clang/test/Analysis/ctor-inlining.mm b/clang/test/Analysis/ctor-inlining.mm index be5d81a..1603ff3 100644 --- a/clang/test/Analysis/ctor-inlining.mm +++ b/clang/test/Analysis/ctor-inlining.mm @@ -1,8 +1,15 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify %s void clang_analyzer_eval(bool); void clang_analyzer_checkInlined(bool); +// A simplified version of std::move. +template +T &&move(T &obj) { + return static_cast(obj); +} + + struct Wrapper { __strong id obj; }; @@ -117,3 +124,96 @@ namespace ConstructorUsedAsRValue { clang_analyzer_eval(result); // expected-warning{{TRUE}} } } + +namespace PODUninitialized { + class POD { + public: + int x, y; + }; + + class PODWrapper { + public: + POD p; + }; + + class NonPOD { + public: + int x, y; + + NonPOD() {} + NonPOD(const NonPOD &Other) + : x(Other.x), y(Other.y) // expected-warning {{undefined}} + { + } + NonPOD(NonPOD &&Other) + : x(Other.x), y(Other.y) // expected-warning {{undefined}} + { + } + }; + + class NonPODWrapper { + public: + class Inner { + public: + int x, y; + + Inner() {} + Inner(const Inner &Other) + : x(Other.x), y(Other.y) // expected-warning {{undefined}} + { + } + Inner(Inner &&Other) + : x(Other.x), y(Other.y) // expected-warning {{undefined}} + { + } + }; + + Inner p; + }; + + void testPOD() { + POD p; + p.x = 1; + POD p2 = p; // no-warning + clang_analyzer_eval(p2.x == 1); // expected-warning{{TRUE}} + POD p3 = move(p); // no-warning + clang_analyzer_eval(p3.x == 1); // expected-warning{{TRUE}} + + // Use rvalues as well. + clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}} + + PODWrapper w; + w.p.y = 1; + PODWrapper w2 = w; // no-warning + clang_analyzer_eval(w2.p.y == 1); // expected-warning{{TRUE}} + PODWrapper w3 = move(w); // no-warning + clang_analyzer_eval(w3.p.y == 1); // expected-warning{{TRUE}} + + // Use rvalues as well. + clang_analyzer_eval(PODWrapper(w3).p.y == 1); // expected-warning{{TRUE}} + } + + void testNonPOD() { + NonPOD p; + p.x = 1; + NonPOD p2 = p; + } + + void testNonPODMove() { + NonPOD p; + p.x = 1; + NonPOD p2 = move(p); + } + + void testNonPODWrapper() { + NonPODWrapper w; + w.p.y = 1; + NonPODWrapper w2 = w; + } + + void testNonPODWrapperMove() { + NonPODWrapper w; + w.p.y = 1; + NonPODWrapper w2 = move(w); + } +} diff --git a/clang/test/Analysis/temporaries.cpp b/clang/test/Analysis/temporaries.cpp index 547be02..e885878 100644 --- a/clang/test/Analysis/temporaries.cpp +++ b/clang/test/Analysis/temporaries.cpp @@ -16,7 +16,7 @@ Trivial getTrivial() { } const Trivial &getTrivialRef() { - return Trivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'struct Trivial' returned to caller}} + return Trivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'const struct Trivial' returned to caller}} } @@ -25,6 +25,6 @@ NonTrivial getNonTrivial() { } const NonTrivial &getNonTrivialRef() { - return NonTrivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'struct NonTrivial' returned to caller}} + return NonTrivial(42); // expected-warning {{Address of stack memory associated with temporary object of type 'const struct NonTrivial' returned to caller}} } -- 2.7.4