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

[Aaron Puchert via Phabricator via cfe-commits]

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