Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -428,21 +428,23 @@ bool isArgumentConstructedDirectly(unsigned Index) const { // This assumes that the object was not yet removed from the state. return ExprEngine::getObjectUnderConstruction( - getState(), {getOriginExpr(), Index}, getCalleeStackFrame()).hasValue(); + getState(), {getOriginExpr(), Index}, getLocationContext()).hasValue(); } /// Some calls have parameter numbering mismatched from argument numbering. /// This function converts an argument index to the corresponding /// parameter index. Returns None is the argument doesn't correspond /// to any parameter variable. - Optional getAdjustedParameterIndex(unsigned ArgumentIndex) const { - if (dyn_cast_or_null(getOriginExpr()) && - dyn_cast_or_null(getDecl())) { - // For member operator calls argument 0 on the expression corresponds - // to implicit this-parameter on the declaration. - return (ArgumentIndex > 0) ? Optional(ArgumentIndex - 1) : None; - } - return ArgumentIndex; + virtual Optional + getAdjustedParameterIndex(unsigned ASTArgumentIndex) const { + return ASTArgumentIndex; + } + + /// Some call event sub-classes conveniently adjust mismatching AST indices + /// to match parameter indices. This function converts an argument index + /// as understood by CallEvent to the argument index as understood by the AST. + virtual unsigned getASTArgumentIndex(unsigned CallArgumentIndex) const { + return CallArgumentIndex; } // Iterator access to formal parameters and their types. @@ -769,6 +771,20 @@ static bool classof(const CallEvent *CA) { return CA->getKind() == CE_CXXMemberOperator; } + + Optional + getAdjustedParameterIndex(unsigned ASTArgumentIndex) const override { + // For member operator calls argument 0 on the expression corresponds + // to implicit this-parameter on the declaration. + return (ASTArgumentIndex > 0) ? Optional(ASTArgumentIndex - 1) + : None; + } + + unsigned getASTArgumentIndex(unsigned CallArgumentIndex) const override { + // For member operator calls argument 0 on the expression corresponds + // to implicit this-parameter on the declaration. + return CallArgumentIndex + 1; + } }; /// Represents an implicit call to a C++ destructor. Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -662,6 +662,11 @@ const EvalCallOptions &CallOpts = {}); private: + ProgramStateRef finishArgumentConstruction(ProgramStateRef State, + const CallEvent &Call); + void finishArgumentConstruction(ExplodedNodeSet &Dst, ExplodedNode *Pred, + const CallEvent &Call); + void evalLoadCommon(ExplodedNodeSet &Dst, const Expr *NodeEx, /* Eventually will be a CFGStmt */ const Expr *BoundEx, Index: lib/StaticAnalyzer/Core/CallEvent.cpp =================================================================== --- lib/StaticAnalyzer/Core/CallEvent.cpp +++ lib/StaticAnalyzer/Core/CallEvent.cpp @@ -285,6 +285,20 @@ // TODO: Factor this out + handle the lower level const pointers. ValuesToInvalidate.push_back(getArgSVal(Idx)); + + // If a function accepts an object by argument (which would of course be a + // temporary that isn't lifetime-extended), invalidate the object itself, + // not only other objects reachable from it. This is necessary because the + // destructor has access to the temporary object after the call. + // TODO: Support placement arguments once we start + // constructing them directly. + // TODO: This is unnecessary when there's no destructor, but that's + // currently hard to figure out. + if (getKind() != CE_CXXAllocator) + if (isArgumentConstructedDirectly(Idx)) + if (auto AdjIdx = getAdjustedParameterIndex(Idx)) + if (const VarRegion *VR = getParameterLocation(*AdjIdx)) + ValuesToInvalidate.push_back(loc::MemRegionVal(VR)); } // Invalidate designated regions using the batch invalidation API. @@ -433,6 +447,10 @@ const ParmVarDecl *ParamDecl = *I; assert(ParamDecl && "Formal parameter has no decl?"); + if (Call.getKind() != CE_CXXAllocator) + if (Call.isArgumentConstructedDirectly(Idx)) + continue; + SVal ArgVal = Call.getArgSVal(Idx); if (!ArgVal.isUnknown()) { Loc ParamLoc = SVB.makeLoc(MRMgr.getVarRegion(ParamDecl, CalleeCtx)); Index: lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngine.cpp +++ lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -2200,17 +2200,21 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC, ExplodedNode *Pred, const ReturnStmt *RS) { + ProgramStateRef State = Pred->getState(); + + if (!Pred->getStackFrame()->inTopFrame()) + State = finishArgumentConstruction( + State, *getStateManager().getCallEventManager().getCaller( + Pred->getStackFrame(), Pred->getState())); + // FIXME: We currently cannot assert that temporaries are clear, because // lifetime extended 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. + // the state manually before asserting. Ideally, this braced block of code + // should go away. { - ExplodedNodeSet CleanUpObjects; - NodeBuilder Bldr(Pred, CleanUpObjects, BC); - ProgramStateRef State = Pred->getState(); const LocationContext *FromLC = Pred->getLocationContext(); const LocationContext *ToLC = FromLC->getStackFrame()->getParent(); const LocationContext *LC = FromLC; @@ -2229,15 +2233,20 @@ } LC = LC->getParent(); } - if (State != Pred->getState()) { - Pred = Bldr.generateNode(Pred->getLocation(), State, Pred); - if (!Pred) { - // The node with clean temporaries already exists. We might have reached - // it on a path on which we initialize different temporaries. - return; - } + } + + // Perform the transition with cleanups. + if (State != Pred->getState()) { + ExplodedNodeSet PostCleanup; + NodeBuilder Bldr(Pred, PostCleanup, BC); + Pred = Bldr.generateNode(Pred->getLocation(), State, Pred); + if (!Pred) { + // The node with clean temporaries already exists. We might have reached + // it on a path on which we initialize different temporaries. + return; } } + assert(areAllObjectsFullyConstructed(Pred->getState(), Pred->getLocationContext(), Pred->getStackFrame()->getParent())); Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -292,8 +292,75 @@ return std::make_pair(State, V); } case ConstructionContext::ArgumentKind: { - // Function argument constructors. Not implemented yet. - break; + // Arguments are technically temporaries. + CallOpts.IsTemporaryCtorOrDtor = true; + + const auto *ACC = cast(CC); + const Expr *E = ACC->getCallLikeExpr(); + unsigned Idx = ACC->getIndex(); + const CXXBindTemporaryExpr *BTE = ACC->getCXXBindTemporaryExpr(); + + CallEventManager &CEMgr = getStateManager().getCallEventManager(); + SVal V = UnknownVal(); + auto getArgLoc = [&](CallEventRef<> Caller) -> Optional { + const LocationContext *FutureSFC = Caller->getCalleeStackFrame(); + // Return early if we are unable to reliably foresee + // the future stack frame. + if (!FutureSFC) + return None; + + // This should be equivalent to Caller->getDecl() for now, but + // FutureSFC->getDecl() is likely to support better stuff (like + // virtual functions) earlier. + const Decl *CalleeD = FutureSFC->getDecl(); + + // FIXME: Support for variadic arguments is not implemented here yet. + if (CallEvent::isVariadic(CalleeD)) + return None; + + // Operator arguments do not correspond to operator parameters + // because this-argument is implemented as a normal argument in + // operator call expressions but not in operator declarations. + const VarRegion *VR = Caller->getParameterLocation( + *Caller->getAdjustedParameterIndex(Idx)); + if (!VR) + return None; + + return loc::MemRegionVal(VR); + }; + + if (const auto *CE = dyn_cast(E)) { + CallEventRef<> Caller = CEMgr.getSimpleCall(CE, State, LCtx); + if (auto OptV = getArgLoc(Caller)) + V = *OptV; + else + break; + State = addObjectUnderConstruction(State, {CE, Idx}, LCtx, V); + } else if (const auto *CCE = dyn_cast(E)) { + // Don't bother figuring out the target region for the future + // constructor because we won't need it. + CallEventRef<> Caller = + CEMgr.getCXXConstructorCall(CCE, /*Target=*/nullptr, State, LCtx); + if (auto OptV = getArgLoc(Caller)) + V = *OptV; + else + break; + State = addObjectUnderConstruction(State, {CCE, Idx}, LCtx, V); + } else if (const auto *ME = dyn_cast(E)) { + CallEventRef<> Caller = CEMgr.getObjCMethodCall(ME, State, LCtx); + if (auto OptV = getArgLoc(Caller)) + V = *OptV; + else + break; + State = addObjectUnderConstruction(State, {ME, Idx}, LCtx, V); + } + + assert(!V.isUnknown()); + + if (BTE) + State = addObjectUnderConstruction(State, BTE, LCtx, V); + + return std::make_pair(State, V); } } } @@ -502,8 +569,15 @@ } } + ExplodedNodeSet DstPostArgumentCleanup; + for (auto I : DstEvaluated) + finishArgumentConstruction(DstPostArgumentCleanup, I, *Call); + + // If there were other constructors called for object-type arguments + // of this constructor, clean them up. ExplodedNodeSet DstPostCall; - getCheckerManager().runCheckersForPostCall(DstPostCall, DstEvaluated, + getCheckerManager().runCheckersForPostCall(DstPostCall, + DstPostArgumentCleanup, *Call, *this); getCheckerManager().runCheckersForPostStmt(destNodes, DstPostCall, CE, *this); } Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -505,6 +505,49 @@ *this); } +ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State, + const CallEvent &Call) { + const Expr *E = Call.getOriginExpr(); + // FIXME: Constructors to placement arguments of operator new + // are not supported yet. + if (!E || isa(E)) + return State; + + const LocationContext *LC = Call.getLocationContext(); + for (unsigned CallI = 0, CallN = Call.getNumArgs(); CallI != CallN; ++CallI) { + unsigned I = Call.getASTArgumentIndex(CallI); + if (Optional V = + getObjectUnderConstruction(State, {E, I}, LC)) { + SVal VV = *V; + assert(cast(VV.castAs().getRegion()) + ->getStackFrame()->getParent() + ->getStackFrame() == LC->getStackFrame()); + State = finishObjectConstruction(State, {E, I}, LC); + } + } + + return State; +} + +void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst, + ExplodedNode *Pred, + const CallEvent &Call) { + ProgramStateRef State = Pred->getState(); + ProgramStateRef CleanedState = finishArgumentConstruction(State, Call); + if (CleanedState == State) { + Dst.insert(Pred); + return; + } + + const Expr *E = Call.getOriginExpr(); + const LocationContext *LC = Call.getLocationContext(); + NodeBuilder B(Pred, Dst, *currBldrCtx); + static SimpleProgramPointTag Tag("ExprEngine", + "Finish argument construction"); + PreStmt PP(E, LC, &Tag); + B.generateNode(PP, CleanedState, Pred); +} + void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred, const CallEvent &Call) { // WARNING: At this time, the state attached to 'Call' may be older than the @@ -516,7 +559,8 @@ // Run any pre-call checks using the generic call interface. ExplodedNodeSet dstPreVisit; - getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, Call, *this); + getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, + Call, *this); // Actually evaluate the function call. We try each of the checkers // to see if the can evaluate the function call, and get a callback at @@ -525,8 +569,14 @@ getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit, Call, *this); + // If there were other constructors called for object-type arguments + // of this call, clean them up. + ExplodedNodeSet dstArgumentCleanup; + for (auto I : dstCallEvaluated) + finishArgumentConstruction(dstArgumentCleanup, I, Call); + // Finally, run any post-call checks. - getCheckerManager().runCheckersForPostCall(Dst, dstCallEvaluated, + getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup, Call, *this); } Index: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -267,8 +267,13 @@ defaultEvalCall(Bldr, Pred, *UpdatedMsg); } + // If there were constructors called for object-type arguments, clean them up. + ExplodedNodeSet dstArgCleanup; + for (auto I : dstEval) + finishArgumentConstruction(dstArgCleanup, I, *Msg); + ExplodedNodeSet dstPostvisit; - getCheckerManager().runCheckersForPostCall(dstPostvisit, dstEval, + getCheckerManager().runCheckersForPostCall(dstPostvisit, dstArgCleanup, *Msg, *this); // Finally, perform the post-condition check of the ObjCMessageExpr and store Index: test/Analysis/copy-elision.cpp =================================================================== --- test/Analysis/copy-elision.cpp +++ test/Analysis/copy-elision.cpp @@ -122,7 +122,7 @@ namespace address_vector_tests { template struct AddressVector { - T *buf[10]; + T *buf[20]; int len; AddressVector() : len(0) {} @@ -138,13 +138,13 @@ public: ClassWithoutDestructor(AddressVector &v) : v(v) { - v.push(this); + push(); } - ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { v.push(this); } - ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) { - v.push(this); - } + ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { push(); } + ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) { push(); } + + void push() { v.push(this); } }; ClassWithoutDestructor make1(AddressVector &v) { @@ -174,20 +174,44 @@ #endif } +void consume(ClassWithoutDestructor c) { + c.push(); +} + +void testArgumentConstructorWithoutDestructor() { + AddressVector v; + + consume(make3(v)); + +#if ELIDE + clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}} +#else + clang_analyzer_eval(v.len == 6); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}} + // We forced a push() in consume(), let's see if the address here matches + // the address during construction. + clang_analyzer_eval(v.buf[4] == v.buf[5]); // expected-warning{{TRUE}} +#endif +} + class ClassWithDestructor { AddressVector &v; public: ClassWithDestructor(AddressVector &v) : v(v) { - v.push(this); + push(); } - ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { v.push(this); } - ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) { - v.push(this); - } + ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { push(); } + ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) { push(); } - ~ClassWithDestructor() { v.push(this); } + ~ClassWithDestructor() { push(); } + + void push() { v.push(this); } }; void testVariable() { @@ -301,4 +325,43 @@ clang_analyzer_eval(v.buf[7] == v.buf[9]); // expected-warning{{TRUE}} #endif } + +void consume(ClassWithDestructor c) { + c.push(); +} + +void testArgumentConstructorWithDestructor() { + AddressVector v; + + consume(make3(v)); + +#if ELIDE + // 0. Construct the argument. + // 1. Forced push() in consume(). + // 2. Destroy the argument. + clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[1] == v.buf[2]); // expected-warning{{TRUE}} +#else + // 0. Construct the temporary in make1(). + // 1. Construct the temporary in make2(). + // 2. Destroy the temporary in make1(). + // 3. Construct the temporary in make3(). + // 4. Destroy the temporary in make2(). + // 5. Construct the temporary here. + // 6. Destroy the temporary in make3(). + // 7. Construct the argument. + // 8. Forced push() in consume(). + // 9. Destroy the argument. Notice the reverse order! + // 10. Destroy the temporary here. + clang_analyzer_eval(v.len == 11); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[1] == v.buf[4]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[3] == v.buf[6]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[5] == v.buf[10]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[7] == v.buf[8]); // expected-warning{{TRUE}} + clang_analyzer_eval(v.buf[8] == v.buf[9]); // expected-warning{{TRUE}} +#endif +} + } // namespace address_vector_tests Index: test/Analysis/explain-svals.cpp =================================================================== --- test/Analysis/explain-svals.cpp +++ test/Analysis/explain-svals.cpp @@ -93,6 +93,6 @@ } // end of anonymous namespace void test_6() { - clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}} + clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of parameter ''}}}} clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}} } Index: test/Analysis/temporaries.cpp =================================================================== --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -1,7 +1,7 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s -// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s -// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11 -// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17 +// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s +// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s +// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11 +// RUN: %clang_analyze_cc1 -Wno-non-pod-varargs -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17 // Note: The C++17 run-line doesn't -verify yet - it is a no-crash test. @@ -962,6 +962,87 @@ } // end namespace pass_references_through +namespace arguments { +int glob; + +struct S { + int x; + S(int x): x(x) {} + S(const S &s) : x(s.x) {} + ~S() {} + + S &operator+(S s) { + glob = s.x; + x += s.x; + return *this; + } +}; + +class C { +public: + virtual void bar3(S s) {} +}; + +class D: public C { +public: + D() {} + virtual void bar3(S s) override { glob = s.x; } +}; + +void bar1(S s) { + glob = s.x; +} + +// Record-typed calls are a different CFGStmt, let's see if we handle that +// as well. +S bar2(S s) { + glob = s.x; + return S(3); +} + +void bar5(int, ...); + +void foo(void (*bar4)(S)) { + bar1(S(1)); + clang_analyzer_eval(glob == 1); +#ifdef TEMPORARY_DTORS + // expected-warning@-2{{TRUE}} +#else + // expected-warning@-4{{UNKNOWN}} +#endif + + bar2(S(2)); + clang_analyzer_eval(glob == 2); +#ifdef TEMPORARY_DTORS + // expected-warning@-2{{TRUE}} +#else + // expected-warning@-4{{UNKNOWN}} +#endif + + C *c = new D(); + c->bar3(S(3)); + // FIXME: Should be TRUE. + clang_analyzer_eval(glob == 3); // expected-warning{{UNKNOWN}} + delete c; + + // What if we've no idea what we're calling? + bar4(S(4)); // no-crash + + S(5) + S(6); + clang_analyzer_eval(glob == 6); +#ifdef TEMPORARY_DTORS + // expected-warning@-2{{TRUE}} +#else + // expected-warning@-4{{UNKNOWN}} +#endif + + // Variadic functions. This will __builtin_trap() because you cannot pass + // an object as a variadic argument. + bar5(7, S(7)); // no-crash + clang_analyzer_warnIfReached(); // no-warning +} +} // namespace arguments + namespace ctor_argument { // Stripped down unique_ptr struct IntPtr { @@ -1004,3 +1085,45 @@ } } // namespace operator_implicit_argument + +#if __cplusplus >= 201103L +namespace argument_lazy_bindings { +int glob; + +struct S { + int x, y, z; +}; + +struct T { + S s; + int w; + T(int w): s{5, 6, 7}, w(w) {} +}; + +void foo(T t) { + t.s = {1, 2, 3}; + glob = t.w; +} + +void bar() { + foo(T(4)); + clang_analyzer_eval(glob == 4); // expected-warning{{TRUE}} +} +} // namespace argument_lazy_bindings +#endif + +namespace operator_argument_cleanup { +struct S { + S(); +}; + +class C { +public: + void operator=(S); +}; + +void foo() { + C c; + c = S(); // no-crash +} +} // namespace operator_argument_cleanup