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

[Alexander Droste via cfe-commits]

Alexander_Droste marked 3 inline comments as done.
Alexander_Droste added a comment.

> I still have to do an overall pass over this checker, but it looks much better than the first version!


:D


================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:47
@@ +46,3 @@
+    BReporter->reportDoubleNonblocking(PreCallEvent, *Req, MR, ExplNode);
+    Ctx.addTransition(ErrorNode->getState(), ErrorNode);
+  }
----------------
zaks.anna wrote:
> Correct, it should be ErrorNode. Moreover, if you do not modify the State and use the state from the predecessor node, which is the case here, you do not need to pass a State to it. You do not need to pass the tag either in his case - a page generated for your checker will be used. By default the CheckerContext will use the state of the predecessor node and that default tag. (If you want to pass your tag, it's fine.)
> 
> You do need to pass the state if you modify the state as in the next example.
Ah I see, this is covered by the default parameters.

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:135
@@ +134,3 @@
+
+void MPIChecker::checkMissingWaitsGlobals(ExplodedGraph &G) const {
+
----------------
zaks.anna wrote:
> I do not think I've seen this code before. If you add new functionality, please submit it in a new commit. Otherwise, there is a high chance it will go unnoticed. 
> 
> Are there examples of the new warnings it caught? 
> 
> checkEndAnalysis is rarely used by checkers, so you might not need it here. Specifically, you should be notified by the checkDeadSymbols before a symbol dies even if it dies because it's the end of path.
Yes, I added this at some point after the initial patch, after I realized that the following test case fails:

```
MPI_Request sendReq1;
void missingWait2() { // Check missing wait for global variable.
  int rank = 0;
  double buf = 0;
  MPI_Comm_rank(MPI_COMM_WORLD, &rank);
  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, &sendReq1);
} // expected-warning{{Request 'sendReq1' has no matching wait.}}
```
if the checker solely relies on `checkDeadSymbols`. I just rechecked this behavior by commenting out

```
BReporter->reportMissingWait(Req.second, Req.first, *NodeIt);
```
in `checkMissingWaitsGlobals` and the test still fails then. Are global variables really covered by `checkDeadSymbols`?


http://reviews.llvm.org/D12761