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