[analyzer] MoveChecker Pt.10: Move the checker out of alpha state.

[NoQ]

Summary

First, I am not exactly sure what are the requirements for moving a checker out of alpha. However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different) Also found a true positive (well, it was only example code but still!) which fixes was sent and accepted in patch D32939 .

Is it enough or should I check it on other open source projects? If so, what results are acceptable? ( @NoQ probably has already used it as well, maybe can have some more comments on the results.)

[danielmarjamaki]

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe what the false positives are? Is it possible to fix them?

Is it enough or should I check it on other open source projects?

you should check a number of different projects. There might be idioms/usages in other projects that are not seen in LLVM.

However I don't know what other open source C++11 projects there are.

But I have a script that runs clang on various Debian projects and I can run that and provide you with the results.

[xazax.hun]

In D38675#891750, @danielmarjamaki wrote:

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe what the false positives are? Is it possible to fix them?

Note that the unique findings are 6. I think there are non-alpha checks with more false positives.

[danielmarjamaki]

In D38675#891912, @xazax.hun wrote:

In D38675#891750, @danielmarjamaki wrote:

However, the checker seems to work with a low false positive rate. (<15 on the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe what the false positives are? Is it possible to fix them?

Note that the unique findings are 6. I think there are non-alpha checks with more false positives.

This does not answer the important questions. What are the false positives, and is it possible to fix them?

In D38675#891750, @danielmarjamaki wrote:

Is it enough or should I check it on other open source projects?

you should check a number of different projects. There might be idioms/usages in other projects that are not seen in LLVM.

However I don't know what other open source C++11 projects there are.

But I have a script that runs clang on various Debian projects and I can run that and provide you with the results.

Something didn't go well. I started my script yesterday afternoon. It has checked 426 projects so far. I do not see a single MisusedMovedObject warning. I am thinking that my script doesn't work well in this case. A wild idea is that maybe my script must somehow tell scan-build to use -std=c++11.

For information here are the cppcheck results for a similar checker:
http://cppcheck.sourceforge.net/cgi-bin/daca2-search.cgi?id=accessMoved

Maybe you could pick a few of those projects and check those. That should exercise this checker.

Please tell me if you think some of the accessMoved warnings are false positives. These warnings are "inconclusive" which indicates they might not always be correct.

[NoQ]

Last time i was running on WebKit; i already lost my results, so i'd try to reproduce the results on the fixed checker and follow up. Apart from D31538, i've seen a few cases where a method was safe to be called on a moved-from object (which led me to believe that we'd need to be safer here), and a few weird cases where a moved-from object was accidentally copied implicitly, which seemed to be a non-issue.

You'd need to update the checker name in the test run line.

[szepet]

Test file (running line) update.

[NoQ]

Rebase!

[NoQ]

Currently the checker is very quiet. I have seen exactly 6 warnings on internal codebases on which i tested it (on which we otherwise emit around 12000 warnings), and all of them were good. The only false positive i've seen so far was fixed in D55566.

We might find more problems with liveness analysis being too conservative, but i'm not seeing many of them right now.

We might also come to a conclusion that the local variable heuristic is not that good, but we have an option to quickly turn it off by flipping a flag introduced in D55730.

So i think this is safe to enable.

[Szelethus]

A little late to the party, but I don't see anything against this.

include/clang/StaticAnalyzer/Checkers/Checkers.td

HelpText<"Check for double-free and use-after-free problems. Traces memory "
            "managed by new/delete.">;
 
 def NewDeleteLeaksChecker : Checker<"NewDeleteLeaks">,
   HelpText<"Check for memory leaks. Traces memory managed by new/delete.">;
 
 def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,
   HelpText<"Checks C++ copy and move assignment operators for self assignment">;
 
+def MoveChecker: Checker<"Move">,
+   HelpText<"Find use-after-move bugs in C++">;
+
 } // end: "cplusplus"
 
 let ParentPackage = CplusplusOptIn in {
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">;
 
 } // end: "optin.cplusplus"

 
 def IteratorRangeChecker : Checker<"IteratorRange">,
   HelpText<"Check for iterators used outside their valid ranges">;
 
 def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
   HelpText<"Check for use of iterators of different containers where iterators "
            "of the same container are expected">;
 
-def MoveChecker: Checker<"Move">,
-     HelpText<"Find use-after-move bugs in C++">;
-
 def UninitializedObjectChecker: Checker<"UninitializedObject">,
      HelpText<"Reports uninitialized fields after object construction">;
 
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//
 // Valist checkers.

test/Analysis/mismatched-iterator.cpp

void good_move_find2(std::vector<int> &v1, std::vector<int> &v2, int n) {
   auto i0 = --v2.cend();
   v1 = std::move(v2);
   std::find(i0, v1.cend(), n); // no-warning
 }
 
 void good_move_find3(std::vector<int> &v1, std::vector<int> &v2, int n) {
   auto i0 = v2.cend();
   v1 = std::move(v2);
-  v2.push_back(n);
+  v2.push_back(n); // expected-warning{{Method called on moved-from object of type 'std::vector'}}
   std::find(v2.cbegin(), i0, n); // no-warning
 }
 
 void good_comparison(std::vector<int> &v) {
   if (v.cbegin() == v.cend()) {} // no-warning
 }
 
 void bad_ctor(std::vector<int> &v1, std::vector<int> &v2) {
   std::vector<int> new_v(v1.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
 }
 
 void bad_find(std::vector<int> &v1, std::vector<int> &v2, int n) {
   std::find(v1.cbegin(), v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
 }
 
 void bad_find_first_of(std::vector<int> &v1, std::vector<int> &v2) {
   std::find_first_of(v1.cbegin(), v2.cend(), v2.cbegin(), v2.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
   std::find_first_of(v1.cbegin(), v1.cend(), v2.cbegin(), v1.cend()); // expected-warning{{Iterators of different containers used where the same container is expected}}
 }
 
 void bad_move_find1(std::vector<int> &v1, std::vector<int> &v2, int n) {
   auto i0 = v2.cbegin();
   v1 = std::move(v2);
   std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+                               // expected-warning@-1{{Method called on moved-from object of type 'std::vector'}}
 }
 
 void bad_insert_find(std::vector<int> &v1, std::vector<int> &v2, int n, int m) {
   auto i = std::find(v1.cbegin(), v1.cend(), n);
   v2.insert(i, m); // expected-warning{{Container accessed using foreign iterator argument}}
 }
 
 void good_overwrite(std::vector<int> &v1, std::vector<int> &v2, int n) {
void good_move(std::vector<int> &v1, std::vector<int> &v2) {
   v1 = std::move(v2);
   v1.erase(i0); // no-warning
 }
 
 void bad_move(std::vector<int> &v1, std::vector<int> &v2) {
   const auto i0 = ++v2.cbegin();
   v1 = std::move(v2);
   v2.erase(i0); // expected-warning{{Container accessed using foreign iterator argument}}
+                // expected-warning@-1{{Method called on moved-from object of type 'std::vector'}}
 }
 
 void bad_move_find2(std::vector<int> &v1, std::vector<int> &v2, int n) {
   auto i0 = --v2.cend();
   v1 = std::move(v2);
   std::find(i0, v2.cend(), n); // expected-warning{{Iterators of different containers used where the same container is expected}}
+                               // expected-warning@-1{{Method called on moved-from object of type 'std::vector'}}
 }
 
 void bad_move_find3(std::vector<int> &v1, std::vector<int> &v2, int n) {
   auto i0 = v2.cend();
   v1 = std::move(v2);
   std::find(v1.cbegin(), i0, n); // expected-warning{{Iterators of different containers used where the same container is expected}}
 }
 
 void bad_comparison(std::vector<int> &v1, std::vector<int> &v2) {
   if (v1.cbegin() != v2.cend()) { // expected-warning{{Iterators of different containers used where the same container is expected}}
     *v1.cbegin();
   }
 }

test/Analysis/use-after-move.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
 // RUN:  -analyzer-checker debug.ExprInspection
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-checker debug.ExprInspection
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN:  -analyzer-config alpha.cplusplus.Move:WarnOn=KnownsOnly -DPEACEFUL\
+// RUN:  -analyzer-config cplusplus.Move:WarnOn=KnownsOnly -DPEACEFUL\
 // RUN:  -analyzer-checker debug.ExprInspection
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN:  -analyzer-config alpha.cplusplus.Move:WarnOn=KnownsOnly -DPEACEFUL\
+// RUN:  -analyzer-config cplusplus.Move:WarnOn=KnownsOnly -DPEACEFUL\
 // RUN:  -analyzer-checker debug.ExprInspection
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN:  -analyzer-config alpha.cplusplus.Move:WarnOn=All -DAGGRESSIVE\
+// RUN:  -analyzer-config cplusplus.Move:WarnOn=All -DAGGRESSIVE\
 // RUN:  -analyzer-checker debug.ExprInspection
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN:  -analyzer-config alpha.cplusplus.Move:WarnOn=All -DAGGRESSIVE\
+// RUN:  -analyzer-config cplusplus.Move:WarnOn=All -DAGGRESSIVE\
 // RUN:  -analyzer-checker debug.ExprInspection
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();
 
 class B {
 public: