[analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.

[NoQ]

Summary

I'd like to try to enable the use-after-move checker developed heroically by Peter "@szepet" Szecsi by default. While currently being a good opt-in lint check for enforcing coding style, currently it has too many false positives for an on-by-default check. The false positives seem to concentrate around objects that are fine to be used after they were moved from, even if no "state reset" method has been called on them, simply because their respective move-constructor/move-assignment leaves the object that is being moved in a well-specified state (eg., a null smart pointer or an empty set). This is not the case for STL objects, but it is the case for many custom objects.

I'll proceed to post a few patches and i'd love to hear your thoughts! The plan (of which this patch implements the first step) is as follows:

1. Rename the checker. In our tradition, "XChecker" usually means a checker that checks if "X" is used correctly, not a checker that finds "X". Eg., MallocChecker warns on misuse of malloc()/free(), but we don't call it "MisusedMallocChecker". The proposed name is "MoveChecker", i.e. checker that checks moves.
2. Address false positives under an on-by-default checker option. The proposed idea is to only warn by default in the following three cases:
   • If the object is a known STL object - because we know that it's left in unspecified state after move and we can memorize all state reset methods.
   • If the object is a local (or parameter) variable and hence there's no temptation to re-use the storage instead of making another local variable.
   • If the object is taken from a local (or parameter) rvalue reference, for the same reason.
3. Make warning messages more specific based information provided by facilities introduced on step 2.
4. Add a few missing state-reset methods.
5. Enable the checker, as restricted on step 2, by default. Keep the aggressive variant around in case someone needs it, but i don't have any specific plans for it.

[xazax.hun]

Looks good. Do we plan to detect problems other than use after move? Maybe it would be worth to synchronize with the tidy checker name use-after-move or is it going to cause more confusion?

[Szelethus]

LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there.

include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 def IteratorRangeChecker : Checker<"IteratorRange">,
   HelpText<"Check for iterators used outside their valid ranges">;
 
 def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
   HelpText<"Check for use of iterators of different containers where iterators "
            "of the same container are expected">;
 
-def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
-     HelpText<"Method calls on a moved-from object and copying a moved-from "
-              "object will be reported">;
+def MoveChecker: Checker<"Move">,
+     HelpText<"Find use-after-move bugs in C++">;
 
 def UninitializedObjectChecker: Checker<"UninitializedObject">,
      HelpText<"Reports uninitialized fields after object construction">;
 
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   LLVMConventionsChecker.cpp
   LocalizationChecker.cpp
   MacOSKeychainAPIChecker.cpp
   MacOSXAPIChecker.cpp
   MallocChecker.cpp
   MallocOverflowSecurityChecker.cpp
   MallocSizeofChecker.cpp
   MmapWriteExecChecker.cpp
-  MisusedMovedObjectChecker.cpp
+  MoveChecker.cpp
   MPI-Checker/MPIBugReporter.cpp
   MPI-Checker/MPIChecker.cpp
   MPI-Checker/MPIFunctionClassifier.cpp
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NonnullGlobalConstantsChecker.cpp

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

-// MisusedMovedObjectChecker.cpp - Check use of moved-from objects. - C++ -===//
+// MoveChecker.cpp - Check use of moved-from objects. - C++ ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
public:
 
   static RegionState getReported() { return RegionState(Reported); }
   static RegionState getMoved() { return RegionState(Moved); }
 
   bool operator==(const RegionState &X) const { return K == X.K; }
   void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
 };
 
-class MisusedMovedObjectChecker
+class MoveChecker
     : public Checker<check::PreCall, check::PostCall,
                      check::DeadSymbols, check::RegionChanges> {
 public:
   void checkPreCall(const CallEvent &MC, CheckerContext &C) const;
   void checkPostCall(const CallEvent &MC, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   ProgramStateRef
   checkRegionChanges(ProgramStateRef State,
static bool isAnyBaseRegionReported(ProgramStateRef State,
   for (auto &E : State->get<TrackedRegionMap>()) {
     if (Region->isSubRegionOf(E.first) && E.second.isReported())
       return true;
   }
   return false;
 }
 
 std::shared_ptr<PathDiagnosticPiece>
-MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
-                                                      BugReporterContext &BRC,
-                                                      BugReport &) {
+MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
+                                        BugReporterContext &BRC, BugReport &) {
   // We need only the last move of the reported object's region.
   // The visitor walks the ExplodedGraph backwards.
   if (Found)
     return nullptr;
   ProgramStateRef State = N->getState();
   ProgramStateRef StatePrev = N->getFirstPred()->getState();
   const RegionState *TrackedObject = State->get<TrackedRegionMap>(Region);
   const RegionState *TrackedObjectPrev =
else
     InfoText = "Became 'moved-from' here";
 
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
   return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
 }
 
-const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation(
-    const ExplodedNode *N, const MemRegion *Region, CheckerContext &C) const {
+const ExplodedNode *MoveChecker::getMoveLocation(const ExplodedNode *N,
+                                                 const MemRegion *Region,
+                                                 CheckerContext &C) const {
   // Walk the ExplodedGraph backwards and find the first node that referred to
   // the tracked region.
   const ExplodedNode *MoveNode = N;
 
   while (N) {
     ProgramStateRef State = N->getState();
     if (!State->get<TrackedRegionMap>(Region))
       break;
     MoveNode = N;
     N = N->pred_empty() ? nullptr : *(N->pred_begin());
   }
   return MoveNode;
 }
 
-ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region,
-                                                   const CallEvent &Call,
-                                                   CheckerContext &C,
-                                                   MisuseKind MK) const {
+ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
+                                     const CallEvent &Call, CheckerContext &C,
   if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!BT)
       BT.reset(new BugType(this, "Usage of a 'moved-from' object",
                            "C++ move semantics"));
 
     // Uniqueing report to the same object.
     PathDiagnosticLocation LocUsedForUniqueing;
     const ExplodedNode *MoveNode = getMoveLocation(N, Region, C);
auto R =
                                      MoveNode->getLocationContext()->getDecl());
     R->addVisitor(llvm::make_unique<MovedBugVisitor>(Region));
     C.emitReport(std::move(R));
     return N;
   }
   return nullptr;
 }
 
-void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call,
-                                              CheckerContext &C) const {
+void MoveChecker::checkPostCall(const CallEvent &Call,
   const auto *AFC = dyn_cast<AnyFunctionCall>(&Call);
   if (!AFC)
     return;
 
   ProgramStateRef State = C.getState();
   const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(AFC->getDecl());
   if (!MethodDecl)
     return;
void MoveChecker::checkPostCall(const CallEvent &Call,
 
   if (State->get<TrackedRegionMap>(ArgRegion))
     return;
   // Mark object as moved-from.
   State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getMoved());
   C.addTransition(State);
 }
 
-bool MisusedMovedObjectChecker::isMoveSafeMethod(
-    const CXXMethodDecl *MethodDec) const {
+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)
       return false;
     if (Tp->isBooleanType() || Tp->isVoidType() || Tp->isVoidPointerType())
       return true;
   }
   // Function call `empty` can be skipped.
   return (MethodDec && MethodDec->getDeclName().isIdentifier() &&
       (MethodDec->getName().lower() == "empty" ||
        MethodDec->getName().lower() == "isempty"));
 }
 
-bool MisusedMovedObjectChecker::isStateResetMethod(
-    const CXXMethodDecl *MethodDec) const {
+bool MoveChecker::isStateResetMethod(const CXXMethodDecl *MethodDec) const {
   if (!MethodDec)
       return false;
   if (MethodDec->hasAttr<ReinitializesAttr>())
       return true;
   if (MethodDec->getDeclName().isIdentifier()) {
     std::string MethodName = MethodDec->getName().lower();
     if (MethodName == "reset" || MethodName == "clear" ||
         MethodName == "destroy")
       return true;
   }
   return false;
 }
 
 // Don't report an error inside a move related operation.
 // We assume that the programmer knows what she does.
-bool MisusedMovedObjectChecker::isInMoveSafeContext(
-    const LocationContext *LC) const {
+bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const {
   do {
     const auto *CtxDec = LC->getDecl();
     auto *CtorDec = dyn_cast_or_null<CXXConstructorDecl>(CtxDec);
     auto *DtorDec = dyn_cast_or_null<CXXDestructorDecl>(CtxDec);
     auto *MethodDec = dyn_cast_or_null<CXXMethodDecl>(CtxDec);
     if (DtorDec || (CtorDec && CtorDec->isCopyOrMoveConstructor()) ||
         (MethodDec && MethodDec->isOverloadedOperator() &&
          MethodDec->getOverloadedOperator() == OO_Equal) ||
         isStateResetMethod(MethodDec) || isMoveSafeMethod(MethodDec))
       return true;
   } while ((LC = LC->getParent()));
   return false;
 }
 
-void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
-                                             CheckerContext &C) const {
+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.
void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
   if (isInMoveSafeContext(LC))
     return;
 
   N = reportBug(ThisRegion, Call, C, MK_FunCall);
   State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
   C.addTransition(State, N);
 }
 
-void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper,
-                                                 CheckerContext &C) const {
+void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   ProgramStateRef State = C.getState();
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
   for (TrackedRegionMapTy::value_type E : TrackedRegions) {
     const MemRegion *Region = E.first;
     bool IsRegDead = !SymReaper.isLiveRegion(Region);
 
     // Remove the dead regions from the region map.
     if (IsRegDead) {
       State = State->remove<TrackedRegionMap>(Region);
     }
   }
   C.addTransition(State);
 }
 
-ProgramStateRef MisusedMovedObjectChecker::checkRegionChanges(
+ProgramStateRef MoveChecker::checkRegionChanges(
     ProgramStateRef State, const InvalidatedSymbols *Invalidated,
     ArrayRef<const MemRegion *> ExplicitRegions,
     ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
     const CallEvent *Call) const {
   // In case of an InstanceCall don't remove the ThisRegion from the GDM since
   // it is handled in checkPreCall and checkPostCall.
   const MemRegion *ThisRegion = nullptr;
   if (const auto *IC = dyn_cast_or_null<CXXInstanceCall>(Call)) {
     ThisRegion = IC->getCXXThisVal().getAsRegion();
   }
 
   for (const auto *Region : ExplicitRegions) {
     if (ThisRegion != Region)
       State = removeFromState(State, Region);
   }
 
   return State;
 }
 
-void MisusedMovedObjectChecker::printState(raw_ostream &Out,
-                                           ProgramStateRef State,
-                                           const char *NL,
-                                           const char *Sep) const {
+void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State,
+                             const char *NL, const char *Sep) const {
 
   TrackedRegionMapTy RS = State->get<TrackedRegionMap>();
 
   if (!RS.isEmpty()) {
     Out << Sep << "Moved-from objects :" << NL;
     for (auto I: RS) {
       I.first->dumpToStream(Out);
       if (I.second.isMoved())
         Out << ": moved";
       else
         Out << ": moved and reported";
       Out << NL;
     }
   }
 }
-void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) {
-  mgr.registerChecker<MisusedMovedObjectChecker>();
+void ento::registerMoveChecker(CheckerManager &mgr) {
+  mgr.registerChecker<MoveChecker>();
 }

test/Analysis/use-after-move.cpp

-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify -analyzer-output=text -analyzer-config exploration_strategy=unexplored_first_queue,eagerly-assume=false %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -analyzer-config exploration_strategy=dfs,eagerly-assume=false -verify -analyzer-output=text -DDFS=1 %s
+// 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: %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
 
 namespace std {
 
 template <typename>
 struct remove_reference;
 
 template <typename _Tp>
 struct remove_reference { typedef _Tp type; };