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

[Alexander Droste via cfe-commits]

Alexander_Droste added a comment.

Hi Anna,

thanks for having a look once more!


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74,3 @@
 
+def MPI : Package<"mpi">;
+
----------------
zaks.anna wrote:
> This should probably go under the 'option' package. What do you think?
Do you mean the 'optin' package? I could not find an 'option' package in `Checkers.td`.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:24
@@ +23,3 @@
+namespace clang {
+namespace mpi {
+
----------------
zaks.anna wrote:
> 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.
I would prefer to put this into `clang::ento::mpi`, rather than having everything in a single file.


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:40
@@ +39,3 @@
+
+  // ast reports ––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
+
----------------
zaks.anna wrote:
> 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.
Sorry about the inconsistent change. I will remove the AST related
functionality completety from this patch, as it will be part of the clang-tidy patch.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43
@@ +42,3 @@
+private:
+  const std::unique_ptr<MPICheckerPathSensitive> CheckerSens;
+
----------------
zaks.anna wrote:
> I'd stress "path" instead of "sensitive" in the name. Also, this indirection is redundant if you remove the AST checks.
If sufficient, I would rename `MPICheckerPathSensitive` to `MPICheckerPath` then.
Do you mind the indirection?

================
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);
+    }
----------------
zaks.anna wrote:
> 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.
> 
> 
I don't think break should be called after the first error is reported. Each memory region
represents a different request, why this should be rated as multiple errors. 

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

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h:87
@@ +86,3 @@
+  const MPIFunctionClassifier FuncClassifier;
+  MPIBugReporter BugReporter;
+};
----------------
zaks.anna wrote:
> Please, use a name other than 'BugReporter' to avoid confusing it with the BugReporter in the analyzer.
I see, that could be confusing. Maybe renaming the variable to BReporter will do.

================
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;
----------------
zaks.anna wrote:
> Some of these classifier functions are not used either..
These model distinct MPI function classes. I agree that it would be better to remove the unused ones, in order to keep the interface as narrow as possible.

================
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;
----------------
zaks.anna wrote:
> 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.)
Sure, I can do that.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:21
@@ +20,3 @@
+
+namespace util {
+
----------------
zaks.anna wrote:
> I'd like to remove the Utility.cpp completely by either making the helper functions static or moving them to other shared components.
So where shall we put `sourceRange()`? It is only used by the BugReporter class.
I could make it a member function of the Reporter class. Or would you prefer this
as a member function of `MemRegion`?

================
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()) {
----------------
zaks.anna wrote:
> This is not used.. 
These are also leftovers from the AST checks.

================
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.}}
----------------
zaks.anna wrote:
> This is not testing the extra information provided by bug reporter visitor; you should use "// expected-note {...}"
I didn't know about `expected-note`, thanks!


http://reviews.llvm.org/D12761