[RFC/PATCH] Makefile: add cppcheck target

[RFC/PATCH] Makefile: add cppcheck target

[Chris Packham]

Add cppcheck target to Makefile. Cppcheck is a static
analysis tool for C/C++ code. Cppcheck primarily detects
the types of bugs that the compilers normally do not detect.
It is an useful target for doing QA analysis.

Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
I had been playing with cppcheck for some other projects and happened to
notice [1] in the archives. This is my attempt to resolve the feedback
that Junio made at the time.

In terms of errors that are actually reported there are only a few

$ make cppcheck
cppcheck --force --quiet --inline-suppr  .
[compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
[compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
[compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
[t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
[t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.

The last 2 are just false positives from test data. I haven't looked
into any of the others.

I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
in the make invocation.

$ make cppcheck CPPCHECK_ADD=--enable=all
... lots of output
    
[1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t

 Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index f53fcc90d..8b5976d88 100644
--- a/Makefile
+++ b/Makefile
@@ -2635,3 +2635,7 @@ cover_db: coverage-report
 cover_db_html: cover_db
 	cover -report html -outputdir cover_db_html cover_db
 
+.PHONY: cppcheck
+
+cppcheck:
+	cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
-- 
2.11.0.24.ge6920cf

Re: [RFC/PATCH] Makefile: add cppcheck target

[Chris Packham]

On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham <judge.packham@gmail.com> wrote:
> Add cppcheck target to Makefile. Cppcheck is a static
> analysis tool for C/C++ code. Cppcheck primarily detects
> the types of bugs that the compilers normally do not detect.
> It is an useful target for doing QA analysis.
>
> Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
> I had been playing with cppcheck for some other projects and happened to
> notice [1] in the archives. This is my attempt to resolve the feedback
> that Junio made at the time.
>
> In terms of errors that are actually reported there are only a few
>
> $ make cppcheck
> cppcheck --force --quiet --inline-suppr  .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.
>
> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
> in the make invocation.
>
> $ make cppcheck CPPCHECK_ADD=--enable=all
> ... lots of output
>
> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t
>
>  Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index f53fcc90d..8b5976d88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
>  cover_db_html: cover_db
>         cover -report html -outputdir cover_db_html cover_db
>
> +.PHONY: cppcheck
> +
> +cppcheck:
> +       cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .

If I'm permitted a little GNU make-ism the following might make
CPPCHECK_ADD a bit more usable

+       cppcheck --force --quiet --inline-suppr $(if
$(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .

Which would take us from

$ make cppcheck CPPCHECK_ADD=--enable=all

to

$ make cppcheck CPPCHECK_ADD=all

Re: [RFC/PATCH] Makefile: add cppcheck target

[stefan.naewe]

Am 13.12.2016 um 10:32 schrieb Chris Packham:
> On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham <judge.packham@gmail.com> wrote:
>> Add cppcheck target to Makefile. Cppcheck is a static
>> analysis tool for C/C++ code. Cppcheck primarily detects
>> the types of bugs that the compilers normally do not detect.
>> It is an useful target for doing QA analysis.
>>
>> Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>> ---
>> I had been playing with cppcheck for some other projects and happened to
>> notice [1] in the archives. This is my attempt to resolve the feedback
>> that Junio made at the time.
>>
>> In terms of errors that are actually reported there are only a few
>>
>> $ make cppcheck
>> cppcheck --force --quiet --inline-suppr  .
>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>>
>> The last 2 are just false positives from test data. I haven't looked
>> into any of the others.
>>
>> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
>> in the make invocation.
>>
>> $ make cppcheck CPPCHECK_ADD=--enable=all
>> ... lots of output
>>
>> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t
>>
>>  Makefile | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index f53fcc90d..8b5976d88 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
>>  cover_db_html: cover_db
>>         cover -report html -outputdir cover_db_html cover_db
>>
>> +.PHONY: cppcheck
>> +
>> +cppcheck:
>> +       cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
> 
> If I'm permitted a little GNU make-ism the following might make
> CPPCHECK_ADD a bit more usable
> 
> +       cppcheck --force --quiet --inline-suppr $(if
> $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .
> 
> Which would take us from
> 
> $ make cppcheck CPPCHECK_ADD=--enable=all
> 
> to
> 
> $ make cppcheck CPPCHECK_ADD=all

Hhhmmm....but this allows for only specifying options to '--enable'.
The other version is much more flexible (i.e. allows for other complete options as well).

Just my 0,02€

Stefan
-- 
----------------------------------------------------------------
/dev/random says: I'd love to, but I prefer to remain an enigma.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')" 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

Re: [RFC/PATCH] Makefile: add cppcheck target

[Jeff King]

On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:

> $ make cppcheck
> cppcheck --force --quiet --inline-suppr  .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
> 
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.

I think these last two are a good sign that we need to be feeding the
list of source files to cppcheck. I tried your patch and it also started
looking in t/perf/build, which are old versions of git built to serve
the performance-testing suite.

See the way that the "tags" target is handled for a possible approach.

My main complaint with any static checker is how we can handle false
positives. I think our use of "-Wall -Werror" is successful because it's
not too hard to keep the normal state to zero warnings. Looking at the
output of cppcheck on my system (which is different than on yours!), I
do see a few real problems, but many false positives, too.
Unfortunately, one of the false positives is:

  int foo = foo;

to silence -Wuninitialized, which causes cppcheck to complain that "foo"
is uninitialized. I'm worried we will end up with two static checkers
fighting each other, and no good way to please both.

-Peff

Re: [RFC/PATCH] Makefile: add cppcheck target

[Jeff King]

On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:

> I think these last two are a good sign that we need to be feeding the
> list of source files to cppcheck. I tried your patch and it also started
> looking in t/perf/build, which are old versions of git built to serve
> the performance-testing suite.
> 
> See the way that the "tags" target is handled for a possible approach.

Maybe something like this:

diff --git a/Makefile b/Makefile
index 8b5976d88..e7684ae63 100644
--- a/Makefile
+++ b/Makefile
@@ -2638,4 +2638,6 @@ cover_db_html: cover_db
 .PHONY: cppcheck
 
 cppcheck:
-	cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
+	$(FIND_SOURCE_FILES) |\
+	grep -v ^t/t |\
+	xargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD)

> My main complaint with any static checker is how we can handle false
> positives. [...]

Here's what that generates on my machine, with annotations:

[builtin/am.c:766]: (error) Resource leak: in

  Correct.

[builtin/notes.c:260]: (error) Memory pointed to by 'buf' is freed twice.
[builtin/notes.c:264]: (error) Memory pointed to by 'buf' is freed twice.

  Nope. It appears not to understand that die() does not return.

[builtin/rev-list.c:391]: (error) Uninitialized variable: reaches
[builtin/rev-list.c:391]: (error) Uninitialized variable: all

  These are "int foo = foo" (which is ugly, and maybe we can get rid of
  if compilers have gotten smart enough to figure it out).

[compat/nedmalloc/malloc.c.h:4646]: (error) Memory leak: mem

  Hard to tell, but I think this is wrong and is confused by pointer
  arithmetic on the malloc chunks.

[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset

  Nope, this return is hit only when sbcset is NULL. The tool is
  presumably confused by a conditional hidden inside a macro.

[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset

  I didn't look at these, but presumably similar.

[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size

  Not sure, but it looks like this function declares another inline
  function inside its scope, and that confuses the tool.

[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap

  Nope. Tool seems confused by hiding free() in a macro.

[contrib/examples/builtin-fetch--tool.c:420]: (error) Uninitialized variable: lrr_count
[contrib/examples/builtin-fetch--tool.c:427]: (error) Uninitialized variable: lrr_list

  More "int foo = foo". Might be worth omitting contrib/examples (or
  possibly contrib/ entirely) from the check.

[t/helper/test-hashmap.c:125]: (error) Memory leak: entries
[t/helper/test-hashmap.c:125]: (error) Memory leak: hashes

  Correct (but unimportant).

So I think it is capable of finding real problems, but I think we'd need
some way of squelching false positives, preferably in a way that carries
forward as the code changes (so not just saying "foo.c:1234 is a false
positive", which will break when it becomes "foo.c:1235").

-Peff

Re: [RFC/PATCH] Makefile: add cppcheck target

[Chris Packham]

On Wed, Dec 14, 2016 at 1:28 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:
>
>> I think these last two are a good sign that we need to be feeding the
>> list of source files to cppcheck. I tried your patch and it also started
>> looking in t/perf/build, which are old versions of git built to serve
>> the performance-testing suite.
>>
>> See the way that the "tags" target is handled for a possible approach.
>
> Maybe something like this:
>
> diff --git a/Makefile b/Makefile
> index 8b5976d88..e7684ae63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2638,4 +2638,6 @@ cover_db_html: cover_db
>  .PHONY: cppcheck
>
>  cppcheck:
> -       cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
> +       $(FIND_SOURCE_FILES) |\
> +       grep -v ^t/t |\
> +       xargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD)

Will look at something like this for v2.

>
>> My main complaint with any static checker is how we can handle false
>> positives. [...]

<snip>

> So I think it is capable of finding real problems, but I think we'd need
> some way of squelching false positives, preferably in a way that carries
> forward as the code changes (so not just saying "foo.c:1234 is a false
> positive", which will break when it becomes "foo.c:1235").

If we're prepared to wear them, the --inline-suppr will let us
annotate the code to avoid the false-positives. Suppressions can also
be specified with --suppressions-list=file-with-suppressions but that
would suffer from the moving target problem although you can specify
the file without the line number to squash a class of warning for a
whole file.

Re: [RFC/PATCH] Makefile: add cppcheck target

[Chris Packham]

On Wed, Dec 14, 2016 at 1:15 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:
>
>> $ make cppcheck
>> cppcheck --force --quiet --inline-suppr  .
>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>>
>> The last 2 are just false positives from test data. I haven't looked
>> into any of the others.
>
> I think these last two are a good sign that we need to be feeding the
> list of source files to cppcheck. I tried your patch and it also started
> looking in t/perf/build, which are old versions of git built to serve
> the performance-testing suite.
>
> See the way that the "tags" target is handled for a possible approach.
>
> My main complaint with any static checker is how we can handle false
> positives. I think our use of "-Wall -Werror" is successful because it's
> not too hard to keep the normal state to zero warnings. Looking at the
> output of cppcheck on my system (which is different than on yours!),

I think you get a similar class of problems with different compilers
(different gcc versions, clang, msvc). Although this appears to be
mitigated already with the diverse developers in the git community.

> I do see a few real problems, but many false positives, too.
> Unfortunately, one of the false positives is:
>
>   int foo = foo;

On I side note I have often wondered how this actually works to avoid
the uninitialised-ness of foo. I can see how some compilers may be
fooled into thinking that foo has been set but that doesn't actually
end up with foo having a deterministic value.

> to silence -Wuninitialized, which causes cppcheck to complain that "foo"
> is uninitialized. I'm worried we will end up with two static checkers
> fighting each other, and no good way to please both.
>
> -Peff

Re: [RFC/PATCH] Makefile: add cppcheck target

[Jeff King]

On Wed, Dec 14, 2016 at 09:33:59PM +1300, Chris Packham wrote:

> > I do see a few real problems, but many false positives, too.
> > Unfortunately, one of the false positives is:
> >
> >   int foo = foo;
> 
> On I side note I have often wondered how this actually works to avoid
> the uninitialised-ness of foo. I can see how some compilers may be
> fooled into thinking that foo has been set but that doesn't actually
> end up with foo having a deterministic value.

Right, this is only used to shut up the compiler when it incorrectly
thinks the variable is uninitialized.

-Peff

[RFC/PATCHv2] Makefile: add cppcheck target

[Chris Packham]

Add cppcheck target to Makefile. Cppcheck is a static
analysis tool for C/C++ code. Cppcheck primarily detects
the types of bugs that the compilers normally do not detect.
It is an useful target for doing QA analysis.

To run the default set of checks run

   make cppcheck

Additional checks can be enabled by specifying CPPCHECK_ADD. This is a
comma separated list which is passed to cppcheck's --enable option. To
enable style and warning checks run

  make cppcheck CPPCHECK_ADD=style,warning

Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Changes in v2:
- only run over actual git source files.
- omit any files in t/
- print cppcheck version to allow for better comparison between
  different build environments
- introduce CPPCHECK_FLAGS which can be overridden in the make command
  line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify
  additional checks to be enabled.

 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index f53fcc90d..e5c86decf 100644
--- a/Makefile
+++ b/Makefile
@@ -2635,3 +2635,12 @@ cover_db: coverage-report
 cover_db_html: cover_db
 	cover -report html -outputdir cover_db_html cover_db
 
+.PHONY: cppcheck
+
+CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))
+
+cppcheck:
+	@cppcheck --version
+	$(FIND_SOURCE_FILES) | \
+	grep -v '^t/t' | \
+	xargs cppcheck $(CPPCHECK_FLAGS)
-- 
2.11.0.24.ge6920cf

Re: [RFC/PATCHv2] Makefile: add cppcheck target

[Jeff King]

On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:

> Changes in v2:
> - only run over actual git source files.
> - omit any files in t/

I actually wonder if FIND_SOURCE_FILES should be taking care of the "t/"
thing. I think "make tags" finds tags in t4051/appended1.c, which is
just silly.

> - introduce CPPCHECK_FLAGS which can be overridden in the make command
>   line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify
>   additional checks to be enabled.

The GNU-ism is fine; we already require GNU make to build.

The patch itself is OK to me, I guess. The interesting part will be
whether people start actually _using_ cppcheck and squelching the false
positives. I'm not sure how I feel about the in-code annotations. I'd
have to see a patch first.

-Peff

Re: [RFC/PATCHv2] Makefile: add cppcheck target

[Jeff King]

On Wed, Dec 14, 2016 at 06:24:01AM -0500, Jeff King wrote:

> On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:
> 
> > Changes in v2:
> > - only run over actual git source files.
> > - omit any files in t/
> 
> I actually wonder if FIND_SOURCE_FILES should be taking care of the "t/"
> thing. I think "make tags" finds tags in t4051/appended1.c, which is
> just silly.

I just posted a series[1] that should improve things there. But be aware
that it _also_ adds in shell scripts to the result. So you'd maybe want
to pipe the result through "grep -v '\.sh'" for cppcheck.

-Peff

[1] http://public-inbox.org/git/20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net/t/#u

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

[Chris Packham]

Pass a list of suppressions to cppcheck so that legitimate errors are
more obvious.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

On Thu, Dec 15, 2016 at 12:24 AM, Jeff King <peff@peff.net> wrote:
> The patch itself is OK to me, I guess. The interesting part will be
> whether people start actually _using_ cppcheck and squelching the false
> positives. I'm not sure how I feel about the in-code annotations. I'd
> have to see a patch first.

So here's a patch that adds supression files. It would work well for
things in contrib/compat that don't change that often. It would be a
nightmare to maintain for high-touch code.

 Makefile       | 7 ++++++-
 nedmalloc.supp | 4 ++++
 regcomp.supp   | 8 ++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 nedmalloc.supp
 create mode 100644 regcomp.supp

diff --git a/Makefile b/Makefile
index e5c86decf..bb335ca0f 100644
--- a/Makefile
+++ b/Makefile
@@ -2637,7 +2637,12 @@ cover_db_html: cover_db
 
 .PHONY: cppcheck
 
-CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))
+CPPCHECK_SUPP = --suppressions-list=nedmalloc.supp \
+	--suppressions-list=regcomp.supp
+
+CPPCHECK_FLAGS = --force --quiet --inline-suppr \
+	$(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) \
+	$(CPPCHECK_SUPP)
 
 cppcheck:
 	@cppcheck --version
diff --git a/nedmalloc.supp b/nedmalloc.supp
new file mode 100644
index 000000000..37bd54def
--- /dev/null
+++ b/nedmalloc.supp
@@ -0,0 +1,4 @@
+nullPointer:compat/nedmalloc/malloc.c.h:4093
+nullPointer:compat/nedmalloc/malloc.c.h:4106
+memleak:compat/nedmalloc/malloc.c.h:4646
+
diff --git a/regcomp.supp b/regcomp.supp
new file mode 100644
index 000000000..3ae023c26
--- /dev/null
+++ b/regcomp.supp
@@ -0,0 +1,8 @@
+memleak:compat/regex/regcomp.c:3086
+memleak:compat/regex/regcomp.c:3634
+memleak:compat/regex/regcomp.c:3086
+memleak:compat/regex/regcomp.c:3634
+uninitvar:compat/regex/regcomp.c:2802
+uninitvar:compat/regex/regcomp.c:2805
+memleak:compat/regex/regcomp.c:532
+
-- 
2.11.0.24.ge6920cf

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

[Junio C Hamano]

Chris Packham <judge.packham@gmail.com> writes:

> -CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))
> +CPPCHECK_SUPP = --suppressions-list=nedmalloc.supp \
> +	--suppressions-list=regcomp.supp
> +
> +CPPCHECK_FLAGS = --force --quiet --inline-suppr \
> +	$(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) \
> +	$(CPPCHECK_SUPP)

Has it been agreed that it is a good idea to tie $(CPPCHECK_ADD)
only to --enable?  I somehow thought I saw an objection to this.

> diff --git a/nedmalloc.supp b/nedmalloc.supp
> new file mode 100644
> index 000000000..37bd54def
> --- /dev/null
> +++ b/nedmalloc.supp
> @@ -0,0 +1,4 @@
> +nullPointer:compat/nedmalloc/malloc.c.h:4093
> +nullPointer:compat/nedmalloc/malloc.c.h:4106
> +memleak:compat/nedmalloc/malloc.c.h:4646
> +
> diff --git a/regcomp.supp b/regcomp.supp
> new file mode 100644
> index 000000000..3ae023c26
> --- /dev/null
> +++ b/regcomp.supp
> @@ -0,0 +1,8 @@
> +memleak:compat/regex/regcomp.c:3086
> +memleak:compat/regex/regcomp.c:3634
> +memleak:compat/regex/regcomp.c:3086
> +memleak:compat/regex/regcomp.c:3634
> +uninitvar:compat/regex/regcomp.c:2802
> +uninitvar:compat/regex/regcomp.c:2805
> +memleak:compat/regex/regcomp.c:532
> +

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.


[Footnote]

*1* "Fixing" a real problem it uncovers is a good thing, but it may
    have to also involve working around false positives reported by
    cppcheck, either by rewriting a perfectly correct code to please
    it, or adding these line-number based suppression that is
    totally unworkable.  Like Peff, I'm worried that we will end up
    with two static checkers fighting each other, and no good way to
    please both.

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

[Jeff King]

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

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

Re: [RFC/PATCHv2] Makefile: add cppcheck target

[Chris Packham]

On Fri, Dec 16, 2016 at 9:28 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
> On 14 Dec 2016, at 12:24, Jeff King <peff@peff.net> wrote:
>
> On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:
>
> Changes in v2:
>
> - only run over actual git source files.
>
> - omit any files in t/
>
>
> I actually wonder if FIND_SOURCE_FILES should be taking care of the "t/"
> thing. I think "make tags" finds tags in t4051/appended1.c, which is
> just silly.
>
> - introduce CPPCHECK_FLAGS which can be overridden in the make command
>
>  line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify
>
>  additional checks to be enabled.
>
>
> The GNU-ism is fine; we already require GNU make to build.
>
> The patch itself is OK to me, I guess. The interesting part will be
> whether people start actually _using_ cppcheck and squelching the false
> positives.
>
>
> @Chris: If this gets in then it would be great to run it as part of the
> Travis-CI build: https://travis-ci.org/git/git/branches
>

Yeah I was thinking about this.

Since as always with a new tool there are some doubts over it's
usefulness. I could easily hook it up to a branch in my own fork of
git and keep that branch rebased on top of pu. I'd need to keep an eye
on it myself and report errors on the list.

If that goes well, at some point someone will ask how I'm detecting
these errors. Then I can point them at this patchset and if enough
people want easy access to it then that may provide an incentive for
this to be merged into git.git.