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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Hi Alexander,

Sorry for the delay!

The patch should be rebased from the clang repo; for example, you could run "svn diff" from the clang directory. More comments inline.


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74,3 @@
 
+def MPI : Package<"mpi">;
+
----------------
This should probably go under the 'option' package. What do you think?

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:24
@@ +23,3 @@
+namespace clang {
+namespace mpi {
+
----------------
Should this be clang::ento::mpi? Alternatively, you could place everything into a single file and have it live in anonymous namespace. This would also be more consistent with the existing checkers.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:40
@@ +39,3 @@
+
+  // ast reports ––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
+
----------------
Looks like some of the AST stuff was deleted and some was kept. Please, either remove all of it or keep all of it in.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:26
@@ +25,3 @@
+public:
+  // path-sensitive callbacks––––––––––––––––––––––––––––––––––––––––––––
+  void checkPreCall(const CallEvent &CE, CheckerContext &Ctx) const {
----------------
nit: Please, remove the comments with "----" for consistency.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43
@@ +42,3 @@
+private:
+  const std::unique_ptr<MPICheckerPathSensitive> CheckerSens;
+
----------------
I'd stress "path" instead of "sensitive" in the name. Also, this indirection is redundant if you remove the AST checks.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:43
@@ +42,3 @@
+  if (Req && Req->CurrentState == Request::State::Nonblocking) {
+    BugReporter.reportDoubleNonblocking(PreCallEvent, *Req, MR, ExplNode);
+  }
----------------
You should use 'generateErrorNode' or 'generateNonFatalErrorNode' to generate the node on which the error should be reported. If the error is non-fatal, you'd need to use the generated node below. Specifically, use it's state and pass it as the predecessor when adding a transition.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:74
@@ +73,3 @@
+      // A wait has no matching nonblocking call.
+      BugReporter.reportUnmatchedWait(PreCallEvent, ReqRegion, ExplNode);
+    }
----------------
This is called in a loop. Should you break once the first error is reported?

Also, as before you should use the CheckerContext API to produce a node on which the error should be reported.



================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:79
@@ +78,3 @@
+  if (!ReqRegions.empty()) {
+    Ctx.addTransition(State);
+  }
----------------
Don't forget to specify a predecessor here once the code above changes.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h:87
@@ +86,3 @@
+  const MPIFunctionClassifier FuncClassifier;
+  MPIBugReporter BugReporter;
+};
----------------
Please, use a name other than 'BugReporter' to avoid confusing it with the BugReporter in the analyzer.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h:45
@@ +44,3 @@
+  bool isAllgatherType(const clang::IdentifierInfo *const IdentInfo) const;
+  bool isAlltoallType(const clang::IdentifierInfo *const IdentInfo) const;
+  bool isReduceType(const clang::IdentifierInfo *const IdentInfo) const;
----------------
Some of these classifier functions are not used either..

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h:77
@@ +76,3 @@
+  llvm::SmallVector<clang::IdentifierInfo *, 4> MPIPointToCollTypes;
+  llvm::SmallVector<clang::IdentifierInfo *, 4> MPICollToPointTypes;
+  llvm::SmallVector<clang::IdentifierInfo *, 6> MPICollToCollTypes;
----------------
MPIPointToCollTypes  and MPICollToPointTypes do not seem to be used.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:56
@@ +55,3 @@
+struct RequestMap {};
+typedef llvm::ImmutableMap<const clang::ento::MemRegion *,
+                           clang::ento::mpi::Request> RequestMapImpl;
----------------
Let's add some documentation on why you are not using the standard macro REGISTER_MAP_WITH_PROGRAMSTATE. (Might help to avoid confusion lear on.)

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:21
@@ +20,3 @@
+
+namespace util {
+
----------------
I'd like to remove the Utility.cpp completely by either making the helper functions static or moving them to other shared components.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:40
@@ +39,3 @@
+
+clang::StringRef sourceRangeAsStringRef(const clang::SourceRange &Range,
+                                        clang::ento::AnalysisManager &AM) {
----------------
This is unused.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:47
@@ +46,3 @@
+
+const clang::IdentifierInfo *getIdentInfo(const clang::CallExpr *CE) {
+  if (CE->getDirectCallee()) {
----------------
This is not used.. 

================
Comment at: tools/clang/test/Analysis/MPIChecker.cpp:112
@@ +111,3 @@
+  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD,
+              &sendReq1); // Request is previously used by nonblocking call here.
+} // expected-warning{{Request 'sendReq1' has no matching wait.}}
----------------
This is not testing the extra information provided by bug reporter visitor; you should use "// expected-note {...}"


http://reviews.llvm.org/D12761