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