From 1fa6f0a871e70af7a02920ce8f00af2a1d8c1bd8 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Sat, 15 Dec 2018 01:50:58 +0000 Subject: [PATCH] [analyzer] MoveChecker: NFC: De-duplicate a few checks. No functional change intended. Differential Revision: https://reviews.llvm.org/D55387 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@349225 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Checkers/MoveChecker.cpp | 170 ++++++++++---------- 1 file changed, 88 insertions(+), 82 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/lib/StaticAnalyzer/Checkers/MoveChecker.cpp index dd83ce02e0..d387e0bea7 100644 --- a/lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -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 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::getMoveLocation(const ExplodedNode *N, 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(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(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 CallEvent &Call, if (!MethodDecl) return; - const auto *ConstructorDecl = dyn_cast(MethodDecl); - - const auto *CC = dyn_cast_or_null(&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(MethodDecl); if (ConstructorDecl && !ConstructorDecl->isMoveConstructor()) return; @@ -310,23 +353,11 @@ 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. + const auto *CC = dyn_cast_or_null(&Call); if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion) return; + if (const auto *IC = dyn_cast(AFC)) if (IC->getCXXThisVal().getAsRegion() == ArgRegion) return; @@ -340,9 +371,16 @@ void MoveChecker::checkPostCall(const CallEvent &Call, if (State->get(ArgRegion)) return; - // Mark object as moved-from. - State = State->set(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(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_ostream &OS, const MemRegion *MR, 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 CallEvent &Call, CheckerContext &C) const { // 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(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(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(&Call); @@ -477,47 +503,14 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (!ThisRegion) return; + // The remaining part is check only for method call on a moved-from object. const auto MethodDecl = dyn_cast_or_null(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(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(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(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 CallEvent &Call, CheckerContext &C) const { return; } - // If it is already reported then we don't report the bug again. - const RegionState *ThisState = State->get(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(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, -- 2.40.0