NoQ updated this revision to Diff 177048.
NoQ marked an inline comment as done.
NoQ added a comment.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55307/new/
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,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<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 +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<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 +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<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
