[analyzer] MoveChecker Pt.6: Suppress the warning for the move-safe STL classes.

Some C++ standard library classes provide additional guarantees about their
state after move. Suppress warnings on such classes until a more precise
behavior is implemented. Warnings for locals are not suppressed anyway
because it's still most likely a bug.

Differential Revision: https://reviews.llvm.org/D55307

llvm-svn: 349191

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

@@ -20,6 +20,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/StringSet.h"
 
 using namespace clang;
 using namespace ento;
@@ -67,20 +68,43 @@ private:
     bool STL : 1; // Is this an object of a standard type?
   };
 
+  // Not all of these are entirely move-safe, but they do provide *some*
+  // guarantees, and it means that somebody is using them after move
+  // in a valid manner.
+  // TODO: We can still try to identify *unsafe* use after move, such as
+  // dereference of a moved-from smart pointer (which is guaranteed to be null).
+  const llvm::StringSet<> StandardMoveSafeClasses = {
+      "basic_filebuf",
+      "basic_ios",
+      "future",
+      "optional",
+      "packaged_task"
+      "promise",
+      "shared_future",
+      "shared_lock",
+      "shared_ptr",
+      "thread",
+      "unique_ptr",
+      "unique_lock",
+      "weak_ptr",
+  };
+
   // Obtains ObjectKind of an object. Because class declaration cannot always
   // be easily obtained from the memory region, it is supplied separately.
-  static ObjectKind classifyObject(const MemRegion *MR,
-                                   const CXXRecordDecl *RD);
+  ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const;
 
   // Classifies the object and dumps a user-friendly description string to
   // the stream. Return value is equivalent to classifyObject.
-  static ObjectKind explainObject(llvm::raw_ostream &OS,
-                                  const MemRegion *MR, const CXXRecordDecl *RD);
+  ObjectKind explainObject(llvm::raw_ostream &OS,
+                           const MemRegion *MR, const CXXRecordDecl *RD) const;
+
+  bool isStandardMoveSafeClass(const CXXRecordDecl *RD) const;
 
   class MovedBugVisitor : public BugReporterVisitor {
   public:
-    MovedBugVisitor(const MemRegion *R, const CXXRecordDecl *RD)
-        : Region(R), RD(RD), Found(false) {}
+    MovedBugVisitor(const MoveChecker &Chk,
+                    const MemRegion *R, const CXXRecordDecl *RD)
+        : Chk(Chk), Region(R), RD(RD), Found(false) {}
 
     void Profile(llvm::FoldingSetNodeID &ID) const override {
       static int X = 0;
@@ -97,6 +121,7 @@ private:
                                                    BugReport &BR) override;
 
   private:
+    const MoveChecker &Chk;
     // The tracked region.
     const MemRegion *Region;
     // The class of the tracked object.
@@ -157,7 +182,7 @@ static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) {
 
 std::shared_ptr<PathDiagnosticPiece>
 MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
-                                        BugReporterContext &BRC, BugReport &) {
+                                        BugReporterContext &BRC, BugReport &BR) {
   // We need only the last move of the reported object's region.
   // The visitor walks the ExplodedGraph backwards.
   if (Found)
@@ -182,7 +207,7 @@ MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
   llvm::raw_svector_ostream OS(Str);
 
   OS << "Object";
-  ObjectKind OK = explainObject(OS, Region, RD);
+  ObjectKind OK = Chk.explainObject(OS, Region, RD);
   if (OK.STL)
     OS << " is left in a valid but unspecified state after move";
   else
@@ -251,7 +276,7 @@ ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
     auto R =
         llvm::make_unique<BugReport>(*BT, OS.str(), N, LocUsedForUniqueing,
                                      MoveNode->getLocationContext()->getDecl());
-    R->addVisitor(llvm::make_unique<MovedBugVisitor>(Region, RD));
+    R->addVisitor(llvm::make_unique<MovedBugVisitor>(*this, Region, RD));
     C.emitReport(std::move(R));
     return N;
   }
@@ -370,20 +395,30 @@ bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const {
   return false;
 }
 
-MoveChecker::ObjectKind MoveChecker::classifyObject(const MemRegion *MR,
-                                                    const CXXRecordDecl *RD) {
+bool MoveChecker::isStandardMoveSafeClass(const CXXRecordDecl *RD) const {
+  const IdentifierInfo *II = RD->getIdentifier();
+  return II && StandardMoveSafeClasses.count(II->getName());
+}
+
+MoveChecker::ObjectKind
+MoveChecker::classifyObject(const MemRegion *MR,
+                            const CXXRecordDecl *RD) const {
+  // Local variables and local rvalue references are classified as "Local".
+  // For the purposes of this checker, we classify move-safe STL types
+  // as not-"STL" types, because that's how the checker treats them.
   MR = unwrapRValueReferenceIndirection(MR);
   return {
     /*Local=*/
         MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(MR->getMemorySpace()),
     /*STL=*/
-        RD && RD->getDeclContext()->isStdNamespace()
+        RD && RD->getDeclContext()->isStdNamespace() &&
+        !isStandardMoveSafeClass(RD)
   };
 }
 
-MoveChecker::ObjectKind MoveChecker::explainObject(llvm::raw_ostream &OS,
-                                                   const MemRegion *MR,
-                                                   const CXXRecordDecl *RD) {
+MoveChecker::ObjectKind
+MoveChecker::explainObject(llvm::raw_ostream &OS, const MemRegion *MR,
+                           const CXXRecordDecl *RD) const {
   // We may need a leading space every time we actually explain anything,
   // and we never know if we are to explain anything until we try.
   if (const auto DR =

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

@@ -237,6 +237,13 @@ namespace std {
     return static_cast<RvalRef>(a);
   }
 
+  template <class T>
+  void swap(T &a, T &b) {
+    T c(std::move(a));
+    a = std::move(b);
+    b = std::move(c);
+  }
+
   template<typename T>
   class vector {
     typedef T value_type;
@@ -770,6 +777,22 @@ namespace std {
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template <typename T> // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+    unique_ptr(const unique_ptr &) = delete;
+    unique_ptr(unique_ptr &&);
+
+    T *get() const;
+
+    typename std::add_lvalue_reference<T>::type operator*() const;
+    T *operator->() const;
+  };
+}
+#endif
+
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);

clang/test/Analysis/diagnostics/explicit-suppression.cpp

@@ -19,6 +19,6 @@ class C {
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }

clang/test/Analysis/use-after-move.cpp

@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template <typename>
-struct remove_reference;
-
-template <typename _Tp>
-struct remove_reference { typedef _Tp type; };
-
-template <typename _Tp>
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template <typename _Tp>
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template <typename _Tp>
-typename remove_reference<_Tp>::type &&move(_Tp &&__t) {
-  return static_cast<typename remove_reference<_Tp>::type &&>(__t);
-}
-
-template <typename _Tp>
-_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template <class T>
-void swap(T &a, T &b) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template <typename T>
-class vector {
-public:
-  vector();
-  void push_back(const T &t);
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@ MoveOnlyWithDestructor foo() {
 
 class HasSTLField {
   std::vector<int> V;
-  void foo() {
+  void testVector() {
     // 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{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
     V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
                       // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr<int> P;
+  void testUniquePtr() {
+    // unique_ptr remains in a well-defined state after move.
+    std::unique_ptr<int> Q = std::move(P);
+    P.get();
+#ifdef AGGRESSIVE
+    // expected-warning@-2{{Method called on moved-from object 'P'}}
+    // expected-note@-4{{Object 'P' is moved}}
+    // expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+    *P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &&a) {
@@ -846,3 +819,11 @@ void localRValueMove(A &&a) {
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
            // expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr<int> P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr<int> Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+           // expected-note@-1{{Method called on moved-from object 'P'}}
+}