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