Page MenuHomePhabricator

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

Authored by NoQ on Oct 8 2017, 10:01 AM.

Details

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.)

Diff Detail

Repository
rC Clang

Event Timeline

szepet created this revision.Oct 8 2017, 10:01 AM
danielmarjamaki added a subscriber: 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 edited edge metadata.Oct 9 2017, 6:12 AM

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.

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?

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 edited edge metadata.Oct 10 2017, 2:51 AM

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 updated this revision to Diff 119032.Oct 14 2017, 10:14 AM

Test file (running line) update.

NoQ commandeered this revision.Dec 14 2018, 5:26 PM
NoQ edited reviewers, added: szepet; removed: NoQ.
NoQ updated this revision to Diff 178324.Dec 14 2018, 5:27 PM
NoQ retitled this revision from [analyzer] MisusedMovedObjectChecker: Moving the checker out of alpha state to [analyzer] MoveChecker Pt.10: Move the checker out of alpha state..

Rebase!

NoQ edited reviewers, added: a_sidorin, rnkovacs, Szelethus; removed: zaks.anna.Dec 14 2018, 5:28 PM
NoQ removed subscribers: Szelethus, rnkovacs, NoQ.
NoQ added a comment.Dec 16 2018, 10:27 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 16 2018, 10:34 PM
Closed by commit rC349328: [analyzer] MoveChecker: Enable by default as cplusplus.Move. (authored by dergachev, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.

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