NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, szepet,
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, baloghadamsoftware.
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.
Repository:
rC Clang
https://reviews.llvm.org/D55307
Files:
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx.h
test/Analysis/diagnostics/explicit-suppression.cpp
test/Analysis/use-after-move.cpp
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 <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 @@
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 @@
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'}}
+}
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/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<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 @@
}
+#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);
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,42 @@
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",
+ "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 +119,7 @@
BugReport &BR) override;
private:
+ const MoveChecker *Chk;
// The tracked region.
const MemRegion *Region;
// The class of the tracked object.
@@ -156,7 +180,7 @@
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)
@@ -181,7 +205,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 +274,7 @@
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;
}
@@ -369,20 +393,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<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 =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
