r349225 - [analyzer] MoveChecker: NFC: De-duplicate a few checks.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 14 17:50:58 PST 2018
Author: dergachev
Date: Fri Dec 14 17:50:58 2018
New Revision: 349225
URL: http://llvm.org/viewvc/llvm-project?rev=349225&view=rev
Log:
[analyzer] MoveChecker: NFC: De-duplicate a few checks.
No functional change intended.
Differential Revision: https://reviews.llvm.org/D55387
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=349225&r1=349224&r2=349225&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Fri Dec 14 17:50:58 2018
@@ -89,6 +89,20 @@ private:
"weak_ptr",
};
+ // Should we bother tracking the state of the object?
+ bool shouldBeTracked(ObjectKind OK) const {
+ // 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.
+ return IsAggressive || OK.Local || OK.STL;
+ }
+
// Obtains ObjectKind of an object. Because class declaration cannot always
// be easily obtained from the memory region, it is supplied separately.
ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const;
@@ -136,8 +150,20 @@ public:
private:
mutable std::unique_ptr<BugType> BT;
+
+ // Check if the given form of potential misuse of a given object
+ // should be reported. If so, get it reported. The callback from which
+ // this function was called should immediately return after the call
+ // because this function adds one or two transitions.
+ void modelUse(ProgramStateRef State, const MemRegion *Region,
+ const CXXRecordDecl *RD, MisuseKind MK,
+ CheckerContext &C) const;
+
+ // Returns the exploded node against which the report was emitted.
+ // The caller *must* add any further transitions against this node.
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;
bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const;
@@ -236,6 +262,25 @@ const ExplodedNode *MoveChecker::getMove
return MoveNode;
}
+void MoveChecker::modelUse(ProgramStateRef State, const MemRegion *Region,
+ const CXXRecordDecl *RD, MisuseKind MK,
+ CheckerContext &C) const {
+ assert(!C.isDifferent() && "No transitions should have been made by now");
+ const RegionState *RS = State->get<TrackedRegionMap>(Region);
+
+ if (!RS || isAnyBaseRegionReported(State, Region)
+ || isInMoveSafeContext(C.getLocationContext())) {
+ // Finalize changes made by the caller.
+ C.addTransition(State);
+ return;
+ }
+
+ ExplodedNode *N = reportBug(Region, RD, C, MK);
+
+ State = State->set<TrackedRegionMap>(Region, RegionState::getReported());
+ C.addTransition(State, N);
+}
+
ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
const CXXRecordDecl *RD,
CheckerContext &C,
@@ -294,12 +339,10 @@ void MoveChecker::checkPostCall(const Ca
if (!MethodDecl)
return;
- const auto *ConstructorDecl = dyn_cast<CXXConstructorDecl>(MethodDecl);
-
- const auto *CC = dyn_cast_or_null<CXXConstructorCall>(&Call);
// Check if an object became moved-from.
// Object can become moved from after a call to move assignment operator or
// move constructor .
+ const auto *ConstructorDecl = dyn_cast<CXXConstructorDecl>(MethodDecl);
if (ConstructorDecl && !ConstructorDecl->isMoveConstructor())
return;
@@ -310,23 +353,11 @@ 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.
+ const auto *CC = dyn_cast_or_null<CXXConstructorCall>(&Call);
if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion)
return;
+
if (const auto *IC = dyn_cast<CXXInstanceCall>(AFC))
if (IC->getCXXThisVal().getAsRegion() == ArgRegion)
return;
@@ -340,9 +371,16 @@ void MoveChecker::checkPostCall(const Ca
if (State->get<TrackedRegionMap>(ArgRegion))
return;
- // Mark object as moved-from.
- State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getMoved());
- C.addTransition(State);
+
+ const CXXRecordDecl *RD = MethodDecl->getParent();
+ ObjectKind OK = classifyObject(ArgRegion, RD);
+ if (shouldBeTracked(OK)) {
+ // Mark object as moved-from.
+ State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getMoved());
+ C.addTransition(State);
+ return;
+ }
+ assert(!C.isDifferent() && "Should not have made transitions on this path!");
}
bool MoveChecker::isMoveSafeMethod(const CXXMethodDecl *MethodDec) const {
@@ -435,8 +473,6 @@ MoveChecker::explainObject(llvm::raw_ost
void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
- const LocationContext *LC = C.getLocationContext();
- ExplodedNode *N = nullptr;
// Remove the MemRegions from the map on which a ctor/dtor call or assignment
// happened.
@@ -448,21 +484,11 @@ void MoveChecker::checkPreCall(const Cal
// Check for copying a moved-from object and report the bug.
if (CtorDec && CtorDec->isCopyOrMoveConstructor()) {
const MemRegion *ArgRegion = CC->getArgSVal(0).getAsRegion();
- 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, RD, C, MK_Move);
- else
- N = reportBug(ArgRegion, RD, C, MK_Copy);
- State = State->set<TrackedRegionMap>(ArgRegion,
- RegionState::getReported());
- }
- }
+ const CXXRecordDecl *RD = CtorDec->getParent();
+ MisuseKind MK = CtorDec->isMoveConstructor() ? MK_Move : MK_Copy;
+ modelUse(State, ArgRegion, RD, MK, C);
+ return;
}
- C.addTransition(State, N);
- return;
}
const auto IC = dyn_cast<CXXInstanceCall>(&Call);
@@ -477,47 +503,14 @@ void MoveChecker::checkPreCall(const Cal
if (!ThisRegion)
return;
+ // The remaining part is check only for method call on a moved-from object.
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;
- // Remove the tracked object for every assignment operator, but report bug
- // only for move or copy assignment's argument.
- if (OperatorEq) {
- State = removeFromState(State, ThisRegion);
- if (MethodDecl->isCopyAssignmentOperator() ||
- MethodDecl->isMoveAssignmentOperator()) {
- const RegionState *ArgState =
- State->get<TrackedRegionMap>(IC->getArgSVal(0).getAsRegion());
- if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) {
- const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion();
- if(MethodDecl->isMoveAssignmentOperator())
- N = reportBug(ArgRegion, RD, C, MK_Move);
- else
- N = reportBug(ArgRegion, RD, C, MK_Copy);
- State =
- State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported());
- }
- }
- C.addTransition(State, N);
- return;
- }
-
- // The remaining part is check only for method call on a moved-from object.
-
// We want to investigate the whole object, not only sub-object of a parent
// class in which the encountered method defined.
- while (const auto *BR = dyn_cast<CXXBaseObjectRegion>(ThisRegion))
- ThisRegion = BR->getSuperRegion();
-
- if (isMoveSafeMethod(MethodDecl))
- return;
+ ThisRegion = ThisRegion->getMostDerivedObjectRegion();
if (isStateResetMethod(MethodDecl)) {
State = removeFromState(State, ThisRegion);
@@ -525,21 +518,34 @@ void MoveChecker::checkPreCall(const Cal
return;
}
- // If it is already reported then we don't report the bug again.
- const RegionState *ThisState = State->get<TrackedRegionMap>(ThisRegion);
- if (!(ThisState && ThisState->isMoved()))
+ if (isMoveSafeMethod(MethodDecl))
return;
- // Don't report it in case if any base region is already reported
- if (isAnyBaseRegionReported(State, ThisRegion))
- return;
+ // Store class declaration as well, for bug reporting purposes.
+ const CXXRecordDecl *RD = MethodDecl->getParent();
- if (isInMoveSafeContext(LC))
- return;
+ if (MethodDecl->isOverloadedOperator()) {
+ OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
- N = reportBug(ThisRegion, RD, C, MK_FunCall);
- State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
- C.addTransition(State, N);
+ if (OOK == OO_Equal) {
+ // Remove the tracked object for every assignment operator, but report bug
+ // only for move or copy assignment's argument.
+ State = removeFromState(State, ThisRegion);
+
+ if (MethodDecl->isCopyAssignmentOperator() ||
+ MethodDecl->isMoveAssignmentOperator()) {
+ const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion();
+ MisuseKind MK =
+ MethodDecl->isMoveAssignmentOperator() ? MK_Move : MK_Copy;
+ modelUse(State, ArgRegion, RD, MK, C);
+ return;
+ }
+ C.addTransition(State);
+ return;
+ }
+ }
+
+ modelUse(State, ThisRegion, RD, MK_FunCall, C);
}
void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper,
More information about the cfe-commits
mailing list