Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ 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; @@ -66,20 +67,43 @@ 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; @@ -96,6 +120,7 @@ BugReport &BR) override; private: + const MoveChecker &Chk; // The tracked region. const MemRegion *Region; // The class of the tracked object. @@ -156,7 +181,7 @@ std::shared_ptr 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) @@ -181,7 +206,7 @@ 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 @@ -250,7 +275,7 @@ auto R = llvm::make_unique(*BT, OS.str(), N, LocUsedForUniqueing, MoveNode->getLocationContext()->getDecl()); - R->addVisitor(llvm::make_unique(Region, RD)); + R->addVisitor(llvm::make_unique(*this, Region, RD)); C.emitReport(std::move(R)); return N; } @@ -369,20 +394,30 @@ 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(MR) && isa(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 = Index: test/Analysis/Inputs/system-header-simulator-cxx.h =================================================================== --- test/Analysis/Inputs/system-header-simulator-cxx.h +++ test/Analysis/Inputs/system-header-simulator-cxx.h @@ -237,6 +237,13 @@ return static_cast(a); } + template + void swap(T &a, T &b) { + T c(std::move(a)); + a = std::move(b); + b = std::move(c); + } + template class vector { typedef T value_type; @@ -770,6 +777,22 @@ } +#if __cplusplus >= 201103L +namespace std { + template // 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::type operator*() const; + T *operator->() const; + }; +} +#endif + #ifdef TEST_INLINABLE_ALLOCATORS namespace std { void *malloc(size_t); Index: test/Analysis/diagnostics/explicit-suppression.cpp =================================================================== --- test/Analysis/diagnostics/explicit-suppression.cpp +++ test/Analysis/diagnostics/explicit-suppression.cpp @@ -19,6 +19,6 @@ 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 } Index: test/Analysis/use-after-move.cpp =================================================================== --- test/Analysis/use-after-move.cpp +++ 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 -struct remove_reference; - -template -struct remove_reference { typedef _Tp type; }; - -template -struct remove_reference<_Tp &> { typedef _Tp type; }; - -template -struct remove_reference<_Tp &&> { typedef _Tp type; }; - -template -typename remove_reference<_Tp>::type &&move(_Tp &&__t) { - return static_cast::type &&>(__t); -} - -template -_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept { - return static_cast<_Tp &&>(__t); -} - -template -void swap(T &a, T &b) { - T c(std::move(a)); - a = std::move(b); - b = std::move(c); -} - -template -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 @@ class HasSTLField { std::vector 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 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 P; + void testUniquePtr() { + // unique_ptr remains in a well-defined state after move. + std::unique_ptr 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 @@ 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 P) { + // Even though unique_ptr is safe to use after move, + // reusing a local variable this way usually indicates a bug. + std::unique_ptr 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'}} +}