This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Fix null pointer dereference warnings [1/n]
Needs ReviewPublic

Authored by mgrang on Apr 24 2020, 8:44 PM.

Details

Summary

Running the PREfast static analysis tool on clang resulted in several null
pointer dereference warnings. This is the first of several patches to fix
these.

The full list of warnings reported is here: https://docs.google.com/spreadsheets/d/1h_3tHxsgBampxb7PXoB5lgwiBSpTty9RLe5maIQxnTk/edit#gid=2014408543

Diff Detail

Event Timeline

mgrang created this revision.Apr 24 2020, 8:44 PM

Please don't add null checks for pointers that can't be null. It makes the code slower and harder to understand. And least one of the checks you added is actively breaking the code.

In some cases, the analysis is pointing to cases where the code could be made more clear for both humans and machines with some refactoring or assertions. Patches welcome, but please make sure any assertions properly explain the invariant. And please split the patches up a bit more; adding assertions for complex invariants in ten different unrelated places is more than I really want to review at once.

(Also, a reminder, please post patches with full context.)

Please don't add null checks for pointers that can't be null. It makes the code slower and harder to understand. And least one of the checks you added is actively breaking the code.

In some cases, the analysis is pointing to cases where the code could be made more clear for both humans and machines with some refactoring or assertions. Patches welcome, but please make sure any assertions properly explain the invariant. And please split the patches up a bit more; adding assertions for complex invariants in ten different unrelated places is more than I really want to review at once.

(Also, a reminder, please post patches with full context.)

Thanks @efriedma. Will split up the patches into smaller chunks and also post with full context.

mgrang updated this revision to Diff 260517.Apr 27 2020, 7:08 PM
mgrang retitled this revision from [Sema] Fix null pointer dereference warnings [1/n] to [Analysis] Fix null pointer dereference warnings [1/n].
dblaikie added inline comments.Apr 27 2020, 7:32 PM
clang/lib/Analysis/ThreadSafety.cpp
1938–1940 ↗(On Diff #260517)

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)

aaronpuchert added a subscriber: aaronpuchert.EditedApr 28 2020, 10:41 AM

Could you explain why we are doing this? Clang has its own static analyzer, which can find null dereferences as well but also tracks constraints to prevent false positives like this.

You can find pretty recent results of Clang static analyzer runs here: http://llvm.org/reports/scan-build/.

clang/lib/Analysis/ThreadSafety.cpp
1938–1940 ↗(On Diff #260517)

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.