r348210 - [analyzer] MoveChecker: Restrict to locals and std:: objects.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 3 15:06:07 PST 2018
Author: dergachev
Date: Mon Dec 3 15:06:07 2018
New Revision: 348210
URL: http://llvm.org/viewvc/llvm-project?rev=348210&view=rev
Log:
[analyzer] MoveChecker: Restrict to locals and std:: objects.
In general case there use-after-move is not a bug. It depends on how the
move-constructor or move-assignment is implemented.
In STL, the convention that applies to most classes is that the move-constructor
(-assignment) leaves an object in a "valid but unspecified" state. Using such
object without resetting it to a known state first is likely a bug. Objects
Local value-type variables are special because due to their automatic lifetime
there is no intention to reuse space. If you want a fresh object, you might
as well make a new variable, no need to move from a variable and than re-use it.
Therefore, it is not always a bug, but it is obviously easy to suppress when it
isn't, and in most cases it indeed is - as there's no valid intention behind
the intentional use of a local after move.
This applies not only to local variables but also to parameter variables,
not only of value type but also of rvalue reference type (but not to lvalue
references).
Differential Revision: https://reviews.llvm.org/D54557
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
cfe/trunk/test/Analysis/use-after-move.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=348210&r1=348209&r2=348210&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Mon Dec 3 15:06:07 2018
@@ -60,7 +60,16 @@ public:
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) {}
@@ -81,8 +90,14 @@ private:
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;
@@ -116,6 +131,16 @@ static bool isAnyBaseRegionReported(Prog
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 &) {
@@ -140,7 +165,8 @@ MoveChecker::MovedBugVisitor::VisitNode(
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();
}
@@ -174,7 +200,8 @@ const ExplodedNode *MoveChecker::getMove
}
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)
@@ -244,6 +271,20 @@ void MoveChecker::checkPostCall(const Ca
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;
@@ -312,6 +353,17 @@ bool MoveChecker::isInMoveSafeContext(co
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();
@@ -330,10 +382,11 @@ void MoveChecker::checkPreCall(const Cal
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());
}
@@ -359,6 +412,9 @@ void MoveChecker::checkPreCall(const Cal
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;
@@ -373,9 +429,9 @@ void MoveChecker::checkPreCall(const Cal
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());
}
@@ -412,7 +468,7 @@ void MoveChecker::checkPreCall(const Cal
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);
}
@@ -471,5 +527,7 @@ void MoveChecker::printState(raw_ostream
}
}
void ento::registerMoveChecker(CheckerManager &mgr) {
- mgr.registerChecker<MoveChecker>();
+ MoveChecker *chk = mgr.registerChecker<MoveChecker>();
+ chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ "Aggressive", false, chk));
}
Modified: cfe/trunk/test/Analysis/use-after-move.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/use-after-move.cpp?rev=348210&r1=348209&r2=348210&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/use-after-move.cpp (original)
+++ cfe/trunk/test/Analysis/use-after-move.cpp Mon Dec 3 15:06:07 2018
@@ -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 @@ void swap(T &a, T &b) {
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 @@ public:
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 at -2{{'b' became 'moved-from' here}}
+#endif
}
A(A &&other, char *k) {
moveconstruct(std::move(other));
@@ -424,26 +442,51 @@ class memberVariablesTest {
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 at -3{{'a' became 'moved-from' here}}
+ // expected-warning at -3 {{Method call on a 'moved-from' object 'a'}}
+ // expected-note at -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 at -3{{'static_a' became 'moved-from' here}}
+ // expected-warning at -3{{Method call on a 'moved-from' object 'static_a'}}
+ // expected-note at -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 at -3{{Became 'moved-from' here}}
+ // expected-warning at -3{{Method call on a 'moved-from' object}}
+ // expected-note at -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 at -3{{Became 'moved-from' here}}
+ // expected-warning at -3{{Method call on a 'moved-from' object}}
+ // expected-note at -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 at -3{{Became 'moved-from' here}}
+ // expected-warning at -3{{Method call on a 'moved-from' object}}
+ // expected-note at -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 @@ struct C : public A {
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 at -3{{'b' became 'moved-from' here}}
+ // expected-warning at -3{{Method call on a 'moved-from' object 'b'}}
+ // expected-note at -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 at -3{{Calling move constructor for 'A'}}
+ // expected-note at -4{{Returning from move constructor for 'A'}}
+ // expected-warning at -4{{Method call on a 'moved-from' object 'b'}}
+ // expected-note at -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 foo() {
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 at -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 at -1{{Method call on a 'moved-from' object}}
+}
More information about the cfe-commits
mailing list