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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
