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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

We gave a talk about building checkers a while back; even though it does not talk about the bug reporter visitors, it might be worth watching if you haven't already seen it.

http://llvm.org/devmtg/2012-11/ 
Building a Checker in 24 hours


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:68
@@ +67,3 @@
+  const CallEventRef<> CERef = PreCallEvent.cloneWithState(State);
+  const ExplodedNode *const ExplNode = Ctx.addTransition();
+  llvm::SmallVector<const MemRegion *, 2> ReqRegions;
----------------
There is no reason to add an empty transition to the graph. Maybe you want to get the predecessor instead?

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:106
@@ +105,3 @@
+    static CheckerProgramPointTag Tag("MPI-Checker", "MissingWait");
+    ExplodedNode *const ExplNode =
+        Ctx.generateNonFatalErrorNode(Ctx.getState(), &Tag);
----------------
generateNonFatalErrorNode will add a transition to the exploded graph, which means that you should only generate it if there is an error and you do not need to call addTransition later in the function. 

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:67
@@ +66,3 @@
+  // Every time a request is 'set' a new 'RequestId' gets created.
+  // Therefore, the 'UserKind' does not need to be profiled.
+  const int RequestId{RequestIdCount++};
----------------
Still it looks like you have several pieces of information in the state that are redundant.. Looks like you've added more now.

For example, why do we need RequestId? Each report will only talk about a single request, is this not the case?

Do you absolutely need to store LastUser and VariableName?

Note that storing in the state is very expensive; on the other hand, we can afford to perform costly analysis on post-processing of an error report. This is why all other checkers store minimal information in the state, usually just a single enum and determine all the other information during the walk over the error path in BugReporterVisitor. The visitor will visit all nodes that you visited while reporting this bug, so it should have access to all the information you need.



http://reviews.llvm.org/D12761