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

Wed Aug 17 20:37:42 PDT 2016

As a sidenote, the issue of a function's/API's intended specs is an issue
Microsoft dealt with through the use of SAL, as both API callers and API
implementers get static-analyzer checking of their code vs their
intentions.... but it also explicit what the API specifies & how it is
intended to be used.

On May 12, 2016 5:30 PM, "David Blaikie via cfe-dev" <cfe-dev at lists.llvm.org>
wrote:

>
>
> 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
>>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160817/9af0fa19/attachment-0001.html>