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

Thu May 12 14:30:27 PDT 2016

On Thu, May 12, 2016 at 2:05 PM, Apelete Seketeli <apelete at seketeli.net>
wrote:

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

Yes, the static analyzer is interprocedural within a single translation
unit, so this might be the issue (it sees a null test of a pointer inside
one function A, then assumes that the parameter to function B (which calls
A and passes along the pointer) must also be possibly null). It would be
good to test that, then fix the analyzer to not make this assumption - it
seems incorrect. Just because a pointer may be null in one function,
doesn't imply much of anything about it in callers to that function - those
callers may have guaranteed non-null pointers still.

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

My argument against this is that it seems we'd end up with a very haphazard
set of assertions - the significant majority of cases where we have
non-null pointers aren't being diagnosed by the tool, so some
semi-arbitrary subset of those cases would end up with asserts and the rest
not. I'm not sure how constructive that would be, which is why I'm inclined
to push back a bit on that approach due to the lack of consistency in the
end result we'd get.

- Dave

>
> Cheers.
> --
>         Apelete
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160512/a9fb8140/attachment.html>