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

[Anna Zaks via cfe-commits]

zaks.anna requested changes to this revision.
zaks.anna added a comment.
This revision now requires changes to proceed.

Thanks for working on this!

Comments inline.


================
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">;
----------------
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. 

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h:30
@@ +29,3 @@
+} // end of namespace: cont
+
+#endif
----------------
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...

================
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"};
----------------
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.)

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:32
@@ +31,3 @@
+
+  // split written string into parts
+  std::vector<std::string> Strs = util::split(LineNo, ':');
----------------
Nit: Please use proper capitalization and punctuation.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:55
@@ +54,3 @@
+  SourceRange MPICallExprRange = MPICallExpr->getCallee()->getSourceRange();
+  std::string BT{"type mismatch"};
+  std::string ErrorText{"Buffer type '" + BufferType.getAsString() +
----------------
"type mismatch" -> "Type mismatch"
Bug Types are capitalized; please, change these throughout the patch.

================
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 +
----------------
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. 

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:31
@@ +30,3 @@
+      : BugReporter{BR}, CkrBase{CB}, AnalysisManager{AM} {
+    DoubleWaitBugType.reset(
+        new clang::ento::BugType(&CB, "double wait", "MPI Error"));
----------------
Bug types should be capitalized.

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

================
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;
----------------
Why not? What is the issue with handling these?

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:36
@@ +35,3 @@
+  ProgramStateRef State = Ctx.getState();
+  CallEventRef<> CERef = CE.cloneWithState(State);
+
----------------
I suspect the logic in the path sensitive checks will have to change. See the comments regarding the tracked state.

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

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

================
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.}}
+
----------------
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?


http://reviews.llvm.org/D12761