NoQ updated this revision to Diff 174475.
NoQ added a comment.
Give the rvalue reference test function a sensible name.
https://reviews.llvm.org/D54557
Files:
lib/StaticAnalyzer/Checkers/MoveChecker.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
@@ -4,6 +4,14 @@
// 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 {
@@ -36,6 +44,13 @@
b = std::move(c);
}
+template <typename T>
+class vector {
+public:
+ vector();
+ void push_back(const T &t);
+};
+
} // namespace std
class B {
@@ -71,7 +86,10 @@
moveconstruct(std::move(*a));
}
A(const A &other) : i(other.i), d(other.d), b(other.b) {}
- A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
+ A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) {
+#ifdef AGGRESSIVE
+ // expected-note@-2{{'b' became 'moved-from' here}}
+#endif
}
A(A &&other, char *k) {
moveconstruct(std::move(other));
@@ -424,26 +442,51 @@
void f() {
A b;
- b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
- a.foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
+ b = std::move(a);
+ a.foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{'a' became 'moved-from' here}}
+ // expected-warning@-3 {{Method call on a 'moved-from' object 'a'}}
+ // expected-note@-4{{Method call on a 'moved-from' object 'a'}}
+#endif
- b = std::move(static_a); // expected-note {{'static_a' became 'moved-from' here}}
- static_a.foo(); // expected-warning {{Method call on a 'moved-from' object 'static_a'}} expected-note {{Method call on a 'moved-from' object 'static_a'}}
+ b = std::move(static_a);
+ static_a.foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{'static_a' became 'moved-from' here}}
+ // expected-warning@-3{{Method call on a 'moved-from' object 'static_a'}}
+ // expected-note@-4{{Method call on a 'moved-from' object 'static_a'}}
+#endif
}
};
void PtrAndArrayTest() {
A *Ptr = new A(1, 1.5);
A Arr[10];
- Arr[2] = std::move(*Ptr); // expected-note {{Became 'moved-from' here}}
- (*Ptr).foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+ Arr[2] = std::move(*Ptr);
+ (*Ptr).foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{Became 'moved-from' here}}
+ // expected-warning@-3{{Method call on a 'moved-from' object}}
+ // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
Ptr = &Arr[1];
- Arr[3] = std::move(Arr[1]); // expected-note {{Became 'moved-from' here}}
- Ptr->foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+ Arr[3] = std::move(Arr[1]);
+ Ptr->foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{Became 'moved-from' here}}
+ // expected-warning@-3{{Method call on a 'moved-from' object}}
+ // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
- Arr[3] = std::move(Arr[2]); // expected-note {{Became 'moved-from' here}}
- Arr[2].foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+ Arr[3] = std::move(Arr[2]);
+ Arr[2].foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{Became 'moved-from' here}}
+ // expected-warning@-3{{Method call on a 'moved-from' object}}
+ // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
Arr[2] = std::move(Arr[3]); // reinitialization
Arr[2].foo(); // no-warning
@@ -649,13 +692,24 @@
void subRegionMoveTest() {
{
A a;
- B b = std::move(a.b); // expected-note {{'b' became 'moved-from' here}}
- a.b.foo(); // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}}
+ B b = std::move(a.b);
+ a.b.foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{'b' became 'moved-from' here}}
+ // expected-warning@-3{{Method call on a 'moved-from' object 'b'}}
+ // expected-note@-4 {{Method call on a 'moved-from' object 'b'}}
+#endif
}
{
A a;
- A a1 = std::move(a); // expected-note {{Calling move constructor for 'A'}} expected-note {{Returning from move constructor for 'A'}}
- a.b.foo(); // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}}
+ A a1 = std::move(a);
+ a.b.foo();
+#ifdef AGGRESSIVE
+ // expected-note@-3{{Calling move constructor for 'A'}}
+ // expected-note@-4{{Returning from move constructor for 'A'}}
+ // expected-warning@-4{{Method call on a 'moved-from' object 'b'}}
+ // expected-note@-5{{Method call on a 'moved-from' object 'b'}}
+#endif
}
// Don't report a misuse if any SuperRegion is already reported.
{
@@ -726,3 +780,20 @@
MoveOnlyWithDestructor m;
return m;
}
+
+class HasSTLField {
+ std::vector<int> V;
+ void foo() {
+ // 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{{'V' became 'moved-from' here}}
+ V.push_back(123); // expected-warning{{Method call on a 'moved-from' object 'V'}}
+ // expected-note@-1{{Method call on a 'moved-from' object 'V'}}
+ }
+};
+
+void localRValueMove(A &&a) {
+ A b = std::move(a); // expected-note{{'a' became 'moved-from' here}}
+ a.foo(); // expected-warning{{Method call on a 'moved-from' object}}
+ // expected-note@-1{{Method call on a 'moved-from' object}}
+}
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -59,7 +59,16 @@
const char *NL, const char *Sep) const override;
private:
- enum MisuseKind {MK_FunCall, MK_Copy, MK_Move};
+ 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?
+ };
+
+ static ObjectKind classifyObject(const MemRegion *MR,
+ const CXXRecordDecl *RD);
+
class MovedBugVisitor : public BugReporterVisitor {
public:
MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {}
@@ -80,8 +89,14 @@
bool Found;
};
+ bool IsAggressive = false;
+
+public:
+ void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; }
+
+private:
mutable std::unique_ptr<BugType> BT;
- ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call,
+ ExplodedNode *reportBug(const MemRegion *Region, const CXXRecordDecl *RD,
CheckerContext &C, MisuseKind MK) const;
bool isInMoveSafeContext(const LocationContext *LC) const;
bool isStateResetMethod(const CXXMethodDecl *MethodDec) const;
@@ -115,6 +130,16 @@
return false;
}
+static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) {
+ if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(MR)) {
+ SymbolRef Sym = SR->getSymbol();
+ 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 &) {
@@ -139,7 +164,8 @@
Found = true;
std::string ObjectName;
- if (const auto DecReg = Region->getAs<DeclRegion>()) {
+ if (const auto DecReg =
+ unwrapRValueReferenceIndirection(Region)->getAs<DeclRegion>()) {
const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl());
ObjectName = RegionDecl->getNameAsString();
}
@@ -173,7 +199,8 @@
}
ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
- const CallEvent &Call, CheckerContext &C,
+ const CXXRecordDecl *RD,
+ CheckerContext &C,
MisuseKind MK) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
if (!BT)
@@ -243,6 +270,20 @@
if (!ArgRegion)
return;
+ // In non-aggressive mode, only warn on use-after-move of local variables (or
+ // local rvalue references) and of STL objects. The former is possible because
+ // local variables (or local rvalue references) are not tempting their user to
+ // re-use the storage. The latter is possible because STL objects are known
+ // to end up in a valid but unspecified state after the move and their
+ // state-reset methods are also known, which allows us to predict
+ // precisely when use-after-move is invalid.
+ // In aggressive mode, warn on any use-after-move because the user
+ // has intentionally asked us to completely eliminate use-after-move
+ // in his code.
+ ObjectKind OK = classifyObject(ArgRegion, MethodDecl->getParent());
+ if (!IsAggressive && !OK.Local && !OK.STL)
+ return;
+
// Skip moving the object to itself.
if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion)
return;
@@ -311,6 +352,17 @@
return false;
}
+MoveChecker::ObjectKind MoveChecker::classifyObject(const MemRegion *MR,
+ const CXXRecordDecl *RD) {
+ MR = unwrapRValueReferenceIndirection(MR);
+ return {
+ /*Local=*/
+ MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(MR->getMemorySpace()),
+ /*STL=*/
+ RD && RD->getDeclContext()->isStdNamespace()
+ };
+}
+
void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
const LocationContext *LC = C.getLocationContext();
@@ -329,10 +381,11 @@
const RegionState *ArgState = State->get<TrackedRegionMap>(ArgRegion);
if (ArgState && ArgState->isMoved()) {
if (!isInMoveSafeContext(LC)) {
+ const CXXRecordDecl *RD = CtorDec->getParent();
if(CtorDec->isMoveConstructor())
- N = reportBug(ArgRegion, Call, C, MK_Move);
+ N = reportBug(ArgRegion, RD, C, MK_Move);
else
- N = reportBug(ArgRegion, Call, C, MK_Copy);
+ N = reportBug(ArgRegion, RD, C, MK_Copy);
State = State->set<TrackedRegionMap>(ArgRegion,
RegionState::getReported());
}
@@ -357,6 +410,10 @@
const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
if (!MethodDecl)
return;
+
+ // Store class declaration as well, for bug reporting purposes.
+ const CXXRecordDecl *RD = MethodDecl->getParent();
+
// Checking assignment operators.
bool OperatorEq = MethodDecl->isOverloadedOperator() &&
MethodDecl->getOverloadedOperator() == OO_Equal;
@@ -371,9 +428,9 @@
if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) {
const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion();
if(MethodDecl->isMoveAssignmentOperator())
- N = reportBug(ArgRegion, Call, C, MK_Move);
+ N = reportBug(ArgRegion, RD, C, MK_Move);
else
- N = reportBug(ArgRegion, Call, C, MK_Copy);
+ N = reportBug(ArgRegion, RD, C, MK_Copy);
State =
State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported());
}
@@ -410,7 +467,7 @@
if (isInMoveSafeContext(LC))
return;
- N = reportBug(ThisRegion, Call, C, MK_FunCall);
+ N = reportBug(ThisRegion, RD, C, MK_FunCall);
State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
C.addTransition(State, N);
}
@@ -469,5 +526,7 @@
}
}
void ento::registerMoveChecker(CheckerManager &mgr) {
- mgr.registerChecker<MoveChecker>();
+ MoveChecker *chk = mgr.registerChecker<MoveChecker>();
+ chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ "Aggressive", false, chk));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
