Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -59,7 +59,16 @@ const char *NL, const char *Sep) const override; private: - enum MisuseKind {MK_FunCall, MK_Copy, MK_Move}; + enum MisuseKind { MK_FunCall, MK_Copy, MK_Move }; + + struct ObjectKind { + bool Local : 1; // Is this a local variable or a local rvalue reference? + bool STL : 1; // Is this an object of a standard type? + }; + + static ObjectKind classifyObject(const MemRegion *MR, + const CXXRecordDecl *RD); + class MovedBugVisitor : public BugReporterVisitor { public: MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} @@ -80,8 +89,14 @@ bool Found; }; + bool IsAggressive = false; + +public: + void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; } + +private: mutable std::unique_ptr BT; - ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, + ExplodedNode *reportBug(const MemRegion *Region, const CXXRecordDecl *RD, CheckerContext &C, MisuseKind MK) const; bool isInMoveSafeContext(const LocationContext *LC) const; bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; @@ -115,6 +130,16 @@ return false; } +static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) { + if (const auto *SR = dyn_cast_or_null(MR)) { + SymbolRef Sym = SR->getSymbol(); + if (Sym->getType()->isRValueReferenceType()) + if (const MemRegion *OriginMR = Sym->getOriginRegion()) + return OriginMR; + } + return MR; +} + std::shared_ptr MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &) { @@ -139,7 +164,8 @@ Found = true; std::string ObjectName; - if (const auto DecReg = Region->getAs()) { + if (const auto DecReg = + unwrapRValueReferenceIndirection(Region)->getAs()) { const auto *RegionDecl = dyn_cast(DecReg->getDecl()); ObjectName = RegionDecl->getNameAsString(); } @@ -173,7 +199,8 @@ } ExplodedNode *MoveChecker::reportBug(const MemRegion *Region, - const CallEvent &Call, CheckerContext &C, + const CXXRecordDecl *RD, + CheckerContext &C, MisuseKind MK) const { if (ExplodedNode *N = C.generateNonFatalErrorNode()) { if (!BT) @@ -243,6 +270,20 @@ if (!ArgRegion) return; + // In non-aggressive mode, only warn on use-after-move of local variables (or + // local rvalue references) and of STL objects. The former is possible because + // local variables (or local rvalue references) are not tempting their user to + // re-use the storage. The latter is possible because STL objects are known + // to end up in a valid but unspecified state after the move and their + // state-reset methods are also known, which allows us to predict + // precisely when use-after-move is invalid. + // In aggressive mode, warn on any use-after-move because the user + // has intentionally asked us to completely eliminate use-after-move + // in his code. + ObjectKind OK = classifyObject(ArgRegion, MethodDecl->getParent()); + if (!IsAggressive && !OK.Local && !OK.STL) + return; + // Skip moving the object to itself. if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion) return; @@ -311,6 +352,17 @@ return false; } +MoveChecker::ObjectKind MoveChecker::classifyObject(const MemRegion *MR, + const CXXRecordDecl *RD) { + MR = unwrapRValueReferenceIndirection(MR); + return { + /*Local=*/ + MR && isa(MR) && isa(MR->getMemorySpace()), + /*STL=*/ + RD && RD->getDeclContext()->isStdNamespace() + }; +} + void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); const LocationContext *LC = C.getLocationContext(); @@ -329,10 +381,11 @@ const RegionState *ArgState = State->get(ArgRegion); if (ArgState && ArgState->isMoved()) { if (!isInMoveSafeContext(LC)) { + const CXXRecordDecl *RD = CtorDec->getParent(); if(CtorDec->isMoveConstructor()) - N = reportBug(ArgRegion, Call, C, MK_Move); + N = reportBug(ArgRegion, RD, C, MK_Move); else - N = reportBug(ArgRegion, Call, C, MK_Copy); + N = reportBug(ArgRegion, RD, C, MK_Copy); State = State->set(ArgRegion, RegionState::getReported()); } @@ -357,6 +410,10 @@ const auto MethodDecl = dyn_cast_or_null(IC->getDecl()); if (!MethodDecl) return; + + // Store class declaration as well, for bug reporting purposes. + const CXXRecordDecl *RD = MethodDecl->getParent(); + // Checking assignment operators. bool OperatorEq = MethodDecl->isOverloadedOperator() && MethodDecl->getOverloadedOperator() == OO_Equal; @@ -371,9 +428,9 @@ if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) { const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion(); if(MethodDecl->isMoveAssignmentOperator()) - N = reportBug(ArgRegion, Call, C, MK_Move); + N = reportBug(ArgRegion, RD, C, MK_Move); else - N = reportBug(ArgRegion, Call, C, MK_Copy); + N = reportBug(ArgRegion, RD, C, MK_Copy); State = State->set(ArgRegion, RegionState::getReported()); } @@ -410,7 +467,7 @@ if (isInMoveSafeContext(LC)) return; - N = reportBug(ThisRegion, Call, C, MK_FunCall); + N = reportBug(ThisRegion, RD, C, MK_FunCall); State = State->set(ThisRegion, RegionState::getReported()); C.addTransition(State, N); } @@ -469,5 +526,7 @@ } } void ento::registerMoveChecker(CheckerManager &mgr) { - mgr.registerChecker(); + MoveChecker *chk = mgr.registerChecker(); + chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption( + "Aggressive", false, chk)); } Index: test/Analysis/use-after-move.cpp =================================================================== --- test/Analysis/use-after-move.cpp +++ test/Analysis/use-after-move.cpp @@ -4,6 +4,14 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\ // RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\ // RUN: -analyzer-config exploration_strategy=dfs -DDFS=1 +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\ +// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\ +// RUN: -analyzer-config exploration_strategy=unexplored_first_queue\ +// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\ +// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\ +// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1\ +// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE namespace std { @@ -36,6 +44,13 @@ b = std::move(c); } +template +class vector { +public: + vector(); + void push_back(const T &t); +}; + } // namespace std class B { @@ -71,7 +86,10 @@ moveconstruct(std::move(*a)); } A(const A &other) : i(other.i), d(other.d), b(other.b) {} - A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}} + A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) { +#ifdef AGGRESSIVE + // expected-note@-2{{'b' became 'moved-from' here}} +#endif } A(A &&other, char *k) { moveconstruct(std::move(other)); @@ -424,26 +442,51 @@ void f() { A b; - b = std::move(a); // expected-note {{'a' became 'moved-from' here}} - a.foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}} + b = std::move(a); + a.foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{'a' became 'moved-from' here}} + // expected-warning@-3 {{Method call on a 'moved-from' object 'a'}} + // expected-note@-4{{Method call on a 'moved-from' object 'a'}} +#endif - b = std::move(static_a); // expected-note {{'static_a' became 'moved-from' here}} - static_a.foo(); // expected-warning {{Method call on a 'moved-from' object 'static_a'}} expected-note {{Method call on a 'moved-from' object 'static_a'}} + b = std::move(static_a); + static_a.foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{'static_a' became 'moved-from' here}} + // expected-warning@-3{{Method call on a 'moved-from' object 'static_a'}} + // expected-note@-4{{Method call on a 'moved-from' object 'static_a'}} +#endif } }; void PtrAndArrayTest() { A *Ptr = new A(1, 1.5); A Arr[10]; - Arr[2] = std::move(*Ptr); // expected-note {{Became 'moved-from' here}} - (*Ptr).foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}} + Arr[2] = std::move(*Ptr); + (*Ptr).foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{Became 'moved-from' here}} + // expected-warning@-3{{Method call on a 'moved-from' object}} + // expected-note@-4{{Method call on a 'moved-from' object}} +#endif Ptr = &Arr[1]; - Arr[3] = std::move(Arr[1]); // expected-note {{Became 'moved-from' here}} - Ptr->foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}} + Arr[3] = std::move(Arr[1]); + Ptr->foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{Became 'moved-from' here}} + // expected-warning@-3{{Method call on a 'moved-from' object}} + // expected-note@-4{{Method call on a 'moved-from' object}} +#endif - Arr[3] = std::move(Arr[2]); // expected-note {{Became 'moved-from' here}} - Arr[2].foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}} + Arr[3] = std::move(Arr[2]); + Arr[2].foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{Became 'moved-from' here}} + // expected-warning@-3{{Method call on a 'moved-from' object}} + // expected-note@-4{{Method call on a 'moved-from' object}} +#endif Arr[2] = std::move(Arr[3]); // reinitialization Arr[2].foo(); // no-warning @@ -649,13 +692,24 @@ void subRegionMoveTest() { { A a; - B b = std::move(a.b); // expected-note {{'b' became 'moved-from' here}} - a.b.foo(); // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}} + B b = std::move(a.b); + a.b.foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{'b' became 'moved-from' here}} + // expected-warning@-3{{Method call on a 'moved-from' object 'b'}} + // expected-note@-4 {{Method call on a 'moved-from' object 'b'}} +#endif } { A a; - A a1 = std::move(a); // expected-note {{Calling move constructor for 'A'}} expected-note {{Returning from move constructor for 'A'}} - a.b.foo(); // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}} + A a1 = std::move(a); + a.b.foo(); +#ifdef AGGRESSIVE + // expected-note@-3{{Calling move constructor for 'A'}} + // expected-note@-4{{Returning from move constructor for 'A'}} + // expected-warning@-4{{Method call on a 'moved-from' object 'b'}} + // expected-note@-5{{Method call on a 'moved-from' object 'b'}} +#endif } // Don't report a misuse if any SuperRegion is already reported. { @@ -726,3 +780,20 @@ MoveOnlyWithDestructor m; return m; } + +class HasSTLField { + std::vector V; + void foo() { + // Warn even in non-aggressive mode when it comes to STL, because + // in STL the object is left in "valid but unspecified state" after move. + std::vector W = std::move(V); // expected-note{{'V' became 'moved-from' here}} + V.push_back(123); // expected-warning{{Method call on a 'moved-from' object 'V'}} + // expected-note@-1{{Method call on a 'moved-from' object 'V'}} + } +}; + +void localRValueMove(A &&a) { + A b = std::move(a); // expected-note{{'a' became 'moved-from' here}} + a.foo(); // expected-warning{{Method call on a 'moved-from' object}} + // expected-note@-1{{Method call on a 'moved-from' object}} +}