Author: dergachev Date: Mon Dec 3 15:06:07 2018 New Revision: 348210 URL: http://llvm.org/viewvc/llvm-project?rev=348210&view=rev Log: [analyzer] MoveChecker: Restrict to locals and std:: objects.
In general case there use-after-move is not a bug. It depends on how the move-constructor or move-assignment is implemented. In STL, the convention that applies to most classes is that the move-constructor (-assignment) leaves an object in a "valid but unspecified" state. Using such object without resetting it to a known state first is likely a bug. Objects Local value-type variables are special because due to their automatic lifetime there is no intention to reuse space. If you want a fresh object, you might as well make a new variable, no need to move from a variable and than re-use it. Therefore, it is not always a bug, but it is obviously easy to suppress when it isn't, and in most cases it indeed is - as there's no valid intention behind the intentional use of a local after move. This applies not only to local variables but also to parameter variables, not only of value type but also of rvalue reference type (but not to lvalue references). Differential Revision: https://reviews.llvm.org/D54557 Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp cfe/trunk/test/Analysis/use-after-move.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=348210&r1=348209&r2=348210&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Mon Dec 3 15:06:07 2018 @@ -60,7 +60,16 @@ public: 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) {} @@ -81,8 +90,14 @@ private: bool Found; }; + bool IsAggressive = false; + +public: + void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; } + +private: mutable std::unique_ptr<BugType> 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; @@ -116,6 +131,16 @@ static bool isAnyBaseRegionReported(Prog return false; } +static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) { + if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(MR)) { + SymbolRef Sym = SR->getSymbol(); + if (Sym->getType()->isRValueReferenceType()) + if (const MemRegion *OriginMR = Sym->getOriginRegion()) + return OriginMR; + } + return MR; +} + std::shared_ptr<PathDiagnosticPiece> MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &) { @@ -140,7 +165,8 @@ MoveChecker::MovedBugVisitor::VisitNode( Found = true; std::string ObjectName; - if (const auto DecReg = Region->getAs<DeclRegion>()) { + if (const auto DecReg = + unwrapRValueReferenceIndirection(Region)->getAs<DeclRegion>()) { const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl()); ObjectName = RegionDecl->getNameAsString(); } @@ -174,7 +200,8 @@ const ExplodedNode *MoveChecker::getMove } 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) @@ -244,6 +271,20 @@ void MoveChecker::checkPostCall(const Ca 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; @@ -312,6 +353,17 @@ bool MoveChecker::isInMoveSafeContext(co return false; } +MoveChecker::ObjectKind MoveChecker::classifyObject(const MemRegion *MR, + const CXXRecordDecl *RD) { + MR = unwrapRValueReferenceIndirection(MR); + return { + /*Local=*/ + MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(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(); @@ -330,10 +382,11 @@ void MoveChecker::checkPreCall(const Cal const RegionState *ArgState = State->get<TrackedRegionMap>(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<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -359,6 +412,9 @@ void MoveChecker::checkPreCall(const Cal 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; @@ -373,9 +429,9 @@ void MoveChecker::checkPreCall(const Cal 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<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -412,7 +468,7 @@ void MoveChecker::checkPreCall(const Cal if (isInMoveSafeContext(LC)) return; - N = reportBug(ThisRegion, Call, C, MK_FunCall); + N = reportBug(ThisRegion, RD, C, MK_FunCall); State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported()); C.addTransition(State, N); } @@ -471,5 +527,7 @@ void MoveChecker::printState(raw_ostream } } void ento::registerMoveChecker(CheckerManager &mgr) { - mgr.registerChecker<MoveChecker>(); + MoveChecker *chk = mgr.registerChecker<MoveChecker>(); + chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption( + "Aggressive", false, chk)); } Modified: cfe/trunk/test/Analysis/use-after-move.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/use-after-move.cpp?rev=348210&r1=348209&r2=348210&view=diff ============================================================================== --- cfe/trunk/test/Analysis/use-after-move.cpp (original) +++ cfe/trunk/test/Analysis/use-after-move.cpp Mon Dec 3 15:06:07 2018 @@ -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 @@ void swap(T &a, T &b) { b = std::move(c); } +template <typename T> +class vector { +public: + vector(); + void push_back(const T &t); +}; + } // namespace std class B { @@ -71,7 +86,10 @@ public: 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 @@ class memberVariablesTest { 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 @@ struct C : public A { 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 foo() { MoveOnlyWithDestructor m; return m; } + +class HasSTLField { + std::vector<int> 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<int> 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}} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
