Skip to content

Commit

Permalink
[analyzer] MoveChecker: Restrict to locals and std:: objects.
Browse files Browse the repository at this point in the history 
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

llvm-svn: 348210
  • Loading branch information
@haoNoQ
haoNoQ committed Dec 3, 2018
1 parent df4e9ba commit eb46925
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 25 deletions.
78 changes: 68 additions & 10 deletions clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
View file
Expand Up @@ -60,7 +60,16 @@ class MoveChecker
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) {}
Expand All @@ -81,8 +90,14 @@ class MoveChecker
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;
Expand Down Expand Up @@ -116,6 +131,16 @@ static bool isAnyBaseRegionReported(ProgramStateRef State,
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 &) {
Expand All @@ -140,7 +165,8 @@ MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
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();
}
Expand Down Expand Up @@ -174,7 +200,8 @@ const ExplodedNode *MoveChecker::getMoveLocation(const ExplodedNode *N,
}

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)
Expand Down Expand Up @@ -244,6 +271,20 @@ void MoveChecker::checkPostCall(const CallEvent &Call,
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;
Expand Down Expand Up @@ -312,6 +353,17 @@ bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const {
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();
Expand All @@ -330,10 +382,11 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
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());
}
Expand All @@ -359,6 +412,9 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
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;
Expand All @@ -373,9 +429,9 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
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());
}
Expand Down Expand Up @@ -412,7 +468,7 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
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);
}
Expand Down Expand Up @@ -471,5 +527,7 @@ void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State,
}
}
void ento::registerMoveChecker(CheckerManager &mgr) {
mgr.registerChecker<MoveChecker>();
MoveChecker *chk = mgr.registerChecker<MoveChecker>();
chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption(
"Aggressive", false, chk));
}
101 changes: 86 additions & 15 deletions clang/test/Analysis/use-after-move.cpp
View file
Expand Up @@ -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 {

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -71,7 +86,10 @@ class A {
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));
Expand Down Expand Up @@ -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@-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
Expand Down Expand Up @@ -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@-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.
{
Expand Down Expand Up @@ -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@-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}}
}

0 comments on commit eb46925

Please sign in to comment.