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

Alexander Droste via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 30 03:02:12 PDT 2016


Alexander_Droste marked 3 inline comments as done.

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:95
@@ +94,3 @@
+  if (Optional<PreStmt> SP = N->getLocation().getAs<PreStmt>()) {
+    if (const CallExpr *CE = clang::dyn_cast<CallExpr>(SP->getStmt())) {
+
----------------
zaks.anna wrote:
> Would it be possible to identify the "interesting node" by just looking at how the state you track changes. This function will be called when we walk the path upward from the error node. For example, you can notice that in the previous state, the Request did not exist and in this state it is Nonblocking. That would allow you to conclude that between those 2 states a function was called. (This is the trick we use in the other checkers (ex: malloc); it simplifies implementation and makes it more robust - we do not need to pattern match as much.)
I think this should work but would slightly change the way diagnostics are presented in a specific case.  If more than 2 nonblocking calls are using a request in sequence, the expected note (`Request is previously used by nonblocking call here.`) would not point to the previous but to the first nonblocking call of that sequence. What do you think; should the `VisitNode` function be changed regardless or should it stay as it is because of this specific case?

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:86
@@ +85,3 @@
+      // A wait has no matching nonblocking call.
+      BReporter->reportUnmatchedWait(PreCallEvent, ReqRegion, ErrorNode);
+    }
----------------
zaks.anna wrote:
> Can you add a test cases for this report when there are multiple ReqRegions? Looks like all of the test cases with MPI_Waitall succeed. (I test a case where one of the regions has a match in Waitall and one does not as well as when several/all do not have a match.)
Sure, I'll add those tests. The case where none of the requests is matched should be covered by `missingNonBlockingMultiple`.

================
Comment at: test/Analysis/MPIChecker.cpp:98
@@ +97,3 @@
+
+  MPI_Request req;
+  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, &req); // expected-note{{Request is previously used by nonblocking call here.}}
----------------
zaks.anna wrote:
> Do you see the notes in the report? I think you should pass "-analyzer-output=text" to see the notes.
If I pass `-analyzer-output=text` I see the notes in the report. But adding that flag also produces a lot of 'undesired' output like: `Loop condition is true. Entering loop body.`

================
Comment at: test/Analysis/MPIChecker.cpp:113
@@ +112,3 @@
+  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, &rs.req); // expected-note{{Request is previously used by nonblocking call here.}}
+  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, &rs.req); // expected-warning{{Double nonblocking on request 'rs.req'.}}
+  MPI_Wait(&rs.req, MPI_STATUS_IGNORE);
----------------
zaks.anna wrote:
> Are we allowed to call reduce -> wait -> reduce with the same req? If so, let's test for that.
> 
> Also, can calls to reduce and wait occur in different functions? If so, we need to test cases where they occur in different functions that are all in the same translation unit - all of the implementations are visible to the analyzer. And also test the cases where we pass the request to a function, whose body is not visible to the analyzer, which is the case when the function is defined in another translation unit.
> 
> Are we allowed to call reduce -> wait -> reduce with the same req? If so, let's test for that.
Yes, that's allowed. I'll add a test.

> Also, can calls to reduce and wait occur in different functions?
This is also possible.

> And also test the cases where we pass the request to a function, whose body is not visible to the analyzer, which is the case when the function is defined in another translation unit.
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. 


http://reviews.llvm.org/D12761





More information about the cfe-commits mailing list