[analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[NoQ]

Summary

This is a no-functional-change part of the next patch.

The checker supports 5 different sorts of "use" after move: copying, moving, copy-assigning from, move-assigning from, calling any other method. De-duplicate the common checks that repeat 5 times into a single method, checkUse().

Re-structure the code a little bit.

[Szelethus]

Woohoo! Thanks!

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

515–516

How about MemRegion::getMostDerivedObjectRegion()? @baloghadamsoftware recently commited it,

[NoQ]

Fxd.

[a_sidorin]

Hi Artem,
The overall idea is good but I have some remarks inline.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

264

I think that if the function is named "checkUse()", committing State changes is not what is really expected from it. Should we rename it or change the logic somehow? For example, return true if a report was emitted and add transition in the caller?

271

This formatting is different from what clang-format does.

341

Should we move RD closer to its first use?

489

MisuseKind MK = CtorDecl->isMoveConstructor() ? MK_Move : MK_Copy;
checkUse(State, ArgRegion, RD, MK, C);

?

[NoQ]

Fxd.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

264

Good point. Renamed to modelUse() because the addTransition() logic becomes more complicated in the next patch, so i didn't want to duplicate it on all those call sites.

271

This gets overwritten in the next patch anyway. But imho this is more fancy than what clang-format does.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

const llvm::StringSet<> StandardMoveSafeClasses = {
       "shared_lock",
       "shared_ptr",
       "thread",
       "unique_ptr",
       "unique_lock",
       "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;
 
   // Classifies the object and dumps a user-friendly description string to
   // the stream. Return value is equivalent to classifyObject.
   ObjectKind explainObject(llvm::raw_ostream &OS,
                            const MemRegion *MR, const CXXRecordDecl *RD) const;
private:
 
   bool IsAggressive = false;
 
 public:
   void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; }
 
 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 checkUse(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;
   const ExplodedNode *getMoveLocation(const ExplodedNode *N,
                                       const MemRegion *Region,
                                       CheckerContext &C) const;
 };
 } // end anonymous namespace
while (N) {
     if (!State->get<TrackedRegionMap>(Region))
       break;
     MoveNode = N;
     N = N->pred_empty() ? nullptr : *(N->pred_begin());
   }
   return MoveNode;
 }
 
+void MoveChecker::checkUse(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,
                                      MisuseKind MK) const {
   if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!BT)
       BT.reset(new BugType(this, "Use-after-move",
                            "C++ move semantics"));
void MoveChecker::checkPostCall(const CallEvent &Call,
   if (!AFC)
     return;
 
   ProgramStateRef State = C.getState();
   const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(AFC->getDecl());
   if (!MethodDecl)
     return;
 
+  const CXXRecordDecl *RD = MethodDecl->getParent();
+
   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 .
   if (ConstructorDecl && !ConstructorDecl->isMoveConstructor())
     return;
 
   if (!ConstructorDecl && !MethodDecl->isMoveAssignmentOperator())
     return;
 
   const auto ArgRegion = AFC->getArgSVal(0).getAsRegion();
   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;
   if (const auto *IC = dyn_cast<CXXInstanceCall>(AFC))
     if (IC->getCXXThisVal().getAsRegion() == ArgRegion)
       return;
 
   const MemRegion *BaseRegion = ArgRegion->getBaseRegion();
   // Skip temp objects because of their short lifetime.
   if (BaseRegion->getAs<CXXTempObjectRegion>() ||
       AFC->getArgExpr(0)->isRValue())
     return;
   // If it has already been reported do not need to modify the state.
 
   if (State->get<TrackedRegionMap>(ArgRegion))
     return;
+
+  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 {
   // We abandon the cases where bool/void/void* conversion happens.
   if (const auto *ConversionDec =
           dyn_cast_or_null<CXXConversionDecl>(MethodDec)) {
     const Type *Tp = ConversionDec->getConversionType().getTypePtrOrNull();
     if (!Tp)
MoveChecker::explainObject(llvm::raw_ostream &OS, const MemRegion *MR,
   if (OK.STL) {
     OS << " of type '" << RD->getQualifiedNameAsString() << "'";
   }
   return OK;
 }
 
 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.
 
   // Checking constructor calls.
   if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
     State = removeFromState(State, CC->getCXXThisVal().getAsRegion());
     auto CtorDec = CC->getDecl();
     // 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());
-        }
-      }
-    }
-    C.addTransition(State, N);
+      if (CtorDec->isMoveConstructor())
+        checkUse(State, ArgRegion, RD, MK_Move, C);
+        checkUse(State, ArgRegion, RD, MK_Copy, C);
       return;
     }
+  }
 
   // Calling a destructor on a moved object is fine.
   if (isa<CXXDestructorCall>(&Call))
     return;
 
   const auto IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
-  // In case of destructor call we do not track the object anymore.
+
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   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;
 
+  // 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 (isStateResetMethod(MethodDecl)) {
+    State = removeFromState(State, ThisRegion);
+    C.addTransition(State);
+    return;
+  }
+
+  if (isMoveSafeMethod(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;
+  if (MethodDecl->isOverloadedOperator()) {
+    OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
+
+    if (OOK == 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);
+        if (MethodDecl->isMoveAssignmentOperator())
+          checkUse(State, ArgRegion, RD, MK_Move, C);
         else
-          N = reportBug(ArgRegion, RD, C, MK_Copy);
-        State =
-            State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported());
-      }
-    }
-    C.addTransition(State, N);
+          checkUse(State, ArgRegion, RD, MK_Copy, C);
         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;
-
-  if (isStateResetMethod(MethodDecl)) {
-    State = removeFromState(State, ThisRegion);
       C.addTransition(State);
       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()))
-    return;
-
-  // Don't report it in case if any base region is already reported
-  if (isAnyBaseRegionReported(State, ThisRegion))
-    return;
-
-  if (isInMoveSafeContext(LC))
-    return;
-
-  N = reportBug(ThisRegion, RD, C, MK_FunCall);
-  State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
-  C.addTransition(State, N);
+  checkUse(State, ThisRegion, RD, MK_FunCall, C);
 }
 
 void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                    CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
   for (TrackedRegionMapTy::value_type E : TrackedRegions) {
     const MemRegion *Region = E.first;