[analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling...

An earlier solution from Artem r315301 solves the reset problem, however, the
reports should be handled the same way in case of method calls. We should not
just report the base class of the object where the method was defined but the
whole object.

Fixed false positive which came from not removing the subobjects in case of a
state-resetting function. (Just replaced the State->remove(...) call to
removeFromState(..) which was defined exactly for that purpose.)

Some minor typos fixed in this patch as well which did not worth a whole new
patch in my opinion, so included them here.

Differential Revision: https://reviews.llvm.org/D31538

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@316850 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
index 4b69a1bd1edb243d015351c5f58f6e296c319535..9d405cd39c584bf79ccf232df714e7fa1a03c4a4 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
@@ -352,7 +352,7 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
   const LocationContext *LC = C.getLocationContext();
   ExplodedNode *N = nullptr;
 
-  // Remove the MemRegions from the map on which a ctor/dtor call or assignement
+  // Remove the MemRegions from the map on which a ctor/dtor call or assignment
   // happened.
 
   // Checking constructor calls.
@@ -380,8 +380,11 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
     return;
   // In case of destructor call we do not track the object anymore.
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return;
+
   if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) {
-    State = removeFromState(State, IC->getCXXThisVal().getAsRegion());
+    State = removeFromState(State, ThisRegion);
     C.addTransition(State);
     return;
   }
@@ -412,28 +415,28 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
   }
 
   // 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 CXXBaseObjectRegion *BR =
+             dyn_cast<CXXBaseObjectRegion>(ThisRegion))
+    ThisRegion = BR->getSuperRegion();
+
   if (isMoveSafeMethod(MethodDecl))
     return;
 
   if (isStateResetMethod(MethodDecl)) {
-    // A state reset method resets the whole object, not only sub-object
-    // of a parent class in which it is defined.
-    const MemRegion *WholeObjectRegion = ThisRegion;
-    while (const CXXBaseObjectRegion *BR =
-               dyn_cast<CXXBaseObjectRegion>(WholeObjectRegion))
-      WholeObjectRegion = BR->getSuperRegion();
-
-    State = State->remove<TrackedRegionMap>(WholeObjectRegion);
+    State = removeFromState(State, ThisRegion);
     C.addTransition(State);
     return;
   }
 
-  // If it is already reported then we dont report the bug again.
+  // 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;
 
-  // Dont report it in case if any base region is already reported
+  // Don't report it in case if any base region is already reported
   if (isAnyBaseRegionReported(State, ThisRegion))
     return;
 

diff --git a/test/Analysis/MisusedMovedObject.cpp b/test/Analysis/MisusedMovedObject.cpp
index 57c1ecc912b54e187fe071deb71bd1f4e804aa48..645f1ab2e5343dd8b22e01410d765ac795c15716 100644 (file)
--- a/test/Analysis/MisusedMovedObject.cpp
+++ b/test/Analysis/MisusedMovedObject.cpp
@@ -332,6 +332,8 @@ void moveStateResetFunctionsTest() {
     A b = std::move(a);
     a.reset(); // no-warning
     a.foo();   // no-warning
+    // Test if resets the state of subregions as well.
+    a.b.foo(); // no-warning
   }
   {
     A a;
@@ -344,6 +346,7 @@ void moveStateResetFunctionsTest() {
     A b = std::move(a);
     a.clear(); // no-warning
     a.foo();   // no-warning
+    a.b.foo(); // no-warning
   }
 }
 
@@ -444,7 +447,7 @@ void differentBranchesTest(int i) {
   // Same thing, but with a switch statement.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 448}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 451}}
     case 1:
       b = std::move(a); // no-warning
       break;            // expected-note {{Execution jumps to the end of the function}}
@@ -456,7 +459,7 @@ void differentBranchesTest(int i) {
   // However, if there's a fallthrough, we do warn.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 460}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 463}}
     case 1:
       b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
     case 2:
@@ -598,6 +601,7 @@ void ifStmtSequencesDeclAndConditionTest() {
   }
 }
 
+class C : public A {};
 void subRegionMoveTest() {
   {
     A a;
@@ -616,12 +620,24 @@ void subRegionMoveTest() {
     a.foo();             // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
     a.b.foo();           // no-warning
   }
+  {
+    C c;
+    C c1 = std::move(c); // expected-note {{'c' became 'moved-from' here}}
+    c.foo();             // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
+    c.b.foo();           // no-warning
+  }
 }
 
-class C: public A {};
 void resetSuperClass() {
   C c;
   C c1 = std::move(c);
   c.clear();
   C c2 = c; // no-warning
 }
+
+void reportSuperClass() {
+  C c;
+  C c1 = std::move(c); // expected-note {{'c' became 'moved-from' here}}
+  c.foo();             // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
+  C c2 = c;            // no-warning
+}