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,
baloghadamsoftware.
This patch implements the post important part of the plan proposed in
https://reviews.llvm.org/D54556. Under `alpha.cplusplus.Move:Aggressive=true`,
the checker retains the original behavior, but when that option is equal to
`false` (which is the proposed default behavior), the checker only warns in the
following three cases:
1. If the object that's being used after move is of a known STL type (the word
"known" here makes no sense; what i'm trying to say is that we'll rant about
objects of any STL type, unless we encounter false positives even there and
it'll cause us to restrict the check even further). The point here is that
according to the C++ Standard, STL objects are known to remain in "valid but
unspecified state", which means that the state is going to be internally
consistent state which is not known. A lot of operations clearly make no sense
(eg., asking for the value of the first character in a string that has just
been moved makes little sense), and we know it and we can warn in these cases
reliably. If some operations do make sense or, moreover, if some operations
return the object into a valid state, we can easily hardcode these operations
in the checker because STL has limited amount of stuff in it.
2. If the object is a local or parameter variable. This not only correlates
with the true positives that i've seen so far, but also kinda makes sense,
though, as usual, i'd love to be proven wrong. When an object is, say, a field
in a class, or, even worse, a global, then even if you move from that object,
the object is still there. It won't go anywhere; it *will* have to be re-used
eventually. Which means that if the object is left in a well-specified state
after getting moved, there's no good reason to enforce the developer to reset
it manually. However, if the object is a local that is going to be cleaned up
soon anyway, it is very unlikely that the desire to re-use its storage is
well-justified. It should be totally fine to create another local and use that
as a storage for the next object that you need. Hence the idea.
3. If the object is an rvalue reference. The idea is exactly the same as in 2:
an xvalue is something that's near the end of its lifetime, so there's no
temptation to re-use the storage. Whoever passed us that value is already fine
with it being moved away, we have no obligation to fill in the space again.
Repository:
rC Clang
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 foo(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
