Re: [RFC/PATCH] Makefile: suppress some cppcheck false-positives

From: Jeff King <peff@peff.net>
Date: Dec 16, 2016, 22:16 UTC
Message-ID: <20161216221612.whxm3z3clhi4xcha@sigill.intra.peff.net>
In-Reply-To: <xmqqshpnsoij.fsf@gitster.mtv.corp.google.com>

On Fri, Dec 16, 2016 at 10:43:48AM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> > diff --git a/nedmalloc.supp b/nedmalloc.supp
> [...]
> > diff --git a/regcomp.supp b/regcomp.supp
> 
> Yuck for both files for multiple reasons.
> 
> I do not think it is a good idea to allow these files to clutter the
> top-level of tree.  How often do we expect that we may have to add
> more of these files?  Every time we start borrowing code from third
> parties?
> 
> What is the goal we want to achieve by running cppcheck?  
> 
>  a. Our code must be clean but we do not bother "fixing" [*1*] the
>     code we borrow from third parties and squelch output instead?
> 
>  b. Both our own code and third party code we borrow need to be free
>     of errors and misdetections from cppcheck?
> 
>  c. Something else?
> 
> If a. is what we aim for, perhaps a better option may be not to run
> cppcheck for the code we borrowed from third-party at all in the
> first place.  
> 
> If b. is our goal, we need to make sure that the false positive rate
> of cppcheck is acceptably low.

I think (b) is the goal; we'd hope that both our code and third party code would be bug-free. I think it's a fact of life with a static analysis tool that we're going to have to silence some false positives.

I think Chris started with the ones in compat because we are pretty sure they won't change much, so suppressing them by line number is easy. But we'd need to revisit this for our code, too. So just turning it off for compat/ is only punting on the problem for a little while. :)

I do think it would be less gross if we could put these files into compat/nedmalloc, etc. I don't know if you can specify --suppressions-list multiple times, but certainly we could do some pre-processing like:

  find . -name '*.cppcheck' |
  while read suppfile; do
	dir=$(dirname $suppfile)
	sed "s{^}{$dir/}" <$suppfile
  done >master-suppfile
  cppcheck --suppressions-file=master-suppfile

That would at least let us drop individual suppression files into their respective directories.

I do wonder, though, if the "inline" suppressions would be less painful. It looks like doing:

  // cppcheck-suppress uninitialized
  int var = var;

would work. I'm not sure if it understands non-C99 comments, though maybe it is time for us to loosen that rule. And suppressing the false positives that way does avoid fighting with gcc's analyzer, since we're not changing the code.

The real question is how often we'd have to sprinkle those comments, and how painful it would be. I see only 4 false positives that need suppressed in our code, but 2 of them rub me the wrong way. They are due to the tool failing to realize that die() is marked with NORETURN. Marking some site as "no, this isn't a double-free, the other code path would have died" feels like the wrong spot. The tool failure isn't where we're marking, but rather 10 lines above.

-Peff