From eb4692582a295feb6a9fde46200ac123802727bc Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Mon, 3 Dec 2018 23:06:07 +0000 Subject: [PATCH] [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 llvm-svn: 348210 --- clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp | 78 ++++++++++++++--- clang/test/Analysis/use-after-move.cpp | 101 ++++++++++++++++++---- 2 files changed, 154 insertions(+), 25 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp index f96e41c..7a9018d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -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 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(ProgramStateRef State, 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 &) { @@ -140,7 +165,8 @@ MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, 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(); } @@ -174,7 +200,8 @@ const ExplodedNode *MoveChecker::getMoveLocation(const ExplodedNode *N, } 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 CallEvent &Call, 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(const LocationContext *LC) const { 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(); @@ -330,10 +382,11 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { 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()); } @@ -359,6 +412,9 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { 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 CallEvent &Call, CheckerContext &C) const { 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()); } @@ -412,7 +468,7 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { 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); } @@ -471,5 +527,7 @@ void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State, } } void ento::registerMoveChecker(CheckerManager &mgr) { - mgr.registerChecker(); + MoveChecker *chk = mgr.registerChecker(); + chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption( + "Aggressive", false, chk)); } diff --git a/clang/test/Analysis/use-after-move.cpp b/clang/test/Analysis/use-after-move.cpp index d825596..9377329 100644 --- a/clang/test/Analysis/use-after-move.cpp +++ b/clang/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 @@ void swap(T &a, T &b) { b = std::move(c); } +template +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 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}} +} -- 2.7.4