My previous email details why we are doing this: http://lists.llvm.org/pipermail/llvm-dev/2020-April/141167.html Basically, we ran the PREfast static analysis tool on LLVM/Clang and it reported a lot of warnings. I guess some of them are false positives after all.
As David suggests that maybe I should validate these warnings by also running the clang static analyzer. There are a lot of other warnings reported by the tool. Here is the full list: https://docs.google.com/spreadsheets/d/1h_3tHxsgBampxb7PXoB5lgwiBSpTty9RLe5maIQxnTk/edit?usp=sharing. If the community is interested in getting those fixed I can upstream patches. --Mandeep On Tue, Apr 28, 2020 at 10:44 AM Aaron Puchert via Phabricator < revi...@reviews.llvm.org> wrote: > aaronpuchert added a comment. > > Could you explain why we are doing this? Clang has its own static analyzer > <https://clang-analyzer.llvm.org/>, which can find null dereferences < > https://clang.llvm.org/docs/analyzer/checkers.html#core-nulldereference-c-c-objc> > as well but also tracks constraints to prevent false positives like this. > > There is a page somewhere with current analysis runs on LLVM, although I > can't find it right now. > > > > ================ > Comment at: clang/lib/Analysis/ThreadSafety.cpp:1938-1940 > + // The default value for the argument VD to the current function is > + // nullptr. So we assert that VD is non null because we deref VD here. > + assert(VD && "!VD"); > ---------------- > dblaikie wrote: > > Doesn't seem like the most informative comment or assertion string - the > invariant "isScopedVar implies VD is non-null" is established earlier in > the function, where isScopedVar only becomes true under the "VD is > non-null" condition at 1809. > > > > Would it be better to improve whatever static analysis you're using to > be able to track that correlation, rather than adding lots of extra > assertions to LLVM? (can the Clang Static Analyzer understand this code and > avoid warning on it, for instance - that'd be a good existence proof for > such "smarts" being reasonably possible for static analysis) > I haven't tested it, but the Clang static analyzer shouldn't complain > here. It explores different code paths and collects constraints along them. > All code paths including the assignment `isScopedVar = true` should have > `VD` being non-null as constraint, because that's the condition to get > there. > > I know that solving constraints can be hard in general, but here the > constraint is exactly what we need to disprove null references from > happening. > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D78853/new/ > > https://reviews.llvm.org/D78853 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
