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

[Apelete Seketeli via cfe-dev]

Hi David,

On Thu, May-05-2016 at 11:20:06 AM -0700, David Blaikie wrote:
> Hi Apelete,
> 
> Thanks for trying to help cleanup the LLVM codebase of Clang Static
> Analyzer warnings.
> 
> But it seems a lot of the fixes that are being proposed are somewhat
> mechanical and may be doing the wrong thing in a few ways.

>From the beginning I wondered for each fix if it was the right way of
solving the possible underlying issue pointed to by the Analyzer.
Since I'm not familiar with the codebase, when I was not able to
decide by reading the code I ended-up trusting the tool until advised
otherwise by the reviewers.

Once put together, the fixes do look somewhat mechanical indeed due to
the fact that the same patterns were found throughout the codebase by
the analyzer.
As of this writing, these patterns result in 90+ warnings still
emitted when scanning LLVM+Clang+Compiler-RT (down from 150+ warnings
when I fooled myself into starting to look at it a few weeks ago).

I'm glad you pointed out some of the patterns in the fixes, let's see
how I can address those:

> * 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 value 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 ?

> * 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 ?

> But if we want to do something like this we should probably have some
> community discussion about how how to go about any of these things across
> the codebase. Maybe using nonnull attributes would be a better approach to
> the parameter issue, for example - but it'll still be a lot of code churn
> that there should probably be general consensus on rather than approaching
> it piecemeal with reviewers in different parts of the codebase.

I agree we should discuss the bigger issue of how to deal with each
type of warning emitted by Clang Static Analyzer. I would prefer to
propose fixes that solve the underlying issues instead of just
suppressing the warnings.

Thanks again for taking the time to suggest a different approach.

Cheers.
-- 
        Apelete