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

[NoQ]

Summary

List of such classes shamelessly copied from https://wiki.sei.cmu.edu/confluence/x/O3s-BQ - thanks @aaron.ballman!

Warnings for locals are not suppressed.

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

Also converted use-after-move tests to use the system header simulator so that not to grow STL code duplication.

[Szelethus]

This looks great!

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

How about making the set constexpr?

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

124

Why not const reference?

test/Analysis/diagnostics/explicit-suppression.cpp

22

Can't we just change this to // expected-warning{{Called C++ object pointer is null}}? This file is so tiny, I think it wouldn't cause much confusion, and reduces unnecessary maintenance work.

test/Analysis/use-after-move.cpp

16

This is cool, thanks!

[NoQ]

Convert the pointer to a reference.

In D55307#1320434, @Szelethus wrote:

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

How about making the set constexpr?

Mmm, i don't think it works this way, even std::set can't be made constexpr yet as of C++17, let alone an LLVM custom collection. It should be possible to make a constexpr array, but we'll lose the presumably-faster lookup this way, so not really worth it; the cost is minimal anyway.

test/Analysis/diagnostics/explicit-suppression.cpp

22

I don't think it'll work. The warning is not on this line, it is in system-header-simulator-cxx.h, so we need to specify it somehow, and it'll appear only in this test, not in other tests that include that header, so we can't put it directly into the header.

[Szelethus]

AFAIK constexpr arrays can be std::sort-ed, but it probably isn't worth the effort, I tried it myself when I was working with non-checker configs, and it's a big hassle for ultimately very little gain.

test/Analysis/diagnostics/explicit-suppression.cpp

22

Ah, okay.

[Szelethus]

test/Analysis/diagnostics/explicit-suppression.cpp

22

Buuuuut since this is the only warning in the file, we could get away with it of we use FileCheck! But I leave it up to you.

I had a lot of bots breaking on me because I forgot git add on this file once, so I might end up fixing it myself.

[NoQ]

Add one more important case: std::optional. When moved, it moves the object within it into the new optional, if any. So the object is left in unspecified state, but the state of the optional itself is well-specified.

test/Analysis/diagnostics/explicit-suppression.cpp

22

Mmm, maybe. This test is tiny enough.

[a_sidorin]

I think the change is fine, just a minor stylish remark.

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

782

Nit: our coding rules require a space before template lbrace.

[NoQ]

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

782

Fxd.

[RKSimon]

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

81

@NoQ https://bugs.llvm.org/show_bug.cgi?id=47030 is reporting that this contains a missing comma - please can you take a look?

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

 
 #include "ClangSACheckers.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #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;
 
 namespace {
 
 struct RegionState {
 private:

 private:
   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?
   };
 
+  // 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;
       ID.AddPointer(&X);
       ID.AddPointer(Region);
       // Don't add RD because it's, in theory, uniquely determined by
       // the region. In practice though, it's not always possible to obtain
       // the declaration directly from the region, that's why we store it
       // in the first place.
     }
 
     std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                    BugReporterContext &BRC,
                                                    BugReport &BR) override;
 
   private:
+    const MoveChecker &Chk;
     // The tracked region.
     const MemRegion *Region;
     // The class of the tracked object.
     const CXXRecordDecl *RD;
     bool Found;
   };
 
   bool IsAggressive = false;
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 &) {
+                                        BugReporterContext &BRC, BugReport &BR) {
   // We need only the last move of the reported object's region.
   // The visitor walks the ExplodedGraph backwards.
   if (Found)
     return nullptr;
   ProgramStateRef State = N->getState();
   ProgramStateRef StatePrev = N->getFirstPred()->getState();
   const RegionState *TrackedObject = State->get<TrackedRegionMap>(Region);
   const RegionState *TrackedObjectPrev =
       StatePrev->get<TrackedRegionMap>(Region);
   if (!TrackedObject)
     return nullptr;
   if (TrackedObjectPrev && TrackedObject)
     return nullptr;
 
   // Retrieve the associated statement.
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
   if (!S)
     return nullptr;
   Found = true;
 
   SmallString<128> Str;
   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
     OS << " is moved";
 
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
switch(MK) {
         explainObject(OS, Region, RD);
         OS << " is moved";
         break;
     }
 
     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;
   }
   return nullptr;
 }
 
 void MoveChecker::checkPostCall(const CallEvent &Call,
                                 CheckerContext &C) const {
if (DtorDec || (CtorDec && CtorDec->isCopyOrMoveConstructor()) ||
         (MethodDec && MethodDec->isOverloadedOperator() &&
          MethodDec->getOverloadedOperator() == OO_Equal) ||
         isStateResetMethod(MethodDec) || isMoveSafeMethod(MethodDec))
       return true;
   } while ((LC = LC->getParent()));
   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 =
           dyn_cast_or_null<DeclRegion>(unwrapRValueReferenceIndirection(MR))) {
     const auto *RegionDecl = cast<NamedDecl>(DR->getDecl());
     OS << " '" << RegionDecl->getNameAsString() << "'";
   }
   ObjectKind OK = classifyObject(MR, RD);

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

namespace std {
   template< class T > struct remove_reference<T&&> {typedef T type;};
 
   template<class T> 
   typename remove_reference<T>::type&& move(T&& a) {
     typedef typename remove_reference<T>::type&& RvalRef;
     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;
     typedef size_t size_type;
     typedef __vector_iterator<T, T *, T &> iterator;
     typedef __vector_iterator<T, const T *, const T &> const_iterator;
 
     T *_start;
ForwardIterator1 find_first_of(ForwardIterator1 first1,
                                  ForwardIterator2 last2);
 
   template <class InputIterator, class OutputIterator>
   OutputIterator copy(InputIterator first, InputIterator last,
                       OutputIterator result);
 
 }
 
+#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);
   void free(void *);
 }
 void* operator new(std::size_t size, const std::nothrow_t&) throw() { return std::malloc(size); }
 void* operator new[](std::size_t size, const std::nothrow_t&) throw() { return std::malloc(size); }
 void operator delete(void* ptr, const std::nothrow_t&) throw() { std::free(ptr); }

test/Analysis/diagnostics/explicit-suppression.cpp

class C {
   // The virtual function is to make C not trivially copy assignable so that we call the
   // variant of std::copy() that does not defer to memmove().
   virtual int f();
 };
 
 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
 }

test/Analysis/use-after-move.cpp

 // 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: %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 {
-
-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:
   B() = default;
   B(const B &) = default;
   B(B &&) = default;
   B& operator=(const B &q) = default;
   void operator=(B &&b) {

 
 MoveOnlyWithDestructor foo() {
   MoveOnlyWithDestructor m;
   return m;
 }
 
 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) {
   A b = std::move(a); // expected-note{{Object 'a' is moved}}
   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'}}
+}