Re: [PATCH] travis-ci: run scan-build every time
On Sun, Feb 26, 2017 at 8:12 AM, Lars Schneiderwrote: > >> On 26 Feb 2017, at 03:09, Samuel Lijin wrote: >> >> On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider >> wrote: >>> On 24 Feb 2017, at 18:29, Samuel Lijin 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
Re: [PATCH] travis-ci: run scan-build every time
> On 26 Feb 2017, at 03:09, Samuel Lijinwrote: > > On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneider > wrote: >> >>> On 24 Feb 2017, at 18:29, Samuel Lijin 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
On Sat, Feb 25, 2017 at 3:48 PM, Lars Schneiderwrote: > >> On 24 Feb 2017, at 18:29, Samuel Lijin 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
> On 25 Feb 2017, at 23:31, Jeff Kingwrote: > > On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote: > >> >>> On 24 Feb 2017, at 18:29, Samuel Lijin 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
On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote: > > > On 24 Feb 2017, at 18:29, Samuel Lijinwrote: > > > > 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
> On 24 Feb 2017, at 18:29, Samuel Lijinwrote: > > 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
[PATCH] travis-ci: run scan-build every time
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