{"thread":{"id":"44692","subject":"[RFC/PATCH] Makefile: add cppcheck target","startedAt":"2016-12-13T09:22:43Z","lastAt":"2016-12-17T07:32:02Z","messageCount":15,"participants":["Chris Packham","stefan.naewe@atlas-elektronik.com","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307582","messageId":"20161213092225.15299-1-judge.packham@gmail.com","threadId":"44692","inReplyTo":null,"subject":"[RFC/PATCH] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-13T09:22:25Z","receivedAt":"2016-12-13T09:22:43Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Add cppcheck target to Makefile. Cppcheck is a static\nanalysis tool for C/C++ code. Cppcheck primarily detects\nthe types of bugs that the compilers normally do not detect.\nIt is an useful target for doing QA analysis.\n\nBased-on-patch-by: Elia Pinto \nSigned-off-by: Chris Packham \n---\nI had been playing with cppcheck for some other projects and happened to\nnotice [1] in the archives. This is my attempt to resolve the feedback\nthat Junio made at the time.\n\nIn terms of errors that are actually reported there are only a few\n\n$ make cppcheck\ncppcheck --force --quiet --inline-suppr .\n[compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp\n[compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp\n[compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects\n[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n[t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.\n[t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.\n\nThe last 2 are just false positives from test data. I haven't looked\ninto any of the others.\n\nI've also provisioned for enabling extra checks by passing CPPCHECK_ADD\nin the make invocation.\n\n$ make cppcheck CPPCHECK_ADD=--enable=all\n... lots of output\n \n[1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t\n\n Makefile | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex f53fcc90d..8b5976d88 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2635,3 +2635,7 @@ cover_db: coverage-report\n cover_db_html: cover_db\n \tcover -report html -outputdir cover_db_html cover_db\n \n+.PHONY: cppcheck\n+\n+cppcheck:\n+\tcppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .\n-- \n2.11.0.24.ge6920cf\n\n"},{"id":"307583","messageId":"CAFOYHZAZAH9Rt1o73cx2uFvtr4weL00J+Yktei3h2GN1JgbY=A@mail.gmail.com","threadId":"44692","inReplyTo":"20161213092225.15299-1-judge.packham@gmail.com","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-13T09:32:19Z","receivedAt":"2016-12-13T09:32:29Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham wrote:\n> Add cppcheck target to Makefile. Cppcheck is a static\n> analysis tool for C/C++ code. Cppcheck primarily detects\n> the types of bugs that the compilers normally do not detect.\n> It is an useful target for doing QA analysis.\n>\n> Based-on-patch-by: Elia Pinto \n> Signed-off-by: Chris Packham \n> ---\n> I had been playing with cppcheck for some other projects and happened to\n> notice [1] in the archives. This is my attempt to resolve the feedback\n> that Junio made at the time.\n>\n> In terms of errors that are actually reported there are only a few\n>\n> $ make cppcheck\n> cppcheck --force --quiet --inline-suppr .\n> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp\n> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp\n> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects\n> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.\n> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.\n>\n> The last 2 are just false positives from test data. I haven't looked\n> into any of the others.\n>\n> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD\n> in the make invocation.\n>\n> $ make cppcheck CPPCHECK_ADD=--enable=all\n> ... lots of output\n>\n> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t\n>\n> Makefile | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index f53fcc90d..8b5976d88 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2635,3 +2635,7 @@ cover_db: coverage-report\n> cover_db_html: cover_db\n> cover -report html -outputdir cover_db_html cover_db\n>\n> +.PHONY: cppcheck\n> +\n> +cppcheck:\n> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .\n\nIf I'm permitted a little GNU make-ism the following might make\nCPPCHECK_ADD a bit more usable\n\n+ cppcheck --force --quiet --inline-suppr $(if\n$(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .\n\nWhich would take us from\n\n$ make cppcheck CPPCHECK_ADD=--enable=all\n\nto\n\n$ make cppcheck CPPCHECK_ADD=all\n"},{"id":"307584","messageId":"ae32eb3f-72b4-0352-d035-701a142b452b@atlas-elektronik.com","threadId":"44692","inReplyTo":"CAFOYHZAZAH9Rt1o73cx2uFvtr4weL00J+Yktei3h2GN1JgbY=A@mail.gmail.com","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2016-12-13T09:37:13Z","receivedAt":"2016-12-13T09:39:12Z","isPatch":true,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Am 13.12.2016 um 10:32 schrieb Chris Packham:\n> On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham wrote:\n>> Add cppcheck target to Makefile. Cppcheck is a static\n>> analysis tool for C/C++ code. Cppcheck primarily detects\n>> the types of bugs that the compilers normally do not detect.\n>> It is an useful target for doing QA analysis.\n>>\n>> Based-on-patch-by: Elia Pinto \n>> Signed-off-by: Chris Packham \n>> ---\n>> I had been playing with cppcheck for some other projects and happened to\n>> notice [1] in the archives. This is my attempt to resolve the feedback\n>> that Junio made at the time.\n>>\n>> In terms of errors that are actually reported there are only a few\n>>\n>> $ make cppcheck\n>> cppcheck --force --quiet --inline-suppr .\n>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp\n>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp\n>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects\n>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.\n>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.\n>>\n>> The last 2 are just false positives from test data. I haven't looked\n>> into any of the others.\n>>\n>> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD\n>> in the make invocation.\n>>\n>> $ make cppcheck CPPCHECK_ADD=--enable=all\n>> ... lots of output\n>>\n>> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t\n>>\n>> Makefile | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index f53fcc90d..8b5976d88 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2635,3 +2635,7 @@ cover_db: coverage-report\n>> cover_db_html: cover_db\n>> cover -report html -outputdir cover_db_html cover_db\n>>\n>> +.PHONY: cppcheck\n>> +\n>> +cppcheck:\n>> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .\n> \n> If I'm permitted a little GNU make-ism the following might make\n> CPPCHECK_ADD a bit more usable\n> \n> + cppcheck --force --quiet --inline-suppr $(if\n> $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .\n> \n> Which would take us from\n> \n> $ make cppcheck CPPCHECK_ADD=--enable=all\n> \n> to\n> \n> $ make cppcheck CPPCHECK_ADD=all\n\nHhhmmm....but this allows for only specifying options to '--enable'.\nThe other version is much more flexible (i.e. allows for other complete options as well).\n\nJust my 0,02€\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: I'd love to, but I prefer to remain an enigma.\npython -c \"print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')\" \nGPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF"},{"id":"307592","messageId":"20161213121510.5o5axuwzztbxcvfd@sigill.intra.peff.net","threadId":"44692","inReplyTo":"20161213092225.15299-1-judge.packham@gmail.com","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-13T12:15:10Z","receivedAt":"2016-12-13T12:15:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:\n\n> $ make cppcheck\n> cppcheck --force --quiet --inline-suppr .\n> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp\n> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp\n> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects\n> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.\n> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.\n> \n> The last 2 are just false positives from test data. I haven't looked\n> into any of the others.\n\nI think these last two are a good sign that we need to be feeding the\nlist of source files to cppcheck. I tried your patch and it also started\nlooking in t/perf/build, which are old versions of git built to serve\nthe performance-testing suite.\n\nSee the way that the \"tags\" target is handled for a possible approach.\n\nMy main complaint with any static checker is how we can handle false\npositives. I think our use of \"-Wall -Werror\" is successful because it's\nnot too hard to keep the normal state to zero warnings. Looking at the\noutput of cppcheck on my system (which is different than on yours!), I\ndo see a few real problems, but many false positives, too.\nUnfortunately, one of the false positives is:\n\n int foo = foo;\n\nto silence -Wuninitialized, which causes cppcheck to complain that \"foo\"\nis uninitialized. I'm worried we will end up with two static checkers\nfighting each other, and no good way to please both.\n\n-Peff\n"},{"id":"307593","messageId":"20161213122854.pphyp342tstxbbqe@sigill.intra.peff.net","threadId":"44692","inReplyTo":"20161213121510.5o5axuwzztbxcvfd@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-13T12:28:54Z","receivedAt":"2016-12-13T12:29:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:\n\n> I think these last two are a good sign that we need to be feeding the\n> list of source files to cppcheck. I tried your patch and it also started\n> looking in t/perf/build, which are old versions of git built to serve\n> the performance-testing suite.\n> \n> See the way that the \"tags\" target is handled for a possible approach.\n\nMaybe something like this:\n\ndiff --git a/Makefile b/Makefile\nindex 8b5976d88..e7684ae63 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2638,4 +2638,6 @@ cover_db_html: cover_db\n .PHONY: cppcheck\n \n cppcheck:\n-\tcppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .\n+\t$(FIND_SOURCE_FILES) |\\\n+\tgrep -v ^t/t |\\\n+\txargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD)\n\n> My main complaint with any static checker is how we can handle false\n> positives. [...]\n\nHere's what that generates on my machine, with annotations:\n\n[builtin/am.c:766]: (error) Resource leak: in\n\n Correct.\n\n[builtin/notes.c:260]: (error) Memory pointed to by 'buf' is freed twice.\n[builtin/notes.c:264]: (error) Memory pointed to by 'buf' is freed twice.\n\n Nope. It appears not to understand that die() does not return.\n\n[builtin/rev-list.c:391]: (error) Uninitialized variable: reaches\n[builtin/rev-list.c:391]: (error) Uninitialized variable: all\n\n These are \"int foo = foo\" (which is ugly, and maybe we can get rid of\n if compilers have gotten smart enough to figure it out).\n\n[compat/nedmalloc/malloc.c.h:4646]: (error) Memory leak: mem\n\n Hard to tell, but I think this is wrong and is confused by pointer\n arithmetic on the malloc chunks.\n\n[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n\n Nope, this return is hit only when sbcset is NULL. The tool is\n presumably confused by a conditional hidden inside a macro.\n\n[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n\n I didn't look at these, but presumably similar.\n\n[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n\n Not sure, but it looks like this function declares another inline\n function inside its scope, and that confuses the tool.\n\n[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n\n Nope. Tool seems confused by hiding free() in a macro.\n\n[contrib/examples/builtin-fetch--tool.c:420]: (error) Uninitialized variable: lrr_count\n[contrib/examples/builtin-fetch--tool.c:427]: (error) Uninitialized variable: lrr_list\n\n More \"int foo = foo\". Might be worth omitting contrib/examples (or\n possibly contrib/ entirely) from the check.\n\n[t/helper/test-hashmap.c:125]: (error) Memory leak: entries\n[t/helper/test-hashmap.c:125]: (error) Memory leak: hashes\n\n Correct (but unimportant).\n\nSo I think it is capable of finding real problems, but I think we'd need\nsome way of squelching false positives, preferably in a way that carries\nforward as the code changes (so not just saying \"foo.c:1234 is a false\npositive\", which will break when it becomes \"foo.c:1235\").\n\n-Peff\n"},{"id":"307742","messageId":"CAFOYHZD5iUzXFH6CFCKhG8UmQb8q0CiUZFSBAeicUmjSt9mgig@mail.gmail.com","threadId":"44692","inReplyTo":"20161213122854.pphyp342tstxbbqe@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-14T08:23:53Z","receivedAt":"2016-12-14T08:25:30Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Dec 14, 2016 at 1:28 AM, Jeff King wrote:\n> On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:\n>\n>> I think these last two are a good sign that we need to be feeding the\n>> list of source files to cppcheck. I tried your patch and it also started\n>> looking in t/perf/build, which are old versions of git built to serve\n>> the performance-testing suite.\n>>\n>> See the way that the \"tags\" target is handled for a possible approach.\n>\n> Maybe something like this:\n>\n> diff --git a/Makefile b/Makefile\n> index 8b5976d88..e7684ae63 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2638,4 +2638,6 @@ cover_db_html: cover_db\n> .PHONY: cppcheck\n>\n> cppcheck:\n> - cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .\n> + $(FIND_SOURCE_FILES) |\\\n> + grep -v ^t/t |\\\n> + xargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD)\n\nWill look at something like this for v2.\n\n>\n>> My main complaint with any static checker is how we can handle false\n>> positives. [...]\n\n\n\n> So I think it is capable of finding real problems, but I think we'd need\n> some way of squelching false positives, preferably in a way that carries\n> forward as the code changes (so not just saying \"foo.c:1234 is a false\n> positive\", which will break when it becomes \"foo.c:1235\").\n\nIf we're prepared to wear them, the --inline-suppr will let us\nannotate the code to avoid the false-positives. Suppressions can also\nbe specified with --suppressions-list=file-with-suppressions but that\nwould suffer from the moving target problem although you can specify\nthe file without the line number to squash a class of warning for a\nwhole file.\n"},{"id":"307743","messageId":"CAFOYHZBRq=5FGswQZSYbn0JdrEv+xqLm6gdpD_nE+1L_CfPHEw@mail.gmail.com","threadId":"44692","inReplyTo":"20161213121510.5o5axuwzztbxcvfd@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-14T08:33:59Z","receivedAt":"2016-12-14T08:34:05Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Dec 14, 2016 at 1:15 AM, Jeff King wrote:\n> On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:\n>\n>> $ make cppcheck\n>> cppcheck --force --quiet --inline-suppr .\n>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp\n>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp\n>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects\n>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset\n>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset\n>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset\n>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset\n>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size\n>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size\n>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap\n>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.\n>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.\n>>\n>> The last 2 are just false positives from test data. I haven't looked\n>> into any of the others.\n>\n> I think these last two are a good sign that we need to be feeding the\n> list of source files to cppcheck. I tried your patch and it also started\n> looking in t/perf/build, which are old versions of git built to serve\n> the performance-testing suite.\n>\n> See the way that the \"tags\" target is handled for a possible approach.\n>\n> My main complaint with any static checker is how we can handle false\n> positives. I think our use of \"-Wall -Werror\" is successful because it's\n> not too hard to keep the normal state to zero warnings. Looking at the\n> output of cppcheck on my system (which is different than on yours!),\n\nI think you get a similar class of problems with different compilers\n(different gcc versions, clang, msvc). Although this appears to be\nmitigated already with the diverse developers in the git community.\n\n> I do see a few real problems, but many false positives, too.\n> Unfortunately, one of the false positives is:\n>\n> int foo = foo;\n\nOn I side note I have often wondered how this actually works to avoid\nthe uninitialised-ness of foo. I can see how some compilers may be\nfooled into thinking that foo has been set but that doesn't actually\nend up with foo having a deterministic value.\n\n> to silence -Wuninitialized, which causes cppcheck to complain that \"foo\"\n> is uninitialized. I'm worried we will end up with two static checkers\n> fighting each other, and no good way to please both.\n>\n> -Peff\n"},{"id":"307748","messageId":"20161214092731.29076-1-judge.packham@gmail.com","threadId":"44692","inReplyTo":"20161213092225.15299-1-judge.packham@gmail.com","subject":"[RFC/PATCHv2] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-14T09:27:31Z","receivedAt":"2016-12-14T09:27:59Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Add cppcheck target to Makefile. Cppcheck is a static\nanalysis tool for C/C++ code. Cppcheck primarily detects\nthe types of bugs that the compilers normally do not detect.\nIt is an useful target for doing QA analysis.\n\nTo run the default set of checks run\n\n make cppcheck\n\nAdditional checks can be enabled by specifying CPPCHECK_ADD. This is a\ncomma separated list which is passed to cppcheck's --enable option. To\nenable style and warning checks run\n\n make cppcheck CPPCHECK_ADD=style,warning\n\nBased-on-patch-by: Elia Pinto \nSigned-off-by: Chris Packham \n---\nChanges in v2:\n- only run over actual git source files.\n- omit any files in t/\n- print cppcheck version to allow for better comparison between\n different build environments\n- introduce CPPCHECK_FLAGS which can be overridden in the make command\n line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify\n additional checks to be enabled.\n\n Makefile | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex f53fcc90d..e5c86decf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2635,3 +2635,12 @@ cover_db: coverage-report\n cover_db_html: cover_db\n \tcover -report html -outputdir cover_db_html cover_db\n \n+.PHONY: cppcheck\n+\n+CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))\n+\n+cppcheck:\n+\t@cppcheck --version\n+\t$(FIND_SOURCE_FILES) | \\\n+\tgrep -v '^t/t' | \\\n+\txargs cppcheck $(CPPCHECK_FLAGS)\n-- \n2.11.0.24.ge6920cf\n\n"},{"id":"307749","messageId":"20161214111856.nhtq4l3ntrhy2fhv@sigill.intra.peff.net","threadId":"44692","inReplyTo":"CAFOYHZBRq=5FGswQZSYbn0JdrEv+xqLm6gdpD_nE+1L_CfPHEw@mail.gmail.com","subject":"Re: [RFC/PATCH] Makefile: add cppcheck target","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-14T11:18:57Z","receivedAt":"2016-12-14T11:19:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2016 at 09:33:59PM +1300, Chris Packham wrote:\n\n> > I do see a few real problems, but many false positives, too.\n> > Unfortunately, one of the false positives is:\n> >\n> > int foo = foo;\n> \n> On I side note I have often wondered how this actually works to avoid\n> the uninitialised-ness of foo. I can see how some compilers may be\n> fooled into thinking that foo has been set but that doesn't actually\n> end up with foo having a deterministic value.\n\nRight, this is only used to shut up the compiler when it incorrectly\nthinks the variable is uninitialized.\n\n-Peff\n"},{"id":"307750","messageId":"20161214112401.mq3n5kui5eeebdtk@sigill.intra.peff.net","threadId":"44692","inReplyTo":"20161214092731.29076-1-judge.packham@gmail.com","subject":"Re: [RFC/PATCHv2] Makefile: add cppcheck target","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-14T11:24:01Z","receivedAt":"2016-12-14T11:24:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:\n\n> Changes in v2:\n> - only run over actual git source files.\n> - omit any files in t/\n\nI actually wonder if FIND_SOURCE_FILES should be taking care of the \"t/\"\nthing. I think \"make tags\" finds tags in t4051/appended1.c, which is\njust silly.\n\n> - introduce CPPCHECK_FLAGS which can be overridden in the make command\n> line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify\n> additional checks to be enabled.\n\nThe GNU-ism is fine; we already require GNU make to build.\n\nThe patch itself is OK to me, I guess. The interesting part will be\nwhether people start actually _using_ cppcheck and squelching the false\npositives. I'm not sure how I feel about the in-code annotations. I'd\nhave to see a patch first.\n\n-Peff\n"},{"id":"307779","messageId":"20161214144619.j3koxecsricuiio5@sigill.intra.peff.net","threadId":"44692","inReplyTo":"20161214112401.mq3n5kui5eeebdtk@sigill.intra.peff.net","subject":"Re: [RFC/PATCHv2] Makefile: add cppcheck target","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-14T14:46:19Z","receivedAt":"2016-12-14T14:46:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2016 at 06:24:01AM -0500, Jeff King wrote:\n\n> On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:\n> \n> > Changes in v2:\n> > - only run over actual git source files.\n> > - omit any files in t/\n> \n> I actually wonder if FIND_SOURCE_FILES should be taking care of the \"t/\"\n> thing. I think \"make tags\" finds tags in t4051/appended1.c, which is\n> just silly.\n\nI just posted a series[1] that should improve things there. But be aware\nthat it _also_ adds in shell scripts to the result. So you'd maybe want\nto pipe the result through \"grep -v '\\.sh'\" for cppcheck.\n\n-Peff\n\n[1] http://public-inbox.org/git/20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net/t/#u\n"},{"id":"307868","messageId":"20161215232240.19427-1-judge.packham@gmail.com","threadId":"44692","inReplyTo":"20161214112401.mq3n5kui5eeebdtk@sigill.intra.peff.net","subject":"[RFC/PATCH] Makefile: suppress some cppcheck false-positives","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-15T23:22:40Z","receivedAt":"2016-12-15T23:23:35Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Pass a list of suppressions to cppcheck so that legitimate errors are\nmore obvious.\n\nSigned-off-by: Chris Packham \n---\n\nOn Thu, Dec 15, 2016 at 12:24 AM, Jeff King wrote:\n> The patch itself is OK to me, I guess. The interesting part will be\n> whether people start actually _using_ cppcheck and squelching the false\n> positives. I'm not sure how I feel about the in-code annotations. I'd\n> have to see a patch first.\n\nSo here's a patch that adds supression files. It would work well for\nthings in contrib/compat that don't change that often. It would be a\nnightmare to maintain for high-touch code.\n\n Makefile | 7 ++++++-\n nedmalloc.supp | 4 ++++\n regcomp.supp | 8 ++++++++\n 3 files changed, 18 insertions(+), 1 deletion(-)\n create mode 100644 nedmalloc.supp\n create mode 100644 regcomp.supp\n\ndiff --git a/Makefile b/Makefile\nindex e5c86decf..bb335ca0f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2637,7 +2637,12 @@ cover_db_html: cover_db\n \n .PHONY: cppcheck\n \n-CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))\n+CPPCHECK_SUPP = --suppressions-list=nedmalloc.supp \\\n+\t--suppressions-list=regcomp.supp\n+\n+CPPCHECK_FLAGS = --force --quiet --inline-suppr \\\n+\t$(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) \\\n+\t$(CPPCHECK_SUPP)\n \n cppcheck:\n \t@cppcheck --version\ndiff --git a/nedmalloc.supp b/nedmalloc.supp\nnew file mode 100644\nindex 000000000..37bd54def\n--- /dev/null\n+++ b/nedmalloc.supp\n@@ -0,0 +1,4 @@\n+nullPointer:compat/nedmalloc/malloc.c.h:4093\n+nullPointer:compat/nedmalloc/malloc.c.h:4106\n+memleak:compat/nedmalloc/malloc.c.h:4646\n+\ndiff --git a/regcomp.supp b/regcomp.supp\nnew file mode 100644\nindex 000000000..3ae023c26\n--- /dev/null\n+++ b/regcomp.supp\n@@ -0,0 +1,8 @@\n+memleak:compat/regex/regcomp.c:3086\n+memleak:compat/regex/regcomp.c:3634\n+memleak:compat/regex/regcomp.c:3086\n+memleak:compat/regex/regcomp.c:3634\n+uninitvar:compat/regex/regcomp.c:2802\n+uninitvar:compat/regex/regcomp.c:2805\n+memleak:compat/regex/regcomp.c:532\n+\n-- \n2.11.0.24.ge6920cf\n\n"},{"id":"307898","messageId":"xmqqshpnsoij.fsf@gitster.mtv.corp.google.com","threadId":"44692","inReplyTo":"20161215232240.19427-1-judge.packham@gmail.com","subject":"Re: [RFC/PATCH] Makefile: suppress some cppcheck false-positives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T18:43:48Z","receivedAt":"2016-12-16T18:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Packham writes:\n\n> -CPPCHECK_FLAGS = --force --quiet --inline-suppr $(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD))\n> +CPPCHECK_SUPP = --suppressions-list=nedmalloc.supp \\\n> +\t--suppressions-list=regcomp.supp\n> +\n> +CPPCHECK_FLAGS = --force --quiet --inline-suppr \\\n> +\t$(if $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) \\\n> +\t$(CPPCHECK_SUPP)\n\nHas it been agreed that it is a good idea to tie $(CPPCHECK_ADD)\nonly to --enable? I somehow thought I saw an objection to this.\n\n> diff --git a/nedmalloc.supp b/nedmalloc.supp\n> new file mode 100644\n> index 000000000..37bd54def\n> --- /dev/null\n> +++ b/nedmalloc.supp\n> @@ -0,0 +1,4 @@\n> +nullPointer:compat/nedmalloc/malloc.c.h:4093\n> +nullPointer:compat/nedmalloc/malloc.c.h:4106\n> +memleak:compat/nedmalloc/malloc.c.h:4646\n> +\n> diff --git a/regcomp.supp b/regcomp.supp\n> new file mode 100644\n> index 000000000..3ae023c26\n> --- /dev/null\n> +++ b/regcomp.supp\n> @@ -0,0 +1,8 @@\n> +memleak:compat/regex/regcomp.c:3086\n> +memleak:compat/regex/regcomp.c:3634\n> +memleak:compat/regex/regcomp.c:3086\n> +memleak:compat/regex/regcomp.c:3634\n> +uninitvar:compat/regex/regcomp.c:2802\n> +uninitvar:compat/regex/regcomp.c:2805\n> +memleak:compat/regex/regcomp.c:532\n> +\n\nYuck for both files for multiple reasons. \n\nI do not think it is a good idea to allow these files to clutter the\ntop-level of tree. How often do we expect that we may have to add\nmore of these files? Every time we start borrowing code from third\nparties?\n\nWhat is the goal we want to achieve by running cppcheck? \n\n a. Our code must be clean but we do not bother \"fixing\" [*1*] the\n code we borrow from third parties and squelch output instead?\n\n b. Both our own code and third party code we borrow need to be free\n of errors and misdetections from cppcheck?\n\n c. Something else?\n\nIf a. is what we aim for, perhaps a better option may be not to run\ncppcheck for the code we borrowed from third-party at all in the\nfirst place. \n\nIf b. is our goal, we need to make sure that the false positive rate\nof cppcheck is acceptably low.\n\n\n[Footnote]\n\n*1* \"Fixing\" a real problem it uncovers is a good thing, but it may\n have to also involve working around false positives reported by\n cppcheck, either by rewriting a perfectly correct code to please\n it, or adding these line-number based suppression that is\n totally unworkable. Like Peff, I'm worried that we will end up\n with two static checkers fighting each other, and no good way to\n please both.\n"},{"id":"307928","messageId":"20161216221612.whxm3z3clhi4xcha@sigill.intra.peff.net","threadId":"44692","inReplyTo":"xmqqshpnsoij.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] Makefile: suppress some cppcheck false-positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T22:16:12Z","receivedAt":"2016-12-16T22:16:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 10:43:48AM -0800, Junio C Hamano wrote:\n\n> > diff --git a/nedmalloc.supp b/nedmalloc.supp\n> [...]\n> > diff --git a/regcomp.supp b/regcomp.supp\n> \n> Yuck for both files for multiple reasons.\n> \n> I do not think it is a good idea to allow these files to clutter the\n> top-level of tree. How often do we expect that we may have to add\n> more of these files? Every time we start borrowing code from third\n> parties?\n> \n> What is the goal we want to achieve by running cppcheck? \n> \n> a. Our code must be clean but we do not bother \"fixing\" [*1*] the\n> code we borrow from third parties and squelch output instead?\n> \n> b. Both our own code and third party code we borrow need to be free\n> of errors and misdetections from cppcheck?\n> \n> c. Something else?\n> \n> If a. is what we aim for, perhaps a better option may be not to run\n> cppcheck for the code we borrowed from third-party at all in the\n> first place. \n> \n> If b. is our goal, we need to make sure that the false positive rate\n> of cppcheck is acceptably low.\n\nI think (b) is the goal; we'd hope that both our code and third party\ncode would be bug-free. I think it's a fact of life with a static\nanalysis tool that we're going to have to silence some false positives.\n\nI think Chris started with the ones in compat because we are pretty sure\nthey won't change much, so suppressing them by line number is easy. But\nwe'd need to revisit this for our code, too. So just turning it off for\ncompat/ is only punting on the problem for a little while. :)\n\nI do think it would be less gross if we could put these files into\ncompat/nedmalloc, etc. I don't know if you can specify\n--suppressions-list multiple times, but certainly we could do some\npre-processing like:\n\n find . -name '*.cppcheck' |\n while read suppfile; do\n\tdir=$(dirname $suppfile)\n\tsed \"s{^}{$dir/}\" <$suppfile\n done >master-suppfile\n cppcheck --suppressions-file=master-suppfile\n\nThat would at least let us drop individual suppression files into their\nrespective directories.\n\nI do wonder, though, if the \"inline\" suppressions would be less painful.\nIt looks like doing:\n\n // cppcheck-suppress uninitialized\n int var = var;\n\nwould work. I'm not sure if it understands non-C99 comments, though\nmaybe it is time for us to loosen that rule. And suppressing the false\npositives that way does avoid fighting with gcc's analyzer, since we're\nnot changing the code.\n\nThe real question is how often we'd have to sprinkle those comments, and\nhow painful it would be. I see only 4 false positives that need\nsuppressed in our code, but 2 of them rub me the wrong way. They are due\nto the tool failing to realize that die() is marked with NORETURN.\nMarking some site as \"no, this isn't a double-free, the other code path\nwould have died\" feels like the wrong spot. The tool failure isn't where\nwe're marking, but rather 10 lines above.\n\n-Peff\n"},{"id":"307949","messageId":"CAFOYHZDSyFxMWqrHNEX+hb1N4iNbe7baO1pS3n9hSb+Hk7WrgA@mail.gmail.com","threadId":"44692","inReplyTo":"6ABA4AA4-BD5C-4178-BB3B-91CA045EA2AD@gmail.com","subject":"Re: [RFC/PATCHv2] Makefile: add cppcheck target","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2016-12-17T07:31:55Z","receivedAt":"2016-12-17T07:32:02Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Fri, Dec 16, 2016 at 9:28 PM, Lars Schneider\n wrote:\n>\n> On 14 Dec 2016, at 12:24, Jeff King wrote:\n>\n> On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:\n>\n> Changes in v2:\n>\n> - only run over actual git source files.\n>\n> - omit any files in t/\n>\n>\n> I actually wonder if FIND_SOURCE_FILES should be taking care of the \"t/\"\n> thing. I think \"make tags\" finds tags in t4051/appended1.c, which is\n> just silly.\n>\n> - introduce CPPCHECK_FLAGS which can be overridden in the make command\n>\n> line. This also uses a GNU make-ism to allow CPPCHECK_ADD to specify\n>\n> additional checks to be enabled.\n>\n>\n> The GNU-ism is fine; we already require GNU make to build.\n>\n> The patch itself is OK to me, I guess. The interesting part will be\n> whether people start actually _using_ cppcheck and squelching the false\n> positives.\n>\n>\n> @Chris: If this gets in then it would be great to run it as part of the\n> Travis-CI build: https://travis-ci.org/git/git/branches\n>\n\nYeah I was thinking about this.\n\nSince as always with a new tool there are some doubts over it's\nusefulness. I could easily hook it up to a branch in my own fork of\ngit and keep that branch rebased on top of pu. I'd need to keep an eye\non it myself and report errors on the list.\n\nIf that goes well, at some point someone will ask how I'm detecting\nthese errors. Then I can point them at this patchset and if enough\npeople want easy access to it then that may provide an incentive for\nthis to be merged into git.git.\n"}]}