[cfe-dev] Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...

[Apelete Seketeli via cfe-dev]

On Thu, May-12-2016 at 11:33:28 AM -0700, David Blaikie wrote:
> On Fri, May 6, 2016 at 3:09 AM, Apelete Seketeli <apelete at seketeli.net>
> wrote:
> 
> > Hi David,
> >
> > On Thu, May-05-2016 at 11:20:06 AM -0700, David Blaikie wrote:
> >
> > > * 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 http://reviews.llvm.org/D19971
> >   and http://reviews.llvm.org/D19975 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
patches).

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 http://reviews.llvm.org/D19385
> >   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.
http://reviews.llvm.org/D19084 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 :).

Cheers.
-- 
        Apelete