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

[Alexander Droste via cfe-commits]

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

Thanks for the review!


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+  HelpText<"Checks MPI code written in C">,
+  DescFile<"MPIChecker.cpp">;
----------------
zaks.anna wrote:
> I'd suggest running this checker on a C++ codebase to ensure it works and does not introduce regressions (crashes) and change the help text. 
I ran the checker on [[ http://gauss.cs.ucsb.edu/~aydin/CombBLAS/html/ | CombBLAS]].
All reports were correct and nothing crashed.  -> changed the help text

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h:30
@@ +29,3 @@
+} // end of namespace: cont
+
+#endif
----------------
zaks.anna wrote:
> This code is generic; why is this part of the MPI checker?
> If you feel that the LLVM containers are missing some functionality, this code should go there...
I think it would belong to `<algorithm>`. As this is part of libc++, how shall I proceed?

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:25
@@ +24,3 @@
+
+const std::string MPIError{"MPI Error"};
+const std::string MPIWarning{"MPI Warning"};
----------------
zaks.anna wrote:
> There is no notion of diagnostic severity (errors vs warnings) in the static analyzer right now. I see you are using issue categories to allow for the differentiation.
> 
> In addition to this, it would be useful to allow a user to get a report of only the MPI Errors. You can split the checkers into 2 checkers, which can be turned on independently. See nullability or malloc checkers for examples on how to do that.
> 
> If we split the checkers, can we just have a single category? (We are very conservative with the number of categories right now.)
I originally intended to additionally use warnings but then decided
to mark all reports as error types. I simply forgot to remove the warning 
constant from the code.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:114
@@ +113,3 @@
+  std::string LastUser = Req.LastUser->getCalleeIdentifier()->getName();
+  std::string ErrorText{"Request '" + Req.variableName() +
+                        "' is already waited upon by '" + LastUser +
----------------
zaks.anna wrote:
> This assumes that the referred to event (last user) occurs in the same file. I am not sure if that's always the case. The analyzer can produce cross file reports even now since it "inlines" implementations from header files.
> 
> The "proper" way to refer to an earlier event for a path sensitive report is through implementing a BugReporterVisitor. See SecKeychainBugVisitor in MacOSKeychainAPIChecker.cpp for a simple example. 
Yes, the last user can be in another file. 
I'll look into the visitor implementation.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:24
@@ +23,3 @@
+
+void MPICheckerPathSensitive::checkDoubleNonblocking(
+    const clang::ento::CallEvent &CE, CheckerContext &Ctx) const {
----------------
zaks.anna wrote:
> It's a bit hard to read this because it is not clear from the interface here which event we are handling (ex: pre-call).
I changed the parameter name to `PreCallEvent`.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:32
@@ +31,3 @@
+  // no way to reason about symbolic region
+  if (MR->getBaseRegion()->getAs<SymbolicRegion>())
+    return;
----------------
zaks.anna wrote:
> Why not? What is the issue with handling these?
The problem about symbolic regions is rather in `collectUsedMemRegions()` than
in `checkDoubleNonblocking()` why the guard is again used in `checkWaitUsage()`. 
>From my understanding, it is not possible to find out for a symbolic region what element count and subregions (in case of an array) it has. All elements of a request array are supposed to be marked as waited in `checkWaitUsage()` if the wait function used is `MPI_Waitall()`. `MPI_Waitall()` receives an array of requests as an argument, in order to wait for all requests (or rather nonblocking operations) to complete. `collectUsedMemRegions()` iterates over the array to collect all single requests used. Therefore, the region is cannot be symbolic.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:64
@@ +63,3 @@
+// register data structure for path-sensitive analysis
+REGISTER_MAP_WITH_PROGRAMSTATE(RequestMap, const clang::ento::MemRegion *,
+                               clang::mpi::Request)
----------------
zaks.anna wrote:
> Semantically, what is this map storing?
> What is the MR in the Request?
> Is LastUser added to the map for the purpose of better diagnostic? If so, please, see my other path sensitive diagnostic comment.
I updated the comment.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h:15
@@ +14,3 @@
+
+#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_UTILITY_H
+#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_UTILITY_H
----------------
zaks.anna wrote:
> A lot of these functions are not specific to the checker, so we would want to move them elsewhere. However, some of these might not be needed if/once we change the way the auxiliary diagnostic event is reported.
Ok, I leave this unmodified for the moment.

================
Comment at: tools/clang/test/Analysis/MPIChecker.c:114
@@ +113,3 @@
+  }
+} // expected-warning{{'MPI_Isend' in line 110, using request 'sendReq1', has no matching wait in the scope of this function.}}
+
----------------
zaks.anna wrote:
> There are 2 potential issues with this check:
> 1) Is it possible that the wait is called by another function?
> 2) It's more user friendly to report this issue at the last point where the request is available rather than the last line of the function.
> 
> This looks similar to leak report checking. Is it?
1. Yes, that's not very common but possible.
2. Does this change involve the use of a BugReporterVisitor?!

You're right, it's very similar.


http://reviews.llvm.org/D12761