StaticAnalyzer - MisusedMovedObjectChecker

[szepet]

Summary

A checker implementation which checks for various cases when a moved-from object is potentially misused.
That means That means method calls on the object or copying it in moved-from state.

[szepet]

removed empty file which was added by mistake

[xazax.hun]

We should compare this patch with the just released tidy checker: https://github.com/llvm-mirror/clang-tools-extra/commit/5434827564c9c3741c63b4dc368b37a630857e29

[zaks.anna]

The clang-tidy check that Gabor has pointed to has a lot of great test cases!

The main differences I see are:

• It would be much easier to write a static analyzer check for this since the infrastructure for reasoning about ordered events is already in place.
• I suspect one of the examples the clang-tidy check will not handle well is something like this:
  
  A a(42, 42.0); A b; if (cond) b = std::move(a); if (!cond) a.foo();

This is a classical example where path sensitivity is needed not to report a false positive.

• I am guessing the check is intra-procedural as well.

(It would be great if someone could confirm these are actually true:))

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

32

nit: Please define MoveStmt before the constructor.

119

Please, add text diagnostic tests for the bug visitor.

139

When is this condition hit? Please, add a test case.

149

nit: Would be good to special case and capitalize the case where the ObjectName is not known.

179

"Unspecified usage of a 'moved-from' object" -> "Usage of a 'moved-from' object"

180

For consistency with other category names, how about "C++ move semantics"?

257

Why is this necessary? Why using checkPointerEscape is not enough?

test/Analysis/MisusedMovedObject.cpp

49

Adding "//no-warning" on lines where you explicitly test that a warning is not produced will simplify with readability and maintainability.

108

How about improving the wording a bit: "Method call on a 'moved-from' object 'a'". Note, all diagnostic messages should be capitalized.

[szepet]

• Some refactor based on comments.
• Updated testfiles.

I checked the tidy checker and I can confirm Anna's statements. To get in details:

• tidy checker is intraprocedural, so doesn't find the interprocedural case in this testfile (and false positives on right reference calls)
• mutually exclusive conditions in if statements results false positive
• In addition, because of no semantic check it does not find the pointer and array examples in this testcase - false negativ

• However it can find the misuse if it comes from the parameter evaluation order, for example:

foobar(std::move(a), a.getI()); //both checker find the problem
foobar(a.getI(), std::move(a)); //only tidy report a warning

That is the case where we have a false negative.

[szepet]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

257

Based on my tests,checkPointerEscape works on only SymRegion but we needed some more generic approach. (In connection with MemRegions.)
It seems kind of hacking it into this checker and it's probably exactly that but I had no other ideas to implement it.

[NoQ]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

257

As far as i understand, at a glance, this code invalidates the program state trait for objects that have escaped into a function.

This makes me think you should be taking check::RegionChanges here. See CStringChecker as an example. This callback would save you a large amount of code by presenting a list of superregions that cover all invalidated regions (well, you can't list all of them). It automatically handles regions pointed to by contents of invalidated regions etc., which you'd still want to handle.

check::PointerEscape is truly not enough because it handles only symbolic pointers, while you are also interested in concrete pointers (such as VarRegion).

There's a bit of a mess with these callbacks which we're trying to sort out (http://lists.llvm.org/pipermail/cfe-dev/2016-September/050792.html), but it shouldn't affect you.

[szepet]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

257

I have tried this callback on some trivial examples.
It seems it solves this problem (function calls with no body), however, in cases when a constructor /destructor called, the callback provides only the members regions in the ExplicitRegions. In case of destructor the Regions contains the element's region, but unfortunately it is not always right to iterate over the whole Regions array.
For example: Destructor calls on an array element would cause invalidating the whole array.

So this callback can help on this one, and it would be structured invalidating instead of this hacking. However, the invalidating in the checkPreCall still needed. (constructorcall, destructorcall...)

Am I right or did I miss/misunderstand something?

[NoQ]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

257

Hmm, i think i want to know more about that. Could you post a version of the patch with checkRegionChanges and the failing test, which you are trying, in case you already have it?

Destructor calls on an array element would cause invalidating the whole array.

Yyeeeah, that's right, we're always invalidating the whole base region. Essentially, you can get from one array element to another array element with a reliable pointer arithmetic (eg. "this - 1" in your case). And i agree that for your case this ultraconservative invalidation is definitely worth lifting.

in cases when a constructor /destructor called, the callback provides only the members regions in the ExplicitRegions

That's probably because the constructor/destructor was inlined, otherwise how would he know what members are there? In this case, there is actually no invalidation, just separate callbacks for every store bind of every member. And in this case...

However, the invalidating in the checkPreCall still needed. (constructorcall, destructorcall...)

... in this case, yeah, this is definitely needed. This is private logic of your checker, the analyzer couldn't have guessed that a perfectly inlined call, without any invalidations, may have such special meaning for your checker.

However, if the constructor/destructor wasn't inlined, then i expect the object to be present in explicit regions. If it isn't there, it's probably worth adding on the core side rather than hacking in checkers.

So in my opinion this leaves us with standard checkRegionChanges and special code for inlined constructors/destructors.

263

By the way, we have C.wasInlined, would it be more useful/accurate here?

[zaks.anna]

Please, add the tests from test/clang-tidy/misc-use-after-move.cpp so that we have better test coverage.

[zaks.anna]

Peter, Do you plan on addressing the review comments or can we commander the revision?

[szepet]

I wasn't able to find time for this checker the last few weeks, but my schedule became much lighter and I plan to commit the required changes (and a bugfix) next week. Starting from next week I can work on the checker on a daily basis. Hopefully it can make it into 4.0.

[szepet]

Test cases updated.
Some invalidation moved to checkRegionChanges callback.
Missing feature added: do not report an SubRegion if any of its SuperRegion is already reported

[NoQ]

Thanks for looking into the region-changes approach, and sorry for the delay in replies.
I think we're close to seeing this checker merged, as it looks great.
A few smaller comments.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

155

Could we use the static PathDiagnosticLocation::getStmt(N) function for obtaining S? That'd be the statement of the program point of the node. This way we don't need to store the statement in the program state. Or are you looking for a different statement?

231

Hmm. Is this not caught by checkDeadSymbols? I expect parameter regions to die eventually. It should not only catch regions themselves, but also regions to which they contain pointers.

479

I definitely admire this trick. However, it only works when the object from which we move is "opaque" enough: for example, if at least one body of its setter methods is available in the current translation unit, we could easily see an arbitrary symbol inside the object (not necessarily having this object as its origin region, probably reassigned from other regions or even conjured), and the code will have no effect.

[szepet]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

231

Lets consider the next case:

struct C {};

void f(C c)
{
  C b = std::move(c);
 //...
}

void g()
{
  C c;
  for(int i = 0;i<2;i++)
  {
    f(c);
  }
}

During the analysis of g() the checker will report that an already moved object will be copied. (Since it is passed by value).
So the main problem was these for loops and resulted false positives as well when i ran the checker on the llvm codebase.
The problem can be come from that tha analyzer doesnt see any difference between the call since not just the parameter but their location are the same and that cause the problem. This is reason why came up with that not so elegant solution.

Please correct me if I'm wrong or let me know in case you know a solution for that but I was not able to write checkDeadSymbols callback to handle that.

479

Well I planned to remove this callback and only remained in the code by mistake. As I understand it is unnecessary since checkRegionChanges will handle these cases, I needed it just before it was implemented. So yes, you are right but this callback can (and should) be removed.

Do you agree?

[szepet]

CheckPointerEscape callback removed since checkRegionChanges handle all the necessary cases.
Member MoveStmt removed from RegionState.
+Some typo fixes in comments.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

155

No, you are right. First I tried to compare the RegionStates so I the statements for operator== but then the code changed and I havent refactored this part.

[NoQ]

This checker would need a bit of rebase before landing, but i think it's almost good to land. I'd even consider enabling by default after some evaluation, unless there are many known false positives - with an awesome test suite and very detailed bugreports, i think it's quite worth it.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

2

This line needs some care (alignment, maybe a short description).

231

Hmm. Debugged this a bit.

Well, they are different objects. c from the old call and c from the new call are different objects, they may even have different addresses on the stack.

The analyzer should normally be noticing the difference between these two objects and represent them with different memory regions. Even though they're represented by VarRegions with same VarDecls, these regions should differ by having different super-regions, namely different StackArgumentsSpaceRegion instances, which, in turn, are different because they are built over different StackFrameContext instances.

But unfortunately in this case the StackFrameContext is the same, because it doesn't include the call site block count! So as long as the call site's statement is the same on subsequent calls, such as in the loop, we run into this problem.

This should be fixed on StackFrameContext side. D19979 should have addressed that, but it ran into problems and doesn't seem to be landing soon. For now, i guess i'd try to add a block count directly to StackFrameContext when i have time, it should be trivial. Then your checkEndFunction callback will be unnecessary.

I'm still curious why checkDeadSymbols doesn't kill these map items, but i guess we already have a bigger problem to tackle.

Could you add this test to your test set for now?

[szepet]

Small infotext added and test cases updated. (More loop test to make sure the checkEndFunction workaround works fine.)

[xazax.hun]

There is a CERT C++ Secure Coding Standard rule that is very similar to this check. I think you should mention this rule somewhere in the documentation or comments (but do not imply that this check conforms to that rule since it is not guaranteed that it finds all those issues).

The description of the rule also contains several useful exceptions that you can check whether your checker already has implemented: https://www.securecoding.cert.org/confluence/display/cplusplus/EXP63-CPP.+Do+not+rely+on+the+value+of+a+moved-from+object

[NoQ]

I believe this checker is great and should be merged. Do you have commit access? If not, could you rebase the checker to the top of trunk? because the patch doesn't apply cleanly anymore.

[zaks.anna]

Great to see this converging? What is left before you would recommend turning this on by default?
You mention evaluating this on LLVM codebase. What were the results?

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

231

But unfortunately in this case the StackFrameContext is the same, because it doesn't include the call site block count! So as long as the call site's statement is the same on subsequent calls, such as in the loop, we run into this problem.

This sounds scary! Can you file a bug/PR about it with a reduced test case.
Can we refer to the PR here if we think this code should change once the PR is fixed?

479

Has this been resolved?

test/Analysis/MisusedMovedObject.cpp

231

Can you please split these notes so that each one is on a separate line? You can use // expected-note@+1 to show that there is 1 line difference between the locations.

[NoQ]

Hello,

I tested this checker on WebKit code. It seems to work as expected, and i liked the support for WTFMove(). However i suddenly realized that it is not always unsafe to use a moved-from object, as discussed in detail at, for example, http://stackoverflow.com/a/7028318 . The checker already includes a state-reset method list, however there are also methods that are generally safe to call and possibly useful (such as .size()) or methods that don't entirely reset the state but affect it (.resize()?). People may use these tricks to improve performance. We shouldn't flag these tricks in a checker that is enabled by default, because it'd make people change and regress working code.

I guess the solution is to come up with more detailed heuristics to figure out which methods are safe and which aren't - either larger lists of methods, or regex-based analysis over method names. Would you be willing to come up with those in a follow-up patch? We may also have a look eventually, because we believe this checker is useful :)

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

231

Filed: http://bugs.llvm.org/show_bug.cgi?id=32163

Also recalled that fixing StackFrameContext is not enough to get rid of this problem entirely, because the inner function call is not bound to exist.

479

Yep.

[szepet]

Done in this diff:

• branch rebase
• check comments in the test file separated to lines (only notes with the same messages are in the same line)

I have runned the checker on the LLVM source, it can be viewed here: http://cc.elte.hu:15001/#run=1 .
This checker reported 44 bugs, but let me summarize them:

• 22 bug from the same location (/mnt/storage/szepet/dev/CodeChecker/llvm/include/llvm/Support/Error.h:841) which is a false positive and it was resulted by not tracking the bitfields (bool Unchecked : 1; ).
• 6 from the toy.cpp-s and they are a true positve in my opinion (http://cc.elte.hu:15001/#run=1&report=351)
• 5 from the test files where some specific class's function was tested in moved state (e.g. http://cc.elte.hu:15001/#run=1&report=48)
• 11 various report but the checker worked as expected (it could be 4 less with better heuristics but probably it would be better to commit the heuristics in a follow up patch)

[szepet]

I guess the solution is to come up with more detailed heuristics to figure out which methods are safe and which aren't - either larger lists of methods, or regex-based analysis over method names. Would you be willing to come up with those in a follow-up patch? We may also have a look eventually, because we believe this checker is useful :)

Yeah, for sure I would really like to work on that! Once this is accepted I will start to figure out some heuristics and experiment with them.

[xazax.hun]

I only found two nits. In case you think this is ready to commit, let me know.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

3

Nit, it looks like the description is too long and the end of the first line is moved to the second.

test/Analysis/MisusedMovedObject.cpp

2

Nit: due to the upcomming Z3 support (as a constraint solver), you should use %clang_analyze_cc1 instead of %clang_cc1

[szepet]

Nit resolved.

[NoQ]

I committed the checker. Thanks / congrats / all the stuff!

I've also been thinking that when doing move-after-move, we should probably say "Moving a 'moved-from' object..." rather than "Copying...".

include/clang/StaticAnalyzer/Checkers/Checkers.td

 } // end: "cplusplus"
 
 let ParentPackage = CplusplusAlpha in {
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
+def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
+     HelpText<"Method calls on a moved-from object and copying a moved-from "
+              "object will be reported">,
+     DescFile<"MisusedMovedObject.cpp">;
+
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//
 // Valist checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = ValistAlpha in {

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   GenericTaintChecker.cpp
   IdenticalExprChecker.cpp
+  MisusedMovedObjectChecker.cpp
   IvarInvalidationChecker.cpp
   LLVMConventionsChecker.cpp
   LocalizationChecker.cpp
   MacOSKeychainAPIChecker.cpp
   MacOSXAPIChecker.cpp
   MallocChecker.cpp
   MallocOverflowSecurityChecker.cpp
   MallocSizeofChecker.cpp

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

+//== MisusedMovedObjectChecker --------------*- C++ -*--==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines checker which checks for potential misuses of a moved-from
+// object. That means method calls on the object or copying it in moved-from
+// state.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+struct RegionState {
+private:
+  enum Kind { Moved, Reported } K;
+  RegionState(Kind InK, const Stmt *S) : K(InK), MoveStmt(S) {}
+  const Stmt *MoveStmt;
+
+public:
+  bool isReported() const { return K == Reported; }
+  bool isMoved() const { return K == Moved; }
+
+  static RegionState getReported(const Stmt *S = nullptr) {
+    return RegionState(Reported, S);
+  }
+  static RegionState getMoved(const Stmt *S = nullptr) {
+    return RegionState(Moved, S);
+  }
+
+  bool operator==(const RegionState &X) const {
+    return K == X.K && MoveStmt == X.MoveStmt;
+  }
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddInteger(K);
+    ID.AddPointer(MoveStmt);
+  }
+  const Stmt *getMoveStmt() const { return MoveStmt; }
+};
+
+class MisusedMovedObjectChecker
+    : public Checker<check::PreCall, check::PostCall, check::EndFunction,
+                     check::DeadSymbols, check::PointerEscape> {
+public:
+  void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
+  void checkPreCall(const CallEvent &MC, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &MC, CheckerContext &C) const;
+  void checkEndFunction(CheckerContext &C) const;
+  ProgramStateRef checkPointerEscape(ProgramStateRef State,
+                                     const InvalidatedSymbols &Escaped,
+                                     const CallEvent *Call,
+                                     PointerEscapeKind Kind) const;
+
+private:
+  class MovedBugVisitor : public BugReporterVisitorImpl<MovedBugVisitor> {
+  public:
+    MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {}
+
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      static int X = 0;
+      ID.AddPointer(&X);
+      ID.AddPointer(Region);
+    }
+
+    PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+                                   const ExplodedNode *PrevN,
+                                   BugReporterContext &BRC,
+                                   BugReport &BR) override;
+
+  private:
+    // The tracked region.
+    const MemRegion *Region;
+    bool Found;
+  };
+
+  mutable std::unique_ptr<BugType> BT;
+  ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call,
+                          CheckerContext &C, bool isCopy) 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
+
+REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState)
+
+// If a region is removed all of the subregions needs to be removed too.
+static ProgramStateRef removeFromState(ProgramStateRef State,
+                                       const MemRegion *Region) {
+  if (!Region)
+    return State;
+  // Note: The isSubRegionOf is not reflexive.
+  State = State->remove<TrackedRegionMap>(Region);
+  for (auto &E : State->get<TrackedRegionMap>()) {
+    if (E.first->isSubRegionOf(Region))
+      State = State->remove<TrackedRegionMap>(E.first);
+  }
+  return State;
+}
+
+PathDiagnosticPiece *MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(
+    const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
+    BugReport &BR) {
+  // 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 = PrevN->getState();
+  const RegionState *TrackedObject = State->get<TrackedRegionMap>(Region);
+  const RegionState *TrackedObjectPrev =
+      StatePrev->get<TrackedRegionMap>(Region);
+  if (!TrackedObject)
+    return nullptr;
+  if (TrackedObjectPrev && TrackedObject)
+    return nullptr;
+
+  // Retrieve the associated statement.
+  const Stmt *S = TrackedObject->getMoveStmt();
+  if (!S)
+    S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+  Found = true;
+
+  std::string ObjectName;
+  if (const auto DecReg = Region->getAs<DeclRegion>()) {
+    const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl());
+    ObjectName = RegionDecl->getNameAsString() + " ";
+  }
+  std::string InfoText = ObjectName + "became moved-from here";
+
+  // Generate the extra diagnostic.
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                             N->getLocationContext());
+  return new PathDiagnosticEventPiece(Pos, InfoText, true, nullptr);
+}
+
+const ExplodedNode *MisusedMovedObjectChecker::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,
+                                                   bool isCopy = false) const {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
+    if (!BT)
+      BT.reset(new BugType(this, "Unspecified usage of a 'moved-from' object",
+                           "CXXMoveSemantics"));
+
+    // Uniqueing report to the same object.
+    PathDiagnosticLocation LocUsedForUniqueing;
+    const ExplodedNode *MoveNode = getMoveLocation(N, Region, C);
+
+    if (const Stmt *MoveStmt = PathDiagnosticLocation::getStmt(MoveNode))
+      LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+          MoveStmt, C.getSourceManager(), MoveNode->getLocationContext());
+
+    // Creating the error message.
+    std::string ErrorMessage;
+    if (isCopy)
+      ErrorMessage = "copying a 'moved-from' object";
+    else
+      ErrorMessage = "method call on a 'moved-from' object";
+    if (const auto DecReg = Region->getAs<DeclRegion>()) {
+      const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl());
+      ErrorMessage += ", named: " + RegionDecl->getNameAsString();
+    }
+
+    auto R =
+        llvm::make_unique<BugReport>(*BT, ErrorMessage, N, LocUsedForUniqueing,
+                                     MoveNode->getLocationContext()->getDecl());
+    R->addVisitor(llvm::make_unique<MovedBugVisitor>(Region));
+    C.emitReport(std::move(R));
+    return N;
+  }
+  return nullptr;
+}
+
+// Removing the function parameters' MemRegion from the state. This is needed
+// for PODs where the trivial destructor does not even created nor executed.
+void MisusedMovedObjectChecker::checkEndFunction(CheckerContext &C) const {
+  auto State = C.getState();
+  TrackedRegionMapTy Objects = State->get<TrackedRegionMap>();
+  if (Objects.isEmpty())
+    return;
+
+  auto LC = C.getLocationContext();
+
+  const auto LD = dyn_cast_or_null<FunctionDecl>(LC->getDecl());
+  if (!LD)
+    return;
+  llvm::SmallSet<const MemRegion *, 8> InvalidRegions;
+
+  for (auto Param : LD->parameters()) {
+    auto Type = Param->getType().getTypePtrOrNull();
+    if (!Type)
+      continue;
+    if (!Type->isPointerType() && !Type->isReferenceType()) {
+      InvalidRegions.insert(State->getLValue(Param, LC).getAsRegion());
+    }
+  }
+
+  if (InvalidRegions.empty())
+    return;
+
+  for (const auto &E : State->get<TrackedRegionMap>()) {
+    if (InvalidRegions.count(E.first->getBaseRegion()))
+      State = State->remove<TrackedRegionMap>(E.first);
+  }
+
+  C.addTransition(State);
+}
+
+void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call,
+                                              CheckerContext &C) const {
+  const auto *AFC = dyn_cast<AnyFunctionCall>(&Call);
+  if (!AFC)
+    return;
+
+  ProgramStateRef State = C.getState();
+  const auto FunDecl = AFC->getDecl();
+  if (!FunDecl)
+    return;
+
+  // Whenever returning from a function with an unknown definition invalidate
+  // the objects' region which was passed by reference or as a pointer.
+  ExplodedNode *N = nullptr;
+  if (!FunDecl->getBody()) {
+    unsigned NumOfArgs = FunDecl->getNumParams();
+    unsigned Idx = 0;
+    for (FunctionDecl::param_const_iterator I = FunDecl->param_begin(),
+                                            E = FunDecl->param_end();
+         I != E && Idx < NumOfArgs; ++I, ++Idx) {
+
+      ParmVarDecl *Param = *I;
+      if (Param->isParameterPack())
+        break;
+      const Type *Tp = Param->getType().getTypePtrOrNull();
+      if (!Tp)
+        continue;
+
+      if ((Tp->isPointerType() || Tp->isReferenceType()) &&
+          !Tp->getPointeeType().isConstQualified()) {
+        State = removeFromState(State, AFC->getArgSVal(Idx).getAsRegion());
+      }
+    }
+    N = C.addTransition(State);
+  }
+
+  // In system header we assume the object is not misused.
+  if (auto LocationDecl = Call.getLocationContext()->getDecl()) {
+    if (C.getSourceManager().isInSystemHeader(LocationDecl->getLocation()))
+      return;
+  }
+  const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(FunDecl);
+  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 assignement operator or
+  // move constructor .
+  if (ConstructorDecl && !ConstructorDecl->isMoveConstructor())
+    return;
+
+  if (!ConstructorDecl && !MethodDecl->isMoveAssignmentOperator())
+    return;
+
+  const auto ArgRegion = AFC->getArgSVal(0).getAsRegion();
+  if (!ArgRegion)
+    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;
+  // Mark object as moved-from.
+  State = State->set<TrackedRegionMap>(
+      ArgRegion, RegionState::getMoved(AFC->getOriginExpr()));
+
+  C.addTransition(State, N);
+}
+
+bool MisusedMovedObjectChecker::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;
+  }
+
+  return false;
+}
+
+bool MisusedMovedObjectChecker::isStateResetMethod(
+    const CXXMethodDecl *MethodDec) const {
+  if (MethodDec && MethodDec->getDeclName().isIdentifier()) {
+    StringRef MethodName;
+    MethodName = MethodDec->getName();
+    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 {
+  do {
+    StringRef MethodName;
+    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->isMoveConstructor()) ||
+        (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 {
+  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 assignement
+  // 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)) {
+          N = reportBug(ArgRegion, Call, C, /*isCopy=*/true);
+          State = State->set<TrackedRegionMap>(ArgRegion,
+                                               RegionState::getReported());
+        }
+      }
+    }
+    C.addTransition(State, N);
+    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 (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) {
+    State = removeFromState(State, IC->getCXXThisVal().getAsRegion());
+    C.addTransition(State);
+    return;
+  }
+
+  const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
+  if (!MethodDecl)
+    return;
+  // Checking assignement operators.
+  bool OperatorEq = MethodDecl->isOverloadedOperator() &&
+                    MethodDecl->getOverloadedOperator() == OO_Equal;
+  // Remove the tracked object for every assignement operator, but report bug
+  // only for move or copy assignement'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();
+        N = reportBug(ArgRegion, Call, C, /*isCopy=*/true);
+        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.
+  if (isMoveSafeMethod(MethodDecl))
+    return;
+
+  if (isStateResetMethod(MethodDecl)) {
+    State = State->remove<TrackedRegionMap>(ThisRegion);
+    C.addTransition(State);
+    return;
+  }
+
+  // If it is already reported then we dont report the bug again.
+  const RegionState *ThisState = State->get<TrackedRegionMap>(ThisRegion);
+  if (!(ThisState && ThisState->isMoved()))
+    return;
+
+  if (isInMoveSafeContext(LC))
+    return;
+
+  N = reportBug(ThisRegion, Call, C);
+  State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
+  C.addTransition(State, N);
+}
+
+void MisusedMovedObjectChecker::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;
+    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::checkPointerEscape(
+    ProgramStateRef State, const InvalidatedSymbols &Escaped,
+    const CallEvent *Call, PointerEscapeKind Kind) const {
+  for (InvalidatedSymbols::const_iterator I = Escaped.begin(),
+                                          E = Escaped.end();
+       I != E; ++I) {
+    SymbolRef Sym = *I;
+    const auto *Region = Sym->getOriginRegion();
+    State = removeFromState(State, Region);
+  }
+  return State;
+}
+
+void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) {
+  mgr.registerChecker<MisusedMovedObjectChecker>();
+}

test/Analysis/MisusedMovedObject.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify %s
+
+namespace std {
+
+template <typename>
+struct remove_reference;
+
+template <typename _Tp>
+struct remove_reference { typedef _Tp type; };
+
+template <typename _Tp>
+struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template <typename _Tp>
+struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template <typename _Tp>
+typename remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast<typename remove_reference<_Tp>::type &&>(__t);
+}
+
+template <typename _Tp>
+_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
+} // namespace std
+
+class B {
+public:
+  B() = default;
+  B(const B &) = default;
+  B(B &&) = default;
+  void operator=(B &&b) {
+    return;
+  }
+  void foo() { return; }
+};
+
+class A {
+  int i;
+  double d;
+  B b;
+
+public:
+  A(int ii = 0, double dd = 0) : d(dd), i(ii) {}
+  void moveconstruct(A &&other) {
+    b = std::move(other.b);
+    d = other.d;
+    i = other.i;
+    return;
+  }
+
+  static A get() {
+    A v(12, 13);
+    return v;
+  }
+  A(A *a) {
+    moveconstruct(std::move(*a));
+  }
+  A(const A &other) : i(other.i), d(other.d), b(other.b) {}
+  A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) {
+  }
+  A(A &&other, char *k) {
+    moveconstruct(std::move(other));
+  }
+  void operator=(A &&other) {
+    moveconstruct(std::move(other));
+    return;
+  }
+  int foo() const { return 42; }
+  int bar() const { return 43; }
+  void reset() {}
+};
+
+int bignum();
+
+void leftRefCall(A &a) {
+  a.foo();
+}
+void rightRefCall(A &&a) {
+  a.foo();
+}
+void copyOrMoveCall(const A a) {
+  a.foo();
+}
+
+void looptest() {
+  A Var;
+  for (int i = 0; i < 2; i++) {
+    rightRefCall(std::move(Var));
+  }
+  for (int i = 0; i < 2; i++) {
+    leftRefCall(Var);
+  }
+  for (int i = 0; i < 2; i++) {
+    copyOrMoveCall(Var);
+  }
+  for (int i = 0; i < 2; i++) {
+    copyOrMoveCall(std::move(Var)); // expected-warning{{copying a 'moved-from' object, named: Var}}
+  }
+}
+
+void uniqueTest() {
+  A a(42, 42.0);
+  A b = std::move(a);
+  if (bignum()) {
+    a.foo(); // expected-warning{{method call on a 'moved-from' object, named: a}}
+  } else {
+    a.bar();
+  }
+  a.bar();
+}
+
+void PtrAndArrayTest() {
+  A *Ptr = new A(1, 1.5);
+  A Arr[10];
+  Arr[2] = std::move(*Ptr);
+  (*Ptr).foo(); // expected-warning{{method call on a 'moved-from' object}}
+  Ptr = &Arr[1];
+  Arr[3] = std::move(Arr[1]);
+  Ptr->foo(); // expected-warning{{method call on a 'moved-from' object}}
+  Arr[3] = std::move(Arr[2]);
+  Arr[2].foo(); // expected-warning{{method call on a 'moved-from' object}}
+  Arr[2] = std::move(Arr[3]);
+  Arr[2].foo(); // no warning, since new value is assigned
+}
+
+void tempTest() {
+  A a = A::get();
+  A::get().foo();
+  for (int i = 0; i < bignum(); i++) {
+    A::get().foo();
+  }
+}