r316850 - [analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects
Peter Szecsi via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 28 16:09:37 PDT 2017
Author: szepet
Date: Sat Oct 28 16:09:37 2017
New Revision: 316850
URL: http://llvm.org/viewvc/llvm-project?rev=316850&view=rev
Log:
[analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects
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
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
cfe/trunk/test/Analysis/MisusedMovedObject.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp?rev=316850&r1=316849&r2=316850&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp Sat Oct 28 16:09:37 2017
@@ -352,7 +352,7 @@ void MisusedMovedObjectChecker::checkPre
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::checkPre
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::checkPre
}
// 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;
Modified: cfe/trunk/test/Analysis/MisusedMovedObject.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/MisusedMovedObject.cpp?rev=316850&r1=316849&r2=316850&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/MisusedMovedObject.cpp (original)
+++ cfe/trunk/test/Analysis/MisusedMovedObject.cpp Sat Oct 28 16:09:37 2017
@@ -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
+}
More information about the cfe-commits
mailing list