threads / rfc / 44692

RFC patchMakefile: add cppcheck target

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

## tl;dr

15 messages between Dec 13, 2016 and Dec 17, 2016. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Chris Packham· Dec 13, 2016, 09:22 UTC · lore

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(+)
Show changes to Makefile +4 −0
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
Chris Packham· Dec 13, 2016, 09:32 UTC · re: Chris Packham · lore

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

On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham <judge.packham@gmail.com> wrote:
Show 55 quoted lines
> 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
stefan.naewe@atlas-elektronik.com· Dec 13, 2016, 09:37 UTC · re: Chris Packham · lore

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

Am 13.12.2016 um 10:32 schrieb Chris Packham:
Show 70 quoted lines
> 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
Jeff King· Dec 13, 2016, 12:15 UTC · re: Chris Packham · lore

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

On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:
Show 17 quoted lines
> $ 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
Jeff King· Dec 13, 2016, 12:28 UTC · re: Jeff King · lore

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

On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:
Show 6 quoted lines
> 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:
Show changes to Makefile +3 −2
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
Chris Packham· Dec 14, 2016, 08:23 UTC · re: Jeff King · lore

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

On Wed, Dec 14, 2016 at 1:28 AM, Jeff King <peff@peff.net> wrote:
Show 23 quoted lines
> 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.

Chris Packham· Dec 14, 2016, 08:33 UTC · re: Jeff King · lore

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

On Wed, Dec 14, 2016 at 1:15 AM, Jeff King <peff@peff.net> wrote:
Show 31 quoted lines
> 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.

Show 5 quoted lines
> 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
Jeff King· Dec 14, 2016, 11:18 UTC · re: Chris Packham · lore

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

On Wed, Dec 14, 2016 at 09:33:59PM +1300, Chris Packham wrote:
Show 9 quoted lines
> > 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
Chris Packham· Dec 14, 2016, 09:27 UTC · re: Chris Packham · lore

[RFC/PATCHv2] Makefile: add cppcheck target

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(+)
Show changes to Makefile +9 −0
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
Jeff King· Dec 14, 2016, 11:24 UTC · re: Chris Packham · lore

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

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
Jeff King· Dec 14, 2016, 14:46 UTC · re: Jeff King · lore

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

On Wed, Dec 14, 2016 at 06:24:01AM -0500, Jeff King wrote:
Show 9 quoted lines
> 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
Chris Packham· Dec 15, 2016, 23:22 UTC · re: Jeff King · lore

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

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
Show changes to 3 files +18 −1

Makefile, nedmalloc.supp, 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
Junio C Hamano· Dec 16, 2016, 18:43 UTC · re: Chris Packham · lore

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

Chris Packham <judge.packham@gmail.com> writes:
Show 7 quoted lines
> -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.

Show 24 quoted lines
> 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.
Jeff King· Dec 16, 2016, 22:16 UTC · re: Junio C Hamano · lore

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

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
Chris Packham· Dec 17, 2016, 07:31 UTC · lore

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

On Fri, Dec 16, 2016 at 9:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 33 quoted lines
>
> 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.

← back to recent threads