> Hi David,
> > * Initializing variables that are only used when initialized through some
> > existing codepath - this can make tools like Memory Sanitizer less useful,
> > because now the value is initialized even in some path where that valu is
> > never intended to be used
> * I guess this case could be related to ⚙ D19971 [scan-build] fix warnings emitted on LLVM TargetRecip code base
> and ⚙ D19975 [scan-build] fix warning emitted on LLVM Bitcode code base for instance.
> I wasn't happy with the fixes in the first place but I
> ended-up trusting the tool because it proposed a codepath where the
> variables were used without being initialized. I don't know about
> msan (yet but if the warnings are false positives is there a way
> to tell scan-build in such cases ?
My first guess with any warning like this would be that there's some
complex invariant (eg: A implies B, so if the variable is conditionally
initialized under A then used under B it's actually fine) that is obvious
enough in the code, but opaque to the static analyzer.
My first guess would be to assert this (assert the condition of A in the
use under B) and only if we find a case where that fails, add a test case
and initialize the variable as appropriate. Adding the variable
initialization without a test case seems suspect.
MSan is a tool for dynamic analysis - it would track the uninitialized
memory and tell us when/where we've used it without initializing it.
Ok, the invariant example is a good insight I wish I kept in mind when
I started. I took interest in Clang Static Analyzer reports because I
wondered how the tool works (and how well). Knowing how to treat the
warnings will definitely help making better decisions (and better
Any further advice will be greatly appreciated :).
> > * Adding assertions for pointers that are known to be non-null - in some
> > cases this is helpful, when the algorithm that ensures the non-null-ness is
> > sufficiently opaque. But for function parameters - we have /lots/ of
> > non-null function parameters. I think it'd be impractical to assert on all
> > of them.
> * This is where I ended-up asserting function parameters in a
> mechanical manner to some extent (as a result of 40+ warnings about
> some object pointers being null). Let's take ⚙ D19385 [scan-build] fix warnings emitted on Clang Format code base
> for instance.
> The right fix in that case was to pass the non-null parameter by
> reference instead of asserting its value, not unlike what you were
> discussing in the previous post (sorry I'm not quoting the right
> post here). For the cases where using references might not be
> possible, how do we deal with the warnings ?
I think the tool needs to be fixed, or not used on the LLVM codebase if it
assumes all pointer parameters may be null. But it seems like it doesn't
assume that or I would expect way more errors from the tool, no? I was
pretty sure the Clang Static Analyzer had been designed to only warn about
null dereferences if it saw a pointer null test somewhere in the function
for that pointer.
Do you know what's going on here?
Judging from the 55 warnings I got by running the analyzer on
LLVM+Clang+Compiler-RT codebase when I started, it does *not* seem to
assume all pointer parameters may be null indeed.
However, the tool seems to be able to warn not only about "null
dereferences if it saw a pointer null test", but also about null
dereferences if it saw a may-be-null pointer passed as a parameter
in a function call in the same compilation unit.
This last sentence should read: the tool also seems to be able to follow the
function call flow inside a compilation unit and warn about null
dereferences at any point during that call flow (e.g.
⚙ D19084 [scan-build] fix warnings emitted on Clang AST code base in lib/AST/NestedNameSpecifier.cpp).
Some of these latter warnings may well be false positives I agree, but
what makes it hard to ignore is when the tool actually suggests a call
path to the null dereference. Ignoring it then looks to me like a
case of "the pointer is *expected* to be non-null, so the warning
should be ignored regardless of the hints provided by the tool".
I'd rather add an assert in a defensive stance in that case, but
that's just me; in the end I take the reviewer's advice into account :).