From: Peter Szecsi Date: Sat, 28 Oct 2017 23:09:37 +0000 (+0000) Subject: [analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a3962bae3fc11604e07e1a7b8f232967b7b610df;p=clang [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 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 4b69a1bd1e..9d405cd39c 100644 --- 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(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(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(WholeObjectRegion)) - WholeObjectRegion = BR->getSuperRegion(); - - State = State->remove(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(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 57c1ecc912..645f1ab2e5 100644 --- 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 +}