[PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

[Mandeep Singh Grang via cfe-commits]

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 <
reviews at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200429/1c943a4b/attachment-0001.html>