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

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

[Samuel Lijin]

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

A build with this patch can be found at [1]. Note that if reports *are*
generated, this doesn't allow us to access them, and if we dumped
the reports as build artifacts, I'm not sure where we would want to
dump them to.

It's worth noting that there seems to be a weird issue with scan-build
where it *will* generate a report for something locally, but won't do it
on Travis. See [2] for an example where I have a C program with a
very obvious memory leak but scan-build on Travis doesn't generate
a report (despite complaining about it in stdout), even though it does
on my local machine.

[1] https://travis-ci.org/sxlijin/git/builds/204853233
[2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342

 .travis.yml | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f..1038b1b3d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -20,6 +20,7 @@ addons:
     - language-pack-is
     - git-svn
     - apache2
+    - clang

 env:
   global:
@@ -78,9 +79,8 @@ before_install:
       brew update --quiet
       # Uncomment this if you want to run perf tests:
       # brew install gnu-time
-      brew install git-lfs gettext
-      brew link --force gettext
-      brew install caskroom/cask/perforce
+      brew install git-lfs gettext caskroom/cask/perforce llvm
+      brew link --force gettext llvm
       ;;
     esac;
     echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)";
@@ -92,9 +92,9 @@ before_install:
     mkdir -p $HOME/travis-cache;
     ln -s $HOME/travis-cache/.prove t/.prove;

-before_script: make --jobs=2
+before_script: scan-build make --jobs=2

-script: make --quiet test
+script: scan-build make --quiet test

 after_failure:
   - >
--
2.11.1

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

[Lars Schneider]

> 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

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


[1] https://github.com/git/git/blob/e7e07d5a4fcc2a203d9873968ad3e6bd4d7419d7/.travis.yml#L42-L53
[2] https://larsxschneider.github.io/git-scan/


> ---
> 
> A build with this patch can be found at [1]. Note that if reports *are*
> generated, this doesn't allow us to access them, and if we dumped
> the reports as build artifacts, I'm not sure where we would want to
> dump them to.

We could upload the results to a Git repo and then use GitHub pages to serve
it. I did that with my run here: https://larsxschneider.github.io/git-scan/


> It's worth noting that there seems to be a weird issue with scan-build
> where it *will* generate a report for something locally, but won't do it
> on Travis. See [2] for an example where I have a C program with a
> very obvious memory leak but scan-build on Travis doesn't generate
> a report (despite complaining about it in stdout), even though it does
> on my local machine.
> 
> [1] https://travis-ci.org/sxlijin/git/builds/204853233
> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342

Scan-build stores the report in some temp folder. I assume you can't access
this folder on TravisCI. Try the scan-build option "-o scan-build-results"
to store the report in the local directory. 


> 
> .travis.yml | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 9c63c8c3f..1038b1b3d 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -20,6 +20,7 @@ addons:
>     - language-pack-is
>     - git-svn
>     - apache2
> +    - clang
> 
> env:
>   global:
> @@ -78,9 +79,8 @@ before_install:
>       brew update --quiet
>       # Uncomment this if you want to run perf tests:
>       # brew install gnu-time
> -      brew install git-lfs gettext
> -      brew link --force gettext
> -      brew install caskroom/cask/perforce
> +      brew install git-lfs gettext caskroom/cask/perforce llvm
> +      brew link --force gettext llvm

This wouldn't be necessary if we only scan on Linux.


>       ;;
>     esac;
>     echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)";
> @@ -92,9 +92,9 @@ before_install:
>     mkdir -p $HOME/travis-cache;
>     ln -s $HOME/travis-cache/.prove t/.prove;
> 
> -before_script: make --jobs=2
> +before_script: scan-build make --jobs=2

I think we should run it like this:

scan-build -analyze-headers --status-bugs --keep-going --force-analyze-debug-code make --jobs=2

This way TravisCI would be notified via the return code if scan-build detected
errors I think.


> -script: make --quiet test
> +script: scan-build make --quiet test

Why do you want to scan the tests?


Cheers,
Lars

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

[Jeff King]

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

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

> 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

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

[Lars Schneider]

> On 25 Feb 2017, at 23:31, Jeff King <peff@peff.net> wrote:
> 
> On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:
> 
>> 
>>> 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.

That makes sense. I suspected that this assignment was intentional
but I wasn't sure why. I didn't know about the rearrangement of argv.

Apparently an "(void)argc;" silences this warning. Would that be too
ugly to bear? :-)


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

Agreed.


- Lars

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

[Samuel Lijin]

On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>
>> It's worth noting that there seems to be a weird issue with scan-build
>> where it *will* generate a report for something locally, but won't do it
>> on Travis. See [2] for an example where I have a C program with a
>> very obvious memory leak but scan-build on Travis doesn't generate
>> a report (despite complaining about it in stdout), even though it does
>> on my local machine.
>>
>> [1] https://travis-ci.org/sxlijin/git/builds/204853233
>> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
>
> Scan-build stores the report in some temp folder. I assume you can't access
> this folder on TravisCI. Try the scan-build option "-o scan-build-results"
> to store the report in the local directory.

That occurred to me, but I don't quite think that's the issue. I just
noticed that on the repo I use to test build matrices, jobs 1-8 don't
generate a report, but 9-14 and 19-20 do [1]. I don't think it's an
issue with write permissions (scan-build complains much more vocally
if that happens), but it doesn't seem to matter if the output dir is
in the tmpfs [2] or a local directory [3].

[1] https://travis-ci.org/sxlijin/travis-testing/builds/205054253
[2] https://travis-ci.org/sxlijin/git/jobs/205028920#L1000
[2] https://travis-ci.org/sxlijin/git/jobs/205411705#L998

>> @@ -78,9 +79,8 @@ before_install:
>>       brew update --quiet
>>       # Uncomment this if you want to run perf tests:
>>       # brew install gnu-time
>> -      brew install git-lfs gettext
>> -      brew link --force gettext
>> -      brew install caskroom/cask/perforce
>> +      brew install git-lfs gettext caskroom/cask/perforce llvm
>> +      brew link --force gettext llvm
>
> This wouldn't be necessary if we only scan on Linux.

Agreed. I'm not sure if macOS static analysis would bring any specific
benefits; I don't really have much experience with static analysis
tools one way or another, so I'm happy to defer on this decision.


>> -script: make --quiet test
>> +script: scan-build make --quiet test
>
> Why do you want to scan the tests?

Brain fart on my end.

> Cheers,
> Lars

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

[Lars Schneider]

> On 26 Feb 2017, at 03:09, Samuel Lijin <sxlijin@gmail.com> wrote:
> 
> On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>> 
>>> It's worth noting that there seems to be a weird issue with scan-build
>>> where it *will* generate a report for something locally, but won't do it
>>> on Travis. See [2] for an example where I have a C program with a
>>> very obvious memory leak but scan-build on Travis doesn't generate
>>> a report (despite complaining about it in stdout), even though it does
>>> on my local machine.
>>> 
>>> [1] https://travis-ci.org/sxlijin/git/builds/204853233
>>> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
>> 
>> Scan-build stores the report in some temp folder. I assume you can't access
>> this folder on TravisCI. Try the scan-build option "-o scan-build-results"
>> to store the report in the local directory.
> 
> That occurred to me, but I don't quite think that's the issue. I just
> noticed that on the repo I use to test build matrices, jobs 1-8 don't
> generate a report, but 9-14 and 19-20 do [1]. I don't think it's an
> issue with write permissions (scan-build complains much more vocally
> if that happens), but it doesn't seem to matter if the output dir is
> in the tmpfs [2] or a local directory [3].
> 
> [1] https://travis-ci.org/sxlijin/travis-testing/builds/205054253
> [2] https://travis-ci.org/sxlijin/git/jobs/205028920#L1000
> [2] https://travis-ci.org/sxlijin/git/jobs/205411705#L998

Scan-build somehow replaces the compiler. My guess is that you 
tell scan-build to substitute clang but "make" is really using 
gcc or something? I reported something strange about the compilers
on TravisCI some time ago but I can't find it anymore. I think I 
remember on OSX they always use clang even if you define gcc. 
Maybe it makes sense to reach out to TravisCI support in case 
this is a bug on their end?

Based on your work I tried the following and it seems to work:
https://travis-ci.org/larsxschneider/git/jobs/205507241
https://github.com/larsxschneider/git/commit/faf4ecfdca1a732459c1f93c334928ee2826d490

- Lars

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

[Samuel Lijin]

On Sun, Feb 26, 2017 at 8:12 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 26 Feb 2017, at 03:09, Samuel Lijin <sxlijin@gmail.com> wrote:
>>
>> On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider
>> <larsxschneider@gmail.com> wrote:
>>>
>>>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>>>
>>>> It's worth noting that there seems to be a weird issue with scan-build
>>>> where it *will* generate a report for something locally, but won't do it
>>>> on Travis. See [2] for an example where I have a C program with a
>>>> very obvious memory leak but scan-build on Travis doesn't generate
>>>> a report (despite complaining about it in stdout), even though it does
>>>> on my local machine.
>>>>
>>>> [1] https://travis-ci.org/sxlijin/git/builds/204853233
>>>> [2] https://travis-ci.org/sxlijin/travis-testing/jobs/205025319#L331-L342
>>>
>>> Scan-build stores the report in some temp folder. I assume you can't access
>>> this folder on TravisCI. Try the scan-build option "-o scan-build-results"
>>> to store the report in the local directory.
>>
>> That occurred to me, but I don't quite think that's the issue. I just
>> noticed that on the repo I use to test build matrices, jobs 1-8 don't
>> generate a report, but 9-14 and 19-20 do [1]. I don't think it's an
>> issue with write permissions (scan-build complains much more vocally
>> if that happens), but it doesn't seem to matter if the output dir is
>> in the tmpfs [2] or a local directory [3].
>>
>> [1] https://travis-ci.org/sxlijin/travis-testing/builds/205054253
>> [2] https://travis-ci.org/sxlijin/git/jobs/205028920#L1000
>> [2] https://travis-ci.org/sxlijin/git/jobs/205411705#L998
>
> Scan-build somehow replaces the compiler. My guess is that you
> tell scan-build to substitute clang but "make" is really using
> gcc or something?

Your hunch is spot-on. I took a look at the Makefile and lo and
behold, it overrides $CC [1]. Looking at the commit which introduced
it [2] I have to admit I'm somewhat surprised that scan-build works at
all...

[1] https://github.com/git/git/blob/master/Makefile#L454
[2] https://github.com/git/git/commit/6d62c983f7d91565a15e49955b3ed94ae7c73434

> I reported something strange about the compilers
> on TravisCI some time ago but I can't find it anymore. I think I
> remember on OSX they always use clang even if you define gcc.
> Maybe it makes sense to reach out to TravisCI support in case
> this is a bug on their end?
>
> Based on your work I tried the following and it seems to work:
> https://travis-ci.org/larsxschneider/git/jobs/205507241
> https://github.com/larsxschneider/git/commit/faf4ecfdca1a732459c1f93c334928ee2826d490

That's promising!

> - Lars