[PATCH] D12761: MPI-Checker patch for Clang Static Analyzer

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Alexander,

This patch is in a pretty good shape. I am fine with committing it and iterating with smaller updates in tree if it is more convenient for you.

One task that I would like to very strongly encourage is running this on a lot of code. You will find a lot of issues this way that are hard to discover during code review. Both false positives and crashes.

Thanks!
Anna.


================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:96
@@ +95,3 @@
+    if (const CallExpr *CE = clang::dyn_cast<CallExpr>(SP->getStmt())) {
+
+      auto FuncIdentifier = CE->getDirectCallee()->getIdentifier();
----------------
The advantage of using the state is that it will be much more robust to any further changes to the compiler/checker because you will not be pattern matching the AST but instead will be checking the state, which the core reasoning is based on. One example that comes to mind is indirect calls. You will reduce the amount of code here as well, simplifying maintainability. This is the pattern we use in other checkers as well, so there is a remote chance we could introduce a new simplified API that will do the walk for the checker writers.

With respect to your example. Does it come up in practice? Wouldn't you warn on the second nonblocking request anyway? Could you add such an example to the tests? (Would be good in any case. If you leave the code as is, you can point to that example as the motivation.)

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:87
@@ +86,3 @@
+      }
+      // A wait has no matching nonblocking call.
+      BReporter->reportUnmatchedWait(PreCallEvent, ReqRegion, ErrorNode);
----------------
This is done, right?

================
Comment at: test/Analysis/MPIChecker.cpp:99
@@ +98,3 @@
+  MPI_Wait(&req, MPI_STATUS_IGNORE);
+}
+
----------------
This are explaining the path on which the problem occurs; the users will see them as well. There should not be a lot of those, you do not have a lot of conditions. Would it be reasonable to change the tests to incorporate those. Other alternative is to have another tests file that tests the notes in that mode.

What do you think?

================
Comment at: test/Analysis/MPIChecker.cpp:114
@@ +113,3 @@
+
+void doubleNonblocking4() {
+  int rank = 0;
----------------
> I would then simply create a new pair of .cpp and .h files in the test folder 
> where I define those functions so that the MPI-Checker tests can use them.

You do not have to do that. You could just declare the functions and not define them. It will be equivalent to having the definitions in the other TUs.




http://reviews.llvm.org/D12761