Re: [PATCH] travis-ci: run scan-build every time

From: Jeff King <peff@peff.net>
Date: Feb 25, 2017, 22:31 UTC
Message-ID: <20170225223146.ixubnwqkfol5q2gn@sigill.intra.peff.net>
In-Reply-To: <BAB1EE0E-B258-4108-AE24-110172086DE4@gmail.com>

On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:

Show 17 quoted lines

> 
> > On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
> > 
> > Introduces the scan-build static code analysis tool from the Clang
> > project to all Travis CI builds. Installs clang (since scan-build
> > needs clang as a dependency) to make this possible (on macOS, also
> > updates PATH to allow scan-build to be invoked without referencing the
> > full path).
> 
> This is a pretty neat idea. However, I think this should become a
> dedicated job in a TravisCI build (similar to the Documentation job [1])
> because:
>  a) We don't want to build and test a scan-build version of Git (AFAIK
>     scan-build kind of proxies the compiler to do its job - I don't if
>     this has any side effects)
>  b) We don't want to slow down the other builds
>  c) It should be enough to run scan-build once on Linux per build

Yeah. I am all for static analysis, but I agree it should be its own job. Especially as it can be quite noisy with false positives (and I really think before any static analysis is useful we need to figure out a way to suppress the false positives, so that we can see the signal in the noise).

Fully a third of the problem cases found are dead assignments or increments. I looked at a few, and I think the right strategy is to tell the tool "no really, our code is fine". For instance, it complains about:

  argc = parse_options(argc, argv, ...);

when argc is not used again later. Sure, that assignment is doing nothing. But from a maintainability perspective, I'd much rather have a dead assignment (that the compiler is free to remove) then for somebody to later add a loop like:

  for (i = 0; i < argc; i++)
          something(argv[i]);

which will read past the end of the rearranged argv (and probably _wouldn't_ be caught by static analysis, because the hidden dependency between argc and argv is buried inside the parse_options() call).

So there is definitely some bug-fixing to be done, but I think there is also some work in figuring out how to suppress these useless reports. Turning off the dead-assignment checker is one option, but I actually think it _could_ produce useful results. It just isn't in these cases. So I'd much rather if we can somehow suppress the specific callsites.

Show 8 quoted lines

> I ran scan-build on the current master and it detected 72 potential bugs [2]. 
> I looked through a few of them and they seem to be legitimate. If the list agrees
> that running scan-build is a useful thing and that these problems should be fixed
> then we could:
> 
> (1) Add scan-build check to Travis CI but only print errors as warning
> (2) Fix the 72 existing bugs over time
> (3) Turn scan-build warnings into errors

If they are warnings socked away in a Travis CI job that nobody looks out, then I doubt anybody is going to bother fixing them.

Not that step (1) hurts necessarily, but I don't think it's really doing anything until step (2) is finished.

I took a look at a few of the non-dead-assignment ones and some of them are obviously false positives. E.g., in check_pbase_path(), it claims that done_pbase_paths might be NULL. But that value just went through ALLOC_GROW() with a non-zero value, which would either have allocated or died.

There are other cases where it complains that a strbuf's "buf" parameter might be NULL. That _shouldn't_ be the case, as it is an invariant of strbuf. It might be a bug, but it is certainly not a bug where the analyzer is pointing.

I won't be surprised at all if there are a bunch of real bugs in that list. But I think the interesting work at this point is not a CI build, but somebody locally slogging through scan-build and categorizing each one.

-Peff