gamesh411 added a comment. > Just to make sure we're on the same page -- the current approach is not > flow-sensitive, and so my concern is that it won't report any true positives > (not that it will be prone to false positives).
Sorry about that. You are absolutely right; what I was trying to say is CallGraph-based. Just some thoughts on this example: In D83717#2279263 <https://reviews.llvm.org/D83717#2279263>, @aaron.ballman wrote: > One of the concerns I have with this not being a flow-sensitive check is that > most of the bad situations are not going to be caught by the clang-tidy > version of the check. The CERT rules show contrived code examples, but the > more frequent issue looks like: > > void cleanup(struct whatever *ptr) { > assert(ptr); // This potentially calls abort() > free(ptr->buffer); > free(ptr); > } > ... What I have in support of this approach is this reasoning: If a handler is used where either branch can abort then that branch is expected to be taken. Otherwise it is dead code. I would argue then, that this abortion should be refactored out of the handler function to ensure well-defined behaviour in every possible case. As a counter-argument; suppose that there is a function that is used as both an exit-handler and as a simple invocation. In this case, I can understand if one would not want to factor the abortion logic out, or possibly pass flags around. Then to this remark: > The fact that we're not looking through the call sites (even without cross-TU > support) means the check isn't going to catch the most problematic cases. You > could modify the called function collector to gather this a bit better, but > you'd issue false positives in flow-sensitive situations like: > > void some_cleanup_func(void) { > for (size_t idx = 0; idx < GlobalElementCount; ++idx) { > struct whatever *ptr = GlobalElement[idx]; > if (ptr) { > // Now we know abort() won't be called > cleanup(ptr); > } > } > } The current approach definitely does not take 'adjacent' call-sites into account (not to mention CTU ones). In this regard I also tend to see the benefit of this being a ClangSA checker as that would solve 3 problems at once: 1. Being path-sensitive, so we can explain how we got to the erroneous program-point 2. It utilizes CTU mode to take callsites from other TU-s into account 3. Runtime-stack building is implicitly done by ExprEngine as a side effect of symbolic execution Counter-argument: But using ClangSA also introduces a big challenge. ClangSA analyzes all top-level functions during analysis. However I don't know if it understands the concept of exit-handlers, and I don't know a way of 'triggering' an analysis 'on-exit' so to speak. So AFAIK this model of analyzing only top-level functions is a limitation when it comes to modelling the program behaviour 'on-exit'. sidenote: To validate this claim I have dumped the exploded graph of the following file: #include <cstdlib> #include <iostream> void f() { std::cout << "handler f"; }; int main() { std::atexit(f); } And it has no mention of std::cout being used, so I concluded, that ClangSA does not model the 'on-exit' behaviour. I wanted to clear these issues before I made the documentation. Thanks for the effort and the tips on evaluating the solution, I will do some more exploration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
