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

[mgrang]

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

[efriedma]

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.)

[mgrang]

In D78853#2003866, @efriedma wrote:

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.

[dblaikie]

clang/lib/Analysis/ThreadSafety.cpp

1938–1940

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]

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

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.

clang/lib/Analysis/ThreadSafety.cpp

Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
                                 M, LK_Exclusive, Loc, isScopedVar),
                       CapDiagKind);
   for (const auto &M : SharedLocksToAdd)
     Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
                                 M, LK_Shared, Loc, isScopedVar),
                       CapDiagKind);
 
   if (isScopedVar) {
+    // 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");
+
     // Add the managing object as a dummy mutex, mapped to the underlying mutex.
     SourceLocation MLoc = VD->getLocation();
     DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,
                     VD->getLocation());
     // FIXME: does this store a pointer to DRE?
     CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr);
 
     auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, MLoc);