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

[Alexander Droste via cfe-commits]

Alexander_Droste added a comment.

Sorry about the delay.


================
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:
> Alexander_Droste wrote:
> > 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.
> I suspect the only thing you need in Request is the enum; everything else can be determined while visiting the path. 
> 
> This should be in ento namespace not in the clang namespace.
I think you're right that the enum is sufficient.
Thus for better diagnostics the source range
of the CallEventRef (LastUser)
should be obtained by the BugReportVisitor.
Is it possible to obtain a CallEventRef based on
a memory region? So conceptually this would mean
to find the last function call (before the error node) using 
a specific region.
 


http://reviews.llvm.org/D12761