From 07964fbdcb0fdccbcdcdb26179dc4f0d3a78fc01 Mon Sep 17 00:00:00 2001 From: Peter Szecsi Date: Sat, 28 Oct 2017 23:24:00 +0000 Subject: [PATCH] [analyzer] MisusedMovedObjectChecker: More precise warning message Added new enum in order to differentiate the warning messages on "misusing" into 3 categories: function calls, moving an object, copying an object. (At the moment the checker gives the same message in case of copying and moving.) Additional test cases added as well. Differential Revision: https://reviews.llvm.org/D38674 llvm-svn: 316852 --- .../Checkers/MisusedMovedObjectChecker.cpp | 32 +++++++++---- clang/test/Analysis/MisusedMovedObject.cpp | 54 +++++++++++++++++----- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp index 9d405cd..497978f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -60,6 +60,7 @@ public: const char *NL, const char *Sep) const override; private: + enum MisuseKind {MK_FunCall, MK_Copy, MK_Move}; class MovedBugVisitor : public BugReporterVisitorImpl { public: MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} @@ -83,7 +84,7 @@ private: mutable std::unique_ptr BT; ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, - CheckerContext &C, bool isCopy) const; + CheckerContext &C, MisuseKind MK) const; bool isInMoveSafeContext(const LocationContext *LC) const; bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const; @@ -179,7 +180,7 @@ const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, const CallEvent &Call, CheckerContext &C, - bool isCopy = false) const { + MisuseKind MK) const { if (ExplodedNode *N = C.generateNonFatalErrorNode()) { if (!BT) BT.reset(new BugType(this, "Usage of a 'moved-from' object", @@ -195,10 +196,17 @@ ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, // Creating the error message. std::string ErrorMessage; - if (isCopy) - ErrorMessage = "Copying a 'moved-from' object"; - else - ErrorMessage = "Method call on a 'moved-from' object"; + switch(MK) { + case MK_FunCall: + ErrorMessage = "Method call on a 'moved-from' object"; + break; + case MK_Copy: + ErrorMessage = "Copying a 'moved-from' object"; + break; + case MK_Move: + ErrorMessage = "Moving a 'moved-from' object"; + break; + } if (const auto DecReg = Region->getAs()) { const auto *RegionDecl = dyn_cast(DecReg->getDecl()); ErrorMessage += " '" + RegionDecl->getNameAsString() + "'"; @@ -365,7 +373,10 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, const RegionState *ArgState = State->get(ArgRegion); if (ArgState && ArgState->isMoved()) { if (!isInMoveSafeContext(LC)) { - N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + if(CtorDec->isMoveConstructor()) + N = reportBug(ArgRegion, Call, C, MK_Move); + else + N = reportBug(ArgRegion, Call, C, MK_Copy); State = State->set(ArgRegion, RegionState::getReported()); } @@ -405,7 +416,10 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, State->get(IC->getArgSVal(0).getAsRegion()); if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) { const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion(); - N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + if(MethodDecl->isMoveAssignmentOperator()) + N = reportBug(ArgRegion, Call, C, MK_Move); + else + N = reportBug(ArgRegion, Call, C, MK_Copy); State = State->set(ArgRegion, RegionState::getReported()); } @@ -443,7 +457,7 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, if (isInMoveSafeContext(LC)) return; - N = reportBug(ThisRegion, Call, C); + N = reportBug(ThisRegion, Call, C, MK_FunCall); State = State->set(ThisRegion, RegionState::getReported()); C.addTransition(State, N); } diff --git a/clang/test/Analysis/MisusedMovedObject.cpp b/clang/test/Analysis/MisusedMovedObject.cpp index 645f1ab..132a65d 100644 --- a/clang/test/Analysis/MisusedMovedObject.cpp +++ b/clang/test/Analysis/MisusedMovedObject.cpp @@ -38,6 +38,7 @@ public: B() = default; B(const B &) = default; B(B &&) = default; + B& operator=(const B &q) = default; void operator=(B &&b) { return; } @@ -70,6 +71,12 @@ public: A(A &&other, char *k) { moveconstruct(std::move(other)); } + void operator=(const A &other) { + i = other.i; + d = other.d; + b = other.b; + return; + } void operator=(A &&other) { moveconstruct(std::move(other)); return; @@ -105,17 +112,42 @@ void copyOrMoveCall(A a) { } void simpleMoveCtorTest() { - A a; - A b; - b = std::move(a); // expected-note {{'a' became 'moved-from' here}} - a.foo(); // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}} + { + A a; + A b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + a.foo(); // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}} + } + { + A a; + A b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + b = a; // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}} + } + { + A a; + A b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + b = std::move(a); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}} + } } void simpleMoveAssignementTest() { - A a; - A b; - b = std::move(a); // expected-note {{'a' became 'moved-from' here}} - a.foo(); // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}} + { + A a; + A b; + b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + a.foo(); // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}} + } + { + A a; + A b; + b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + A c(a); // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}} + } + { + A a; + A b; + b = std::move(a); // expected-note {{'a' became 'moved-from' here}} + A c(std::move(a)); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}} + } } void moveInInitListTest() { @@ -270,7 +302,7 @@ void loopTest() { { A a; for (int i = 0; i < bignum(); i++) { // expected-note {{Loop condition is true. Entering loop body}} expected-note {{Loop condition is true. Entering loop body}} - constCopyOrMoveCall(std::move(a)); // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}} + constCopyOrMoveCall(std::move(a)); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}} // expected-note@-1 {{'a' became 'moved-from' here}} } } @@ -447,7 +479,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 451}} + switch (i) { // expected-note {{Control jumps to 'case 1:' at line 483}} case 1: b = std::move(a); // no-warning break; // expected-note {{Execution jumps to the end of the function}} @@ -459,7 +491,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 463}} + switch (i) { // expected-note {{Control jumps to 'case 1:' at line 495}} case 1: b = std::move(a); // expected-note {{'a' became 'moved-from' here}} case 2: -- 2.7.4