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

[Alexander Droste via cfe-commits]

Alexander_Droste added a comment.

In http://reviews.llvm.org/D12761#250296, @zaks.anna wrote:

> > Sorry, I should have been more precise. The check tests if a request is in use by a nonblocking 
>
> >  call at the end of a function. But the request can still be alive after return. You can think of it as
>
> >  a pointer for which the memory must be freed in the function it was allocated. But the pointer 
>
> >  can exist in global space outside a function. Multiple functions can use the same request.
>
> >  So the symbol is not necessarily dead at the end of a function.
>
>
> Is it the API requirement that the wait is performed in the same function?


That's a good catch. It is no API requirement that a nonblocking call is matched by a wait within
the scope of a function. I think for C this would be reasonable but for CPP it is not sufficient,
as the matching could be realised with RAII or lambdas. I will change the check so that
it makes use of the `checkDeadSymbols` callback.


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:137
@@ +136,3 @@
+
+  // This is never reached...
+  llvm::outs() << "////////////////"
----------------
zaks.anna wrote:
> Alexander_Droste wrote:
> > Alexander_Droste wrote:
> > > I figured out what the problem about the request retrieval is:
> > > `REGISTER_MAP_WITH_PROGRAMSTATE(RequestMap, const clang::ento::clang::mpi::Request)`
> > > The documentation states: "The macro should not be used inside namespaces, or for traits that must
> > > be accessible from more than one translation unit." I think I need some advice here, how to make the  
> > > same RequestMap accessible in multiple translation units.
> > How about this: 
> > We could capture the `PathDiagnosticLocation` of the LastUser in each `mpi::Request` as a member. The `RequestMap` could then be local to `MPICheckerPathSensitive.cpp` and no visitors would be needed. The bug reporter class would then not need to know anything about the request map. The only function I can't find to realise this concept would be something like: `Report->addPathDiagnosticLocation()`.
> Almost all other checkers occupy a single file per checker. This is not much more complicated than the others so you could do the same. This would probably be better for consistency, but I do not have such a strong opinion about it and see why you might want to have separate files. 
> 
> See TaintManager.h for the example of how to share the program state maps across translation units.
Thanks for pointing out `TaintManager.h`. I'll try to derive a solution from that.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:66
@@ +65,3 @@
+// Register data structure for path-sensitive analysis. The map stores MPI
+// requests which are identified by their memory region. Requests are used in
+// MPI to complete nonblocking operations with wait operations. Requests can be
----------------
zaks.anna wrote:
> Look at the ways the other checkers reference a node that occurred earlier along the symbolic execution path. For example, malloc checker tracks the state of each pointer, which could be "Allocated", "Freed"... When we report a leak, for example, the state would be "Allocated" and the symbol would be dead. The BugReporterVisitor walks up the symbolic path on which the issue occurred and prints an extra note at the place where the node is allocated. It knows at which node the pointer is allocated because that is where the state of the pointer changes from untracked to "Allocated".
> 
Ok, I will use the same pattern. Thanks for the explanation.


http://reviews.llvm.org/D12761