[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

 def NewDeleteLeaksChecker : Checker<"NewDeleteLeaks">,
   HelpText<"Check for memory leaks. Traces memory managed by new/delete.">,
   DescFile<"MallocChecker.cpp">;
 
 def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,
   HelpText<"Checks C++ copy and move assignment operators for self assignment">,
   DescFile<"CXXSelfAssignmentChecker.cpp">;
 
+def MisusedMovedObjectChecker : Checker<"MisusedMovedObject">,
+  HelpText<"Method calls on a moved-from object and copying a moved-from "
+           "object will be reported">,
+  DescFile<"MisusedMovedObjectChecker.cpp">;
+
 } // end: "cplusplus"
 
 let ParentPackage = CplusplusOptIn in {
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
 } // end: "optin.cplusplus"
 
 let ParentPackage = CplusplusAlpha in {
 
 def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
            "destructor in their base class">,
   DescFile<"DeleteWithNonVirtualDtorChecker.cpp">;
 
 def IteratorRangeChecker : Checker<"IteratorRange">,
   HelpText<"Check for iterators used outside their valid ranges">,
   DescFile<"IteratorChecker.cpp">;
 
-def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
-     HelpText<"Method calls on a moved-from object and copying a moved-from "
-              "object will be reported">,
-     DescFile<"MisusedMovedObjectChecker.cpp">;
-
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//
 // Valist checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Valist in {

test/Analysis/MisusedMovedObject.cpp

-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify -analyzer-output=text %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=cplusplus.MisusedMovedObject -std=c++11 -verify -analyzer-output=text %s
 
 namespace std {
 
 template <typename>
 struct remove_reference;
 
 template <typename _Tp>
 struct remove_reference { typedef _Tp type; };