aaron.ballman added a reviewer: NoQ. aaron.ballman added a subscriber: NoQ. aaron.ballman added a comment. Herald added a subscriber: Charusso.
In D83717#2366598 <https://reviews.llvm.org/D83717#2366598>, @gamesh411 wrote: >> 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. If the assert was directly within the handler code, then sure. However, consider a situation like this: struct Something { Something(int *ip) { assert(ip); ... } ... }; where the use of the assertion is far removed from the fact that it's being used within a handler. > 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. Yes, this is exactly the situation I'm worried about. > 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 Agreed. > 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'. I'm hoping someone more well-versed in the details of the static analyzer can speak to this point. @NoQ @Szelethus others? > 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. Thank you for looking into this! If it turns out that there's some reason why the static analyzer cannot be at least as good of a home for the functionality as clang-tidy, that would be really interesting to learn. Either there are improvements we could consider making to the static analyzer, or we could leave the check in clang-tidy despite the limitations, but there's still a path forward. 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
