[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

diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
index f96e41c..7a9018d 100644 (file)
--- 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<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(ProgramStateRef State,
   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(const ExplodedNode *N,
   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::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<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 CallEvent &Call, CheckerContext &C) const {
       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 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<TrackedRegionMap>(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<TrackedRegionMap>(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>();
+  MoveChecker *chk = mgr.registerChecker<MoveChecker>();
+  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 (file)
--- 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 <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}}
+}