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

[Alexander Droste via cfe-commits]

Alexander_Droste 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.


I have watched it some months ago but I'm sure it is a good idea to revisit the talk. Thanks for the idea!


================
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;
----------------
zaks.anna wrote:
> There is no reason to add an empty transition to the graph. Maybe you want to get the predecessor instead?
Yes, that's better!

================
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++};
----------------
zaks.anna wrote:
> 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.
> 
I need the RequestID to distinct function calls of the same type 
and location using the request multiple times along the path.
Like in a loop:
```
for int i ...
   nonblocking(.., request) // double nonblocking
```

> Do you absolutely need to store LastUser and VariableName?
Absolutely not. :D I will remove the string member.
The variable name retrieval can be delayed to the point where the report is created.

I can also remove the enum, as the state of the request can be derived from the LastUser.
The reason why I added the enum member, is that I was concerned about that the CallEventRef
can be invalid to reference again if the call is inlined from an implementation defined in a header. 
As this seems not to be the case, I can remove the redundancy.

> 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.

Ah, I get it. Until now that seemed a bit overly complicated to me.


http://reviews.llvm.org/D12761