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 +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
