{"thread":{"id":"48941","subject":"[PATCH 0/5] Misc Coccinelle-related improvements","startedAt":"2018-07-23T13:51:21Z","lastAt":"2018-08-05T23:02:51Z","messageCount":31,"participants":["SZEDER Gábor","Derrick Stolee","Duy Nguyen","René Scharfe","Eric Sunshine","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason","Jonathan Nieder","Julia Lawall"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"353347","messageId":"20180723135100.24288-1-szeder.dev@gmail.com","threadId":"48941","inReplyTo":null,"subject":"[PATCH 0/5] Misc Coccinelle-related improvements","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:50:55Z","receivedAt":"2018-07-23T13:51:21Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Just a couple of minor Coccinelle-related improvements:\n\n - The first two patches are small cleanups.\n\n - The last three could make life perhaps just a tad bit easier for\n devs running 'make coccicheck'.\n\n\nSZEDER Gábor (5):\n coccinelle: mark the 'coccicheck' make target as .PHONY\n coccinelle: use $(addsuffix) in 'coccicheck' make target\n coccinelle: exclude sha1dc source files from static analysis\n coccinelle: put sane filenames into output patches\n coccinelle: extract dedicated make target to clean Coccinelle's\n results\n\n Makefile | 24 +++++++++++++++++-------\n 1 file changed, 17 insertions(+), 7 deletions(-)\n\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353348","messageId":"20180723135100.24288-2-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PATCH 1/5] coccinelle: mark the 'coccicheck' make target as .PHONY","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:50:56Z","receivedAt":"2018-07-23T13:51:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The 'coccicheck' target doesn't create a file with the same name, so\nmark it as .PHONY.\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex e4b503d259..27bfc196dd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2685,6 +2685,8 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n \tfi\n coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))\n \n+.PHONY: coccicheck\n+\n ### Installation rules\n \n ifneq ($(filter /%,$(firstword $(template_dir))),)\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353349","messageId":"20180723135100.24288-3-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PATCH 2/5] coccinelle: use $(addsuffix) in 'coccicheck' make target","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:50:57Z","receivedAt":"2018-07-23T13:51:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The dependencies of the 'coccicheck' make target are listed with the\nhelp of the $(patsubst) make function, which in this case doesn't do\nany pattern substitution, but only adds the '.patch' suffix.\n\nUse the shorter and more idiomatic $(addsuffix) make function instead.\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 27bfc196dd..8f509576e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2683,7 +2683,7 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n \tthen \\\n \t\techo ' ' SPATCH result: $@; \\\n \tfi\n-coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))\n+coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n \n .PHONY: coccicheck\n \n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353350","messageId":"20180723135100.24288-4-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PATCH 3/5] coccinelle: exclude sha1dc source files from static analysis","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:50:58Z","receivedAt":"2018-07-23T13:51:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"sha1dc is an external library, that we carry in-tree for convenience\nor grab as a submodule, so there is no use in applying our semantic\npatches to its source files.\n\nTherefore, exclude sha1dc's source files from Coccinelle's static\nanalysis.\n\nThis change also makes the static analysis somewhat faster: presumably\nbecause of the heavy use of repetitive macro declarations, applying\nthe semantic patches 'array.cocci' and 'swap.cocci' to 'sha1dc/sha1.c'\ntakes over half a minute each on my machine, which amounts to about a\nthird of the runtime of applying these two semantic patches to the\nwhole git source tree.\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 8f509576e9..73e2d16926 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2666,10 +2666,16 @@ check: command-list.h\n \tfi\n \n C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n-%.cocci.patch: %.cocci $(C_SOURCES)\n+ifdef DC_SHA1_SUBMODULE\n+COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n+else\n+COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n+endif\n+\n+%.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t@echo ' ' SPATCH $<; \\\n \tret=0; \\\n-\tfor f in $(C_SOURCES); do \\\n+\tfor f in $(COCCI_SOURCES); do \\\n \t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n \t\t\t{ ret=$$?; break; }; \\\n \tdone >$@+ 2>$@.log; \\\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353351","messageId":"20180723135100.24288-5-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PATCH 4/5] coccinelle: put sane filenames into output patches","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:50:59Z","receivedAt":"2018-07-23T13:51:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Coccinelle outputs its suggested transformations as patches, whose\nheader looks something like this:\n\n --- commit.c\n +++ /tmp/cocci-output-19250-7ae78a-commit.c\n\nNote the lack of 'diff --opts ' line, the differing number\nof path components on the --- and +++ lines, and the nonsensical\nfilename on the +++ line. 'patch -p0' can still apply these patches,\nas it takes the filename to be modified from the --- line. Alas, 'git\napply' can't, because it takes the filename from the +++ line, and\nthen complains about the nonexisting file.\n\nPass the '--patch .' options to Coccinelle via the SPATCH_FLAGS 'make'\nvariable, as it seems to make it generate proper context diff patches,\nwith the header starting with a 'diff ...' line and containing sane\nfilenames. The resulting 'contrib/coccinelle/*.cocci.patch' files\nthen can be applied both with 'git apply' and 'patch' (even without\n'-p0').\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 73e2d16926..72ea29df4e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -564,7 +564,7 @@ SPATCH = spatch\n export TCL_PATH TCLTK_PATH\n \n SPARSE_FLAGS =\n-SPATCH_FLAGS = --all-includes\n+SPATCH_FLAGS = --all-includes --patch .\n \n \n \n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353352","messageId":"20180723135100.24288-6-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PATCH 5/5] coccinelle: extract dedicated make target to clean Coccinelle's results","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:51:00Z","receivedAt":"2018-07-23T13:51:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Sometimes I want to remove only Coccinelle's results, but keep all\nother build artifacts left after my usual 'make all man' build. This\nnew 'cocciclean' make target will allow just that.\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 72ea29df4e..aee629cbaf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2903,7 +2903,10 @@ profile-clean:\n \t$(RM) $(addsuffix *.gcda,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))\n \t$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))\n \n-clean: profile-clean coverage-clean\n+cocciclean:\n+\t$(RM) contrib/coccinelle/*.cocci.patch*\n+\n+clean: profile-clean coverage-clean cocciclean\n \t$(RM) *.res\n \t$(RM) $(OBJECTS)\n \t$(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)\n@@ -2915,7 +2918,6 @@ clean: profile-clean coverage-clean\n \t$(RM) -r $(GIT_TARNAME) .doc-tmp-dir\n \t$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz\n \t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n-\t$(RM) contrib/coccinelle/*.cocci.patch*\n \t$(MAKE) -C Documentation/ clean\n ifndef NO_PERL\n \t$(MAKE) -C gitweb clean\n@@ -2931,7 +2933,7 @@ endif\n \t$(RM) GIT-USER-AGENT GIT-PREFIX\n \t$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS\n \n-.PHONY: all install profile-clean clean strip\n+.PHONY: all install profile-clean cocciclean clean strip\n .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell\n .PHONY: FORCE cscope\n \n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353364","messageId":"9f560db6-b50a-e2f3-7215-d046c60e255a@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-5-szeder.dev@gmail.com","subject":"Re: [PATCH 4/5] coccinelle: put sane filenames into output patches","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T14:34:21Z","receivedAt":"2018-07-23T14:34:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:50 AM, SZEDER Gábor wrote:\n> Coccinelle outputs its suggested transformations as patches, whose\n> header looks something like this:\n>\n> --- commit.c\n> +++ /tmp/cocci-output-19250-7ae78a-commit.c\n>\n> Note the lack of 'diff --opts ' line, the differing number\n> of path components on the --- and +++ lines, and the nonsensical\n> filename on the +++ line. 'patch -p0' can still apply these patches,\n> as it takes the filename to be modified from the --- line. Alas, 'git\n> apply' can't, because it takes the filename from the +++ line, and\n> then complains about the nonexisting file.\n>\n> Pass the '--patch .' options to Coccinelle via the SPATCH_FLAGS 'make'\n> variable, as it seems to make it generate proper context diff patches,\n> with the header starting with a 'diff ...' line and containing sane\n> filenames. The resulting 'contrib/coccinelle/*.cocci.patch' files\n> then can be applied both with 'git apply' and 'patch' (even without\n> '-p0').\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 73e2d16926..72ea29df4e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -564,7 +564,7 @@ SPATCH = spatch\n> export TCL_PATH TCLTK_PATH\n> \n> SPARSE_FLAGS =\n> -SPATCH_FLAGS = --all-includes\n> +SPATCH_FLAGS = --all-includes --patch .\n\nThank you for this! These garbage filenames cause significant pain when \ntrying to apply the suggested patch.\n\n"},{"id":"353365","messageId":"0023a8c0-9239-d8d7-4187-922f9305449b@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/5] coccinelle: mark the 'coccicheck' make target as .PHONY","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T14:36:55Z","receivedAt":"2018-07-23T14:37:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:50 AM, SZEDER Gábor wrote:\n> The 'coccicheck' target doesn't create a file with the same name, so\n> mark it as .PHONY.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Makefile | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index e4b503d259..27bfc196dd 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2685,6 +2685,8 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> \tfi\n> coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))\n> \n> +.PHONY: coccicheck\n> +\n> ### Installation rules\n> \n> ifneq ($(filter /%,$(firstword $(template_dir))),)\n\nI did not know about phony targets [1] so thanks for teaching me \nsomething today.\n\n[1] https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html\n\n"},{"id":"353366","messageId":"fbf95215-a174-6917-256c-3ed71f960a6a@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/5] Misc Coccinelle-related improvements","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T14:38:10Z","receivedAt":"2018-07-23T14:38:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:50 AM, SZEDER Gábor wrote:\n> Just a couple of minor Coccinelle-related improvements:\n>\n> - The first two patches are small cleanups.\n>\n> - The last three could make life perhaps just a tad bit easier for\n> devs running 'make coccicheck'.\n\nI appreciate your focus on making 'make coccicheck' easier to use!\n\nReviewed-by: Derrick Stolee \n\n"},{"id":"353367","messageId":"CACsJy8Av5vBY_fGGyHXdqezZJpdE-_-roro96+WAc2xiSbOYPQ@mail.gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/5] Misc Coccinelle-related improvements","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-23T15:29:04Z","receivedAt":"2018-07-23T15:29:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 23, 2018 at 3:51 PM SZEDER Gábor wrote:\n>\n> Just a couple of minor Coccinelle-related improvements:\n>\n> - The first two patches are small cleanups.\n>\n> - The last three could make life perhaps just a tad bit easier for\n> devs running 'make coccicheck'.\n\nI'm not a heavy cocci user and probably won't spot any corner case\nproblems. With that in mind, I've read this though and the series\nlooks good to me.\n-- \nDuy\n"},{"id":"353374","messageId":"b405bbd5-9af5-6884-6582-9aa7f7db0878@web.de","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/5] Misc Coccinelle-related improvements","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-07-23T16:30:02Z","receivedAt":"2018-07-23T16:30:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.07.2018 um 15:50 schrieb SZEDER Gábor:\n> Just a couple of minor Coccinelle-related improvements:\n> \n> - The first two patches are small cleanups.\n> \n> - The last three could make life perhaps just a tad bit easier for\n> devs running 'make coccicheck'.\n> \n> \n> SZEDER Gábor (5):\n> coccinelle: mark the 'coccicheck' make target as .PHONY\n> coccinelle: use $(addsuffix) in 'coccicheck' make target\n> coccinelle: exclude sha1dc source files from static analysis\n> coccinelle: put sane filenames into output patches\n> coccinelle: extract dedicated make target to clean Coccinelle's\n> results\n\nThese patches look good to me. Thanks a lot!\n\nRené\n"},{"id":"353392","messageId":"CAPig+cSOZd+t17j7FSCYAydS34ZtfcRFZyE6E9fz=u7xB-01Mg@mail.gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/5] coccinelle: exclude sha1dc source files from static analysis","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-23T18:27:53Z","receivedAt":"2018-07-23T18:28:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 23, 2018 at 9:51 AM SZEDER Gábor wrote:\n> sha1dc is an external library, that we carry in-tree for convenience\n> or grab as a submodule, so there is no use in applying our semantic\n> patches to its source files.\n>\n> Therefore, exclude sha1dc's source files from Coccinelle's static\n> analysis.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> diff --git a/Makefile b/Makefile\n> @@ -2666,10 +2666,16 @@ check: command-list.h\n> +ifdef DC_SHA1_SUBMODULE\n> +COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> +else\n> +COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> +endif\n\nCan't you just filter out both of these unconditionally without\nworrying about DC_SHA1_SUBMODULE?\n"},{"id":"353395","messageId":"CAM0VKjkJoqRFmXdnuujSaiZ=hvz6MeAmgoUQNAkZ+82ZrKtotw@mail.gmail.com","threadId":"48941","inReplyTo":"CAPig+cSOZd+t17j7FSCYAydS34ZtfcRFZyE6E9fz=u7xB-01Mg@mail.gmail.com","subject":"Re: [PATCH 3/5] coccinelle: exclude sha1dc source files from static analysis","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T18:43:59Z","receivedAt":"2018-07-23T18:44:14Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 23, 2018 at 8:28 PM Eric Sunshine wrote:\n>\n> On Mon, Jul 23, 2018 at 9:51 AM SZEDER Gábor wrote:\n> > sha1dc is an external library, that we carry in-tree for convenience\n> > or grab as a submodule, so there is no use in applying our semantic\n> > patches to its source files.\n> >\n> > Therefore, exclude sha1dc's source files from Coccinelle's static\n> > analysis.\n> >\n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > diff --git a/Makefile b/Makefile\n> > @@ -2666,10 +2666,16 @@ check: command-list.h\n> > +ifdef DC_SHA1_SUBMODULE\n> > +COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> > +else\n> > +COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> > +endif\n>\n> Can't you just filter out both of these unconditionally without\n> worrying about DC_SHA1_SUBMODULE?\n\nI'm not sure what you mean by that. Like this perhaps?\n\n COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(filter-out\nsha1dc/%,$(C_SOURCES)))\n\nWhile it's only a single line, I don't think it's any easier on the\neyes.\n"},{"id":"353400","messageId":"CAPig+cQM-hdXXe6uZ768HbJ5T5QqJNKo9kuz6F=i7D+9Q-M90Q@mail.gmail.com","threadId":"48941","inReplyTo":"CAM0VKjkJoqRFmXdnuujSaiZ=hvz6MeAmgoUQNAkZ+82ZrKtotw@mail.gmail.com","subject":"Re: [PATCH 3/5] coccinelle: exclude sha1dc source files from static analysis","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-23T18:57:37Z","receivedAt":"2018-07-23T18:57:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 23, 2018 at 2:44 PM SZEDER Gábor wrote:\n> On Mon, Jul 23, 2018 at 8:28 PM Eric Sunshine wrote:\n> > On Mon, Jul 23, 2018 at 9:51 AM SZEDER Gábor wrote:\n> > > +ifdef DC_SHA1_SUBMODULE\n> > > +COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> > > +else\n> > > +COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> > > +endif\n> >\n> > Can't you just filter out both of these unconditionally without\n> > worrying about DC_SHA1_SUBMODULE?\n>\n> I'm not sure what you mean by that. Like this perhaps?\n>\n> COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(filter-out\n> sha1dc/%,$(C_SOURCES)))\n>\n> While it's only a single line, I don't think it's any easier on the\n> eyes.\n\nI wasn't worried about readability or one or two lines (indeed, you\ncould still do the filtering over two statements).\n\nWhat I meant was that sha1dc/ contains files whether DC_SHA1_SUBMODULE\nis defined or not. If the idea of this change is that there's no point\nin having Coccinelle check those foreign, imported files (and waste\ntime in the process), then I was thinking that you'd want to omit\nsha1dc/* regardless of whether DC_SHA1_SUBMODULE is defined.\n\nLooking more closely at the Makefile, however, I see that C_SOURCES\nholds only one or the other of sha1dc/* or\nsha1collisiondetection/lib/*, so my concern is unfounded, which\nexplains why my question confused you.\n"},{"id":"353406","messageId":"xmqqefft68wv.fsf@gitster-ct.c.googlers.com","threadId":"48941","inReplyTo":"20180723135100.24288-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/5] coccinelle: mark the 'coccicheck' make target as .PHONY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-23T19:37:04Z","receivedAt":"2018-07-23T19:37:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The 'coccicheck' target doesn't create a file with the same name, so\n> mark it as .PHONY.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n\nGood. It is customary to do so immediately before the target, not\nafter a blank line, though.\n\n> Makefile | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index e4b503d259..27bfc196dd 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2685,6 +2685,8 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> \tfi\n> coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))\n> \n> +.PHONY: coccicheck\n> +\n> ### Installation rules\n> \n> ifneq ($(filter /%,$(firstword $(template_dir))),)\n"},{"id":"353407","messageId":"xmqqa7qh68vr.fsf@gitster-ct.c.googlers.com","threadId":"48941","inReplyTo":"20180723135100.24288-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/5] coccinelle: use $(addsuffix) in 'coccicheck' make target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-23T19:37:44Z","receivedAt":"2018-07-23T19:37:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The dependencies of the 'coccicheck' make target are listed with the\n> help of the $(patsubst) make function, which in this case doesn't do\n> any pattern substitution, but only adds the '.patch' suffix.\n>\n> Use the shorter and more idiomatic $(addsuffix) make function instead.\n\nMakes sense.\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 27bfc196dd..8f509576e9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2683,7 +2683,7 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> \tthen \\\n> \t\techo ' ' SPATCH result: $@; \\\n> \tfi\n> -coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))\n> +coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n> \n> .PHONY: coccicheck\n"},{"id":"353408","messageId":"xmqq601568re.fsf@gitster-ct.c.googlers.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/5] Misc Coccinelle-related improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-23T19:40:21Z","receivedAt":"2018-07-23T19:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Just a couple of minor Coccinelle-related improvements:\n>\n> - The first two patches are small cleanups.\n>\n> - The last three could make life perhaps just a tad bit easier for\n> devs running 'make coccicheck'.\n\nThanks. All 5 patches make sense.\n\nQueued.\n"},{"id":"354236","messageId":"20180802115522.16107-1-szeder.dev@gmail.com","threadId":"48941","inReplyTo":"20180723135100.24288-1-szeder.dev@gmail.com","subject":"[PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-02T11:55:22Z","receivedAt":"2018-08-02T11:55:44Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nSo, I have this PoC patch below on top of this patch series for some\nmonths now; actually this series was all fallout from working on this\npatch. It makes 'make -j coccicheck' much faster (speedup of over\n99% :) in some scenarios that, in my experience, occur rather\nfrequently when working on Coccinelle semantic patches (as opposed to\noccasionally running 'make coccicheck' to see whether there are any\nregressions).\n\nHowever, it's not ready for inclusion as it is, because,\nunfortunately, the improvements are not without some disadvantages, as\nexplained in the second half of the commit message below.\n\nAnyway, I'm sending this out, because I don't see how I could make it\nany better, but other contributors working on semantic patches might\nstill find it useful to keep this in their own fork, even in its\ncurrent form. And perhaps someone will come up with a brilliant idea\nto address those disadvantages...\n\nAlso available at:\n\n https://github.com/szeder/git make-coccicheck-finegrained\n\n\n -- >8 --\n\nSubject: [PATCH] [PoC] coccinelle: make Coccinelle-related make targets more fine-grained\n\nWhen running 'make coccicheck', each semantic patch is applied to all\nsource files in a separate shell for loop. This approach has the\nfollowing disadvantages:\n\n 1. Even if you only modified a single C source file, that shell loop\n will still iterate over all source files and apply the semantic\n patches to all of them, wasting a lot of time.\n\n 2. If you apply only a single semantic patch (either implicitly,\n after you modified only one of them (and the results of the\n previous 'make coccicheck' are still there), or explicitly, by\n running e.g. 'make contrib/coccinelle/array.cocci.patch'), then\n you won't be able to exploit multiple CPU cores to speed up the\n operation, because that shell loop will iterate over all source\n files sequentially.\n\n 3. 'make coccicheck' can only use as many parallel jobs as the\n number of semantic patches, so even if you have more available\n CPU cores than that, you can't exploit them all to speed up this\n operation.\n\n 4. During 'make coccicheck' there is only a single\n\n SPATCH contrib/coccinelle/.cocci\n\n line (or as many lines as the number of parallel jobs) of output\n when starting to apply each semantic patch, and then comes a long\n silence without any indication of progress, because applying some\n of our semantic patches to all source files can take over a\n minute. This can trick developers new to semantic patches into\n thinking that something went wrong, hung, ended up in an endless\n loop, etc. It certainly confused my the first time.\n\nLet's add a bit of Makefile metaprogramming to generate finer-grained\nmake targets applying one semantic patch to only a single source file,\nand specify these as dependencies of the targets applying one semantic\npatch to all source files. This way that shell loop can be avoided,\nsemantic patches will only be applied to changed source files, and the\nsame semantic patch can be applied in parallel to multiple source\nfiles. The only remaining sequential part is aggregating the\nsuggested transformations from the individual targets into a single\npatch file, which is comparatively cheap (especially since ideally\nthere aren't any suggestions).\n\nThis change brings spectacular speedup in the scenario described in\npoint (1) above. When the results of a previous 'make coccicheck' are\nthere, the time needed to run\n\n touch apply.c ; time make -j4 coccicheck\n\nwent from 3m42s to 1.848s, which is just over 99% speedup, yay!, and\n'apply.c' is the second longest source file in our codebase. In the\nscenario in point (2), running\n\n touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck\n\nwent from 56.364s to 35.883s, which is ~36% speedup.\n\nAll the above timings are best-of-five on a machine with 2 physical\nand 2 logical cores. I don't have the hardware to bring any numbers\nfor point (3). The time needed to run 'make -j4 coccicheck' in a\nclean state didn't change, it's ~3m42s both with and without this\npatch.\n\nUnfortunately, this new approach has some disadvantages compared to\nthe current situation:\n\n - [RFC]\n With this patch 'make coccicheck's output will look like this\n ('make' apparently doesn't iterate over semantic patches in\n lexicographical order):\n\n SPATCH commit.cocci abspath.c\n SPATCH commit.cocci advice.c\n <... lots of lines ...>\n SPATCH array.cocci http-walker.c\n SPATCH array.cocci remote-curl.c\n\n which means that the number of lines in the output grows from\n \"Nr-of-semantic-patches\" to \"Nr-of-semantic-patches *\n Nr-of-source-files\".\n\n Now, while this certainly addresses point (4) above, it can be\n considered too much, and I'm not sure that (4) is that big an\n issue to justify this much output.\n\n OTOH, I couldn't yet figure out a way to print a single 'SPATCH\n contrib/coccinelle/.cocci' line at the beginning of\n applying that semantic patch without triggering unnecessary work,\n effectively killing most of the runtime benefits. Or to somehow\n iterate over source files in the outer loop and over semantic\n patches in the inner loop, so we could have one output line per\n file.\n\n - [RFC]\n The overhead of applying a semantic patch to all source files\n became larger. 'make coccicheck' currently runs only one shell\n process and creates two output files for each semantic patch.\n With this patch it will run approx. \"Nr-of-semantic-patches *\n Nr-of-source-files\" shell processes and create twice as many\n output files.\n\n This overhead can be quantified by measuring the runtime of:\n\n make -j4 SPATCH=true coccicheck\n\n and this patch increases it from 0.142s to 1.479s. While this is\n indeed an order of magnitude increase, it's still negligible\n compared to the \"real\" runtime of 3m42s.\n\n So the increased overhead doesn't seem to matter on Linux, but I\n would expect that it's worse on OSX and Windows; though I'm not\n sure whether Coccinelle (with all its OCaml dependencies) works on\n those platforms in the first place.\n\n - [RFC]\n This approach uses $(eval), which we haven't used in any of our\n Makefiles yet. I wonder whether it's portable enough. And it's\n ugly and complicated.\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 52 ++++++++++++++++++++++++-----------\n contrib/coccinelle/.gitignore | 3 +-\n 2 files changed, 38 insertions(+), 17 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex d616c04125..f516dd93d1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2674,25 +2674,44 @@ COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n else\n COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n endif\n+COCCI_SEM_PATCHES = $(wildcard contrib/coccinelle/*.cocci)\n \n-%.cocci.patch: %.cocci $(COCCI_SOURCES)\n-\t@echo ' ' SPATCH $<; \\\n-\tret=0; \\\n-\tfor f in $(COCCI_SOURCES); do \\\n-\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n-\t\t\t{ ret=$$?; break; }; \\\n-\tdone >$@+ 2>$@.log; \\\n-\tif test $$ret != 0; \\\n+define cocci_template\n+$(cocci_sem_patch)_dirs := $$(addprefix $(cocci_sem_patch).patches/,$$(sort $$(dir $$(COCCI_SOURCES))))\n+\n+$$($(cocci_sem_patch)_dirs):\n+\t@mkdir -p $$@\n+\n+# e.g. 'contrib/coccinelle/strbuf.cocci.patches/builtin/commit.c.patch'\n+# Applies one semantic patch to _one_ source file.\n+$(cocci_sem_patch).patches/%.patch: % $(cocci_sem_patch)\n+\t@printf ' SPATCH %-25s %s\\n' $$(notdir $(cocci_sem_patch)) $$<; \\\n+\tif ! $$(SPATCH) --sp-file $(cocci_sem_patch) $$< $$(SPATCH_FLAGS) >$$@ 2>$$@.log; \\\n \tthen \\\n-\t\tcat $@.log; \\\n+\t\trm -f $$@; \\\n+\t\tcat $$@.log; \\\n \t\texit 1; \\\n-\tfi; \\\n-\tmv $@+ $@; \\\n-\tif test -s $@; \\\n-\tthen \\\n-\t\techo ' ' SPATCH result: $@; \\\n \tfi\n-coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n+\n+# e.g. 'contrib/coccinelle/strbuf.cocci.patch'\n+# Applies one semantic patch to _all_ source files.\n+$(cocci_sem_patch).patch: $(cocci_sem_patch) $$($(cocci_sem_patch)_dirs) $$(patsubst %,$(cocci_sem_patch).patches/%.patch,$(COCCI_SOURCES))\n+\t@>$$@+; \\\n+\tfor f in $$(filter %.patch,$$^); do \\\n+\t\tif test -s $$$$f; \\\n+\t\tthen \\\n+\t\t\tcat $$$$f >>$$@+; \\\n+\t\tfi \\\n+\tdone; \\\n+\tmv $$@+ $$@; \\\n+\tif test -s $$@; then \\\n+\t\techo ' ' SPATCH result: $$@; \\\n+\tfi\n+endef\n+\n+$(foreach cocci_sem_patch,$(COCCI_SEM_PATCHES),$(eval $(cocci_template)))\n+\n+coccicheck: $(addsuffix .patch,$(COCCI_SEM_PATCHES))\n \n .PHONY: coccicheck\n \n@@ -2907,7 +2926,8 @@ profile-clean:\n \t$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))\n \n cocciclean:\n-\t$(RM) contrib/coccinelle/*.cocci.patch*\n+\t$(RM) contrib/coccinelle/*.cocci.patch\n+\t$(RM) -r contrib/coccinelle/*.cocci.patches/\n \n clean: profile-clean coverage-clean cocciclean\n \t$(RM) *.res\ndiff --git a/contrib/coccinelle/.gitignore b/contrib/coccinelle/.gitignore\nindex d3f29646dc..7ae6ffa983 100644\n--- a/contrib/coccinelle/.gitignore\n+++ b/contrib/coccinelle/.gitignore\n@@ -1 +1,2 @@\n-*.patch*\n+*.cocci.patch\n+*.cocci.patches/\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"354244","messageId":"edd896c5-9d73-7771-984e-f7443b74d01f@web.de","threadId":"48941","inReplyTo":"20180802115522.16107-1-szeder.dev@gmail.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-02T13:24:32Z","receivedAt":"2018-08-02T13:24:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.08.2018 um 13:55 schrieb SZEDER Gábor:\n> Let's add a bit of Makefile metaprogramming to generate finer-grained\n> make targets applying one semantic patch to only a single source file,\n> and specify these as dependencies of the targets applying one semantic\n> patch to all source files. This way that shell loop can be avoided,\n> semantic patches will only be applied to changed source files, and the\n> same semantic patch can be applied in parallel to multiple source\n> files. The only remaining sequential part is aggregating the\n> suggested transformations from the individual targets into a single\n> patch file, which is comparatively cheap (especially since ideally\n> there aren't any suggestions).\n> \n> This change brings spectacular speedup in the scenario described in\n> point (1) above. When the results of a previous 'make coccicheck' are\n> there, the time needed to run\n> \n> touch apply.c ; time make -j4 coccicheck\n> \n> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and\n> 'apply.c' is the second longest source file in our codebase. In the\n> scenario in point (2), running\n> \n> touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck\n> \n> went from 56.364s to 35.883s, which is ~36% speedup.\n\nAwesome!\n\n> Unfortunately, this new approach has some disadvantages compared to\n> the current situation:\n> \n> - [RFC]\n> With this patch 'make coccicheck's output will look like this\n> ('make' apparently doesn't iterate over semantic patches in\n> lexicographical order):\n> \n> SPATCH commit.cocci abspath.c\n> SPATCH commit.cocci advice.c\n> <... lots of lines ...>\n> SPATCH array.cocci http-walker.c\n> SPATCH array.cocci remote-curl.c\n> \n> which means that the number of lines in the output grows from\n> \"Nr-of-semantic-patches\" to \"Nr-of-semantic-patches *\n> Nr-of-source-files\".\n\nThe output is mostly interesting to see if something is happening, to\nsee if some semantic patch generated a non-empty diff and to get\nerror details. We have ca. 400 .c files and 8 .cocci files -- that\nmeans the lines would fill up my scroll-back buffer. Hmm.\n\nWould it be possible to have a phony target (or directory) per source\nfile that just prints a line and depends on the individual file+cocci\ntargets? (I don't know much make, you probably thought about it\nalready.)\n\nOr to select one random .cocci file (let's say the first one) and\nonly print SPATCH lines for this one (without mentioning its name)?\n(A dirty hack..)\n\n> ---\n> Makefile | 52 ++++++++++++++++++++++++-----------\n> contrib/coccinelle/.gitignore | 3 +-\n> 2 files changed, 38 insertions(+), 17 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index d616c04125..f516dd93d1 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2674,25 +2674,44 @@ COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> else\n> COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> endif\n> +COCCI_SEM_PATCHES = $(wildcard contrib/coccinelle/*.cocci)\n> \n> -%.cocci.patch: %.cocci $(COCCI_SOURCES)\n> -\t@echo ' ' SPATCH $<; \\\n> -\tret=0; \\\n> -\tfor f in $(COCCI_SOURCES); do \\\n> -\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n> -\t\t\t{ ret=$$?; break; }; \\\n> -\tdone >$@+ 2>$@.log; \\\n> -\tif test $$ret != 0; \\\n> +define cocci_template\n> +$(cocci_sem_patch)_dirs := $$(addprefix $(cocci_sem_patch).patches/,$$(sort $$(dir $$(COCCI_SOURCES))))\n> +\n> +$$($(cocci_sem_patch)_dirs):\n> +\t@mkdir -p $$@\n> +\n> +# e.g. 'contrib/coccinelle/strbuf.cocci.patches/builtin/commit.c.patch'\n> +# Applies one semantic patch to _one_ source file.\n> +$(cocci_sem_patch).patches/%.patch: % $(cocci_sem_patch)\n> +\t@printf ' SPATCH %-25s %s\\n' $$(notdir $(cocci_sem_patch)) $$<; \\\n> +\tif ! $$(SPATCH) --sp-file $(cocci_sem_patch) $$< $$(SPATCH_FLAGS) >$$@ 2>$$@.log; \\\n> \tthen \\\n> -\t\tcat $@.log; \\\n> +\t\trm -f $$@; \\\n> +\t\tcat $$@.log; \\\n> \t\texit 1; \\\n> -\tfi; \\\n> -\tmv $@+ $@; \\\n> -\tif test -s $@; \\\n> -\tthen \\\n> -\t\techo ' ' SPATCH result: $@; \\\n> \tfi\n> -coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n> +\n> +# e.g. 'contrib/coccinelle/strbuf.cocci.patch'\n> +# Applies one semantic patch to _all_ source files.\n> +$(cocci_sem_patch).patch: $(cocci_sem_patch) $$($(cocci_sem_patch)_dirs) $$(patsubst %,$(cocci_sem_patch).patches/%.patch,$(COCCI_SOURCES))\n\nAre the dependencies correct (before and after the patch)? E.g. are all\nsemantic patches reapplied after cache.h was changed? I think we ignore\nheader files currently, and that becomes more of a problem if the check\nbecomes more fine-grained.\n\nRené\n"},{"id":"354285","messageId":"20180802180155.GD15984@sigill.intra.peff.net","threadId":"48941","inReplyTo":"20180802115522.16107-1-szeder.dev@gmail.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T18:01:55Z","receivedAt":"2018-08-02T18:01:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:\n\n> Let's add a bit of Makefile metaprogramming to generate finer-grained\n> make targets applying one semantic patch to only a single source file,\n> and specify these as dependencies of the targets applying one semantic\n> patch to all source files. This way that shell loop can be avoided,\n> semantic patches will only be applied to changed source files, and the\n> same semantic patch can be applied in parallel to multiple source\n> files. The only remaining sequential part is aggregating the\n> suggested transformations from the individual targets into a single\n> patch file, which is comparatively cheap (especially since ideally\n> there aren't any suggestions).\n> \n> This change brings spectacular speedup in the scenario described in\n> point (1) above. When the results of a previous 'make coccicheck' are\n> there, the time needed to run\n> \n> touch apply.c ; time make -j4 coccicheck\n> \n> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and\n> 'apply.c' is the second longest source file in our codebase. In the\n> scenario in point (2), running\n> \n> touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck\n> \n> went from 56.364s to 35.883s, which is ~36% speedup.\n\nI really like this direction. The slowness of coccicheck is one of the\nthings that has prevented me from running it more frequently. And I'm a\nbig fan of breaking steps down as much as possible into make targets. It\nlets make do its job (avoiding repeated work and parallelizing).\n\n> All the above timings are best-of-five on a machine with 2 physical\n> and 2 logical cores. I don't have the hardware to bring any numbers\n> for point (3). The time needed to run 'make -j4 coccicheck' in a\n> clean state didn't change, it's ~3m42s both with and without this\n> patch.\n\nOn a 40-core (20+20) machine, doing \"make -j40 coccicheck\" from scratch\nwent from:\n\n real\t1m25.520s\n user\t5m41.492s\n sys\t0m26.776s\n\nto:\n\n real\t0m24.300s\n user\t14m35.084s\n sys\t0m50.108s\n\nI was surprised by the jump in CPU times. Doing \"make -j1 coccicheck\"\nwith your patch results in:\n\n real\t5m34.887s\n user\t5m5.620s\n sys\t0m19.908s\n\nso it's really the parallelizing that seems to be to blame (possibly\nbecause this CPU boosts from 2.3Ghz to 3.0Ghz, and we're only using 8\nthreads in the first example).\n\n> - [RFC]\n> With this patch 'make coccicheck's output will look like this\n> ('make' apparently doesn't iterate over semantic patches in\n> lexicographical order):\n> \n> SPATCH commit.cocci abspath.c\n> SPATCH commit.cocci advice.c\n> <... lots of lines ...>\n> SPATCH array.cocci http-walker.c\n> SPATCH array.cocci remote-curl.c\n> \n> which means that the number of lines in the output grows from\n> \"Nr-of-semantic-patches\" to \"Nr-of-semantic-patches *\n> Nr-of-source-files\".\n\nIMHO this is not really that big a deal. We are doing useful work for\neach line, so to me it's just more eye candy (and it's really satisfying\nto watch it zip by on the 40-core machine ;) ).\n\nWhat if we inverted the current loop? That is, right now we iterate over\nthe cocci patches at the Makefile level, and then for each target we\niterate over the giant list of source files. Instead, we could teach the\nMakefile to iterate over the source files, with a target for each, and\nthen hit each cocci patch inside there.\n\nThat would give roughly the same output as a normal build. But moreover,\nI wonder if we could make things faster by actually combining the cocci\nfiles into a single set of rules to be applied to each source file. That\nwould mean invoking spatch 1/8 as much. It does give fewer opportunities\nfor \"make\" to reuse work, but that only matters when the cocci files\nchange (which is much less frequent than source files changing).\n\nDoing:\n\n cat contrib/coccinelle/*.cocci >mega.cocci\n make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci\n\ngives me:\n\n real\t0m17.573s\n user\t10m56.724s\n sys\t0m11.548s\n\nAnd that should show an improvement on more normal-sized systems, too,\nbecause we really are eliminating some of the startup overhead.\n\nThe other obvious downside is that you don't get individual patches for\neach class of transformation. Do we care? Certainly for a routine \"make\ncoccicheck\" I primarily want to know:\n\n - is there something that needs fixing?\n\n - give me the patch for all fixes\n\nSo I wonder if we'd want to have that be the default, and then perhaps\noptionally give some targets to let people run single scripts (or not;\nthey could probably just run spatch themselves).\n\n> - [RFC]\n> The overhead of applying a semantic patch to all source files\n> became larger. 'make coccicheck' currently runs only one shell\n> process and creates two output files for each semantic patch.\n> With this patch it will run approx. \"Nr-of-semantic-patches *\n> Nr-of-source-files\" shell processes and create twice as many\n> output files.\n\nDoing the \"one big .cocci\" would help with this, too (and again puts us\nin the same ballpark as a compile).\n\n> - [RFC]\n> This approach uses $(eval), which we haven't used in any of our\n> Makefiles yet. I wonder whether it's portable enough. And it's\n> ugly and complicated.\n\nI looked into this a long time ago for another set of Makefile patches I\nwas considering. $(eval) was added to GNU make in 3.80, released in\n2002. Which is probably fine by now.\n\nIf it isn't, one more exotic option would be to push the coccicheck\nstuff into its own Makefile, and just run it via recursive make. Then\nanybody doing a vanilla build can do so even with an antique make, but\nyou could only \"make coccicheck\" with something from the last 16 years\n(but good luck getting ocaml running there).\n\nI suspect if we go with the one-spatch-per-source route, though, that we\ncould do this just with regular make rules.\n\n-Peff\n"},{"id":"354290","messageId":"20180802183145.GA23690@sigill.intra.peff.net","threadId":"48941","inReplyTo":"20180802180155.GD15984@sigill.intra.peff.net","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T18:31:45Z","receivedAt":"2018-08-02T18:31:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 02:01:55PM -0400, Jeff King wrote:\n\n> I suspect if we go with the one-spatch-per-source route, though, that we\n> could do this just with regular make rules.\n\nYeah, it's pretty straightforward:\n\ndiff --git a/Makefile b/Makefile\nindex d616c0412..86fdcf567 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2674,15 +2674,17 @@ COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n else\n COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n endif\n+COCCI_COMBINED = contrib/coccinelle/combined.cocci\n+COCCI_SEM_PATCHES = $(filter-out $(COCCI_COMBINED), $(wildcard contrib/coccinelle/*.cocci))\n \n-%.cocci.patch: %.cocci $(COCCI_SOURCES)\n+$(COCCI_COMBINED): $(COCCI_SEM_PATCHES)\n+\tcat $^ >$@+\n+\tmv $@+ $@\n+\n+$(COCCI_COMBINED).patches/%.patch: % $(COCCI_COMBINED)\n \t@echo ' ' SPATCH $<; \\\n-\tret=0; \\\n-\tfor f in $(COCCI_SOURCES); do \\\n-\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n-\t\t\t{ ret=$$?; break; }; \\\n-\tdone >$@+ 2>$@.log; \\\n-\tif test $$ret != 0; \\\n+\tmkdir -p $(dir $@) || exit 1; \\\n+\tif ! $(SPATCH) --sp-file $(COCCI_COMBINED) $< $(SPATCH_FLAGS) >$@+ 2>$@.log; \\\n \tthen \\\n \t\tcat $@.log; \\\n \t\texit 1; \\\n@@ -2692,7 +2694,8 @@ endif\n \tthen \\\n \t\techo ' ' SPATCH result: $@; \\\n \tfi\n-coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n+\n+coccicheck: $(patsubst %, $(COCCI_COMBINED).patches/%.patch, $(COCCI_SOURCES))\n \n .PHONY: coccicheck\n \n@@ -2907,7 +2910,7 @@ profile-clean:\n \t$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))\n \n cocciclean:\n-\t$(RM) contrib/coccinelle/*.cocci.patch*\n+\t$(RM) -r contrib/coccinelle/*.cocci.patches\n \n clean: profile-clean coverage-clean cocciclean\n \t$(RM) *.res\n\nI guess you could even replace \"COCCI_COMBINED\" with \"COCCI_PATCH\" in\nmost of the targets, and that would let people do individual:\n\n make COCCI_PATCH=contrib/coccinelle/foo.cocci coccicheck\n\nThe default would just be the concatenated version.\n\n-Peff\n"},{"id":"354313","messageId":"CAPig+cQSg=MBB7zp5bx5Z6tep89FaqNfTXV0tATRRG3WAPn2MQ@mail.gmail.com","threadId":"48941","inReplyTo":"20180802180155.GD15984@sigill.intra.peff.net","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T19:46:08Z","receivedAt":"2018-08-02T19:46:22Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 2:02 PM Jeff King wrote:\n> On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:\n> > This approach uses $(eval), which we haven't used in any of our\n> > Makefiles yet. I wonder whether it's portable enough. And it's\n> > ugly and complicated.\n>\n> I looked into this a long time ago for another set of Makefile patches I\n> was considering. $(eval) was added to GNU make in 3.80, released in\n> 2002. Which is probably fine by now.\n\nFor the record, MacOS developer tools are stuck at GNU make 3.81 (from 2006).\n"},{"id":"354331","messageId":"20180802212930.GB32538@sigill.intra.peff.net","threadId":"48941","inReplyTo":"CAPig+cQSg=MBB7zp5bx5Z6tep89FaqNfTXV0tATRRG3WAPn2MQ@mail.gmail.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T21:29:31Z","receivedAt":"2018-08-02T21:29:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 03:46:08PM -0400, Eric Sunshine wrote:\n\n> On Thu, Aug 2, 2018 at 2:02 PM Jeff King wrote:\n> > On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:\n> > > This approach uses $(eval), which we haven't used in any of our\n> > > Makefiles yet. I wonder whether it's portable enough. And it's\n> > > ugly and complicated.\n> >\n> > I looked into this a long time ago for another set of Makefile patches I\n> > was considering. $(eval) was added to GNU make in 3.80, released in\n> > 2002. Which is probably fine by now.\n> \n> For the record, MacOS developer tools are stuck at GNU make 3.81 (from 2006).\n\nThanks, that's interesting to know. Fortunately from my past research\nthat means that it should have both $(eval) and $(call).\n\nIf we follow my suggestions here, we don't need to care for now, but I\nwon't be surprised if it comes up again at some point.\n\n-Peff\n"},{"id":"354334","messageId":"87bmaktpbt.fsf@evledraar.gmail.com","threadId":"48941","inReplyTo":"20180802180155.GD15984@sigill.intra.peff.net","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-02T21:45:42Z","receivedAt":"2018-08-02T21:45:47Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 02 2018, Jeff King wrote:\n\n> On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:\n>\n>> Let's add a bit of Makefile metaprogramming to generate finer-grained\n>> make targets applying one semantic patch to only a single source file,\n>> and specify these as dependencies of the targets applying one semantic\n>> patch to all source files. This way that shell loop can be avoided,\n>> semantic patches will only be applied to changed source files, and the\n>> same semantic patch can be applied in parallel to multiple source\n>> files. The only remaining sequential part is aggregating the\n>> suggested transformations from the individual targets into a single\n>> patch file, which is comparatively cheap (especially since ideally\n>> there aren't any suggestions).\n>>\n>> This change brings spectacular speedup in the scenario described in\n>> point (1) above. When the results of a previous 'make coccicheck' are\n>> there, the time needed to run\n>>\n>> touch apply.c ; time make -j4 coccicheck\n>>\n>> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and\n>> 'apply.c' is the second longest source file in our codebase. In the\n>> scenario in point (2), running\n>>\n>> touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck\n>>\n>> went from 56.364s to 35.883s, which is ~36% speedup.\n>\n> I really like this direction. The slowness of coccicheck is one of the\n> things that has prevented me from running it more frequently. And I'm a\n> big fan of breaking steps down as much as possible into make targets. It\n> lets make do its job (avoiding repeated work and parallelizing).\n\nYeah, this is great. Also, CC-ing some of the recent contributors to\nlinux.git's coccinelle, perhaps they're interested / have comments.\n\n>> All the above timings are best-of-five on a machine with 2 physical\n>> and 2 logical cores. I don't have the hardware to bring any numbers\n>> for point (3). The time needed to run 'make -j4 coccicheck' in a\n>> clean state didn't change, it's ~3m42s both with and without this\n>> patch.\n>\n> On a 40-core (20+20) machine, doing \"make -j40 coccicheck\" from scratch\n> went from:\n>\n> real\t1m25.520s\n> user\t5m41.492s\n> sys\t0m26.776s\n>\n> to:\n>\n> real\t0m24.300s\n> user\t14m35.084s\n> sys\t0m50.108s\n>\n> I was surprised by the jump in CPU times. Doing \"make -j1 coccicheck\"\n> with your patch results in:\n>\n> real\t5m34.887s\n> user\t5m5.620s\n> sys\t0m19.908s\n>\n> so it's really the parallelizing that seems to be to blame (possibly\n> because this CPU boosts from 2.3Ghz to 3.0Ghz, and we're only using 8\n> threads in the first example).\n>\n>> - [RFC]\n>> With this patch 'make coccicheck's output will look like this\n>> ('make' apparently doesn't iterate over semantic patches in\n>> lexicographical order):\n>>\n>> SPATCH commit.cocci abspath.c\n>> SPATCH commit.cocci advice.c\n>> <... lots of lines ...>\n>> SPATCH array.cocci http-walker.c\n>> SPATCH array.cocci remote-curl.c\n>>\n>> which means that the number of lines in the output grows from\n>> \"Nr-of-semantic-patches\" to \"Nr-of-semantic-patches *\n>> Nr-of-source-files\".\n>\n> IMHO this is not really that big a deal. We are doing useful work for\n> each line, so to me it's just more eye candy (and it's really satisfying\n> to watch it zip by on the 40-core machine ;) ).\n\nFWIW on the 8-cpu box I usually test on this went from 2m30s to 1m50s,\nso not a huge improvement in time, but nice to have the per-file\nprogress.\n\n> What if we inverted the current loop? That is, right now we iterate over\n> the cocci patches at the Makefile level, and then for each target we\n> iterate over the giant list of source files. Instead, we could teach the\n> Makefile to iterate over the source files, with a target for each, and\n> then hit each cocci patch inside there.\n>\n> That would give roughly the same output as a normal build. But moreover,\n> I wonder if we could make things faster by actually combining the cocci\n> files into a single set of rules to be applied to each source file. That\n> would mean invoking spatch 1/8 as much. It does give fewer opportunities\n> for \"make\" to reuse work, but that only matters when the cocci files\n> change (which is much less frequent than source files changing).\n>\n> Doing:\n>\n> cat contrib/coccinelle/*.cocci >mega.cocci\n> make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci\n>\n> gives me:\n>\n> real\t0m17.573s\n> user\t10m56.724s\n> sys\t0m11.548s\n>\n> And that should show an improvement on more normal-sized systems, too,\n> because we really are eliminating some of the startup overhead.\n>\n> The other obvious downside is that you don't get individual patches for\n> each class of transformation. Do we care? Certainly for a routine \"make\n> coccicheck\" I primarily want to know:\n>\n> - is there something that needs fixing?\n>\n> - give me the patch for all fixes\n>\n> So I wonder if we'd want to have that be the default, and then perhaps\n> optionally give some targets to let people run single scripts (or not;\n> they could probably just run spatch themselves).\n>\n>> - [RFC]\n>> The overhead of applying a semantic patch to all source files\n>> became larger. 'make coccicheck' currently runs only one shell\n>> process and creates two output files for each semantic patch.\n>> With this patch it will run approx. \"Nr-of-semantic-patches *\n>> Nr-of-source-files\" shell processes and create twice as many\n>> output files.\n>\n> Doing the \"one big .cocci\" would help with this, too (and again puts us\n> in the same ballpark as a compile).\n>\n>> - [RFC]\n>> This approach uses $(eval), which we haven't used in any of our\n>> Makefiles yet. I wonder whether it's portable enough. And it's\n>> ugly and complicated.\n>\n> I looked into this a long time ago for another set of Makefile patches I\n> was considering. $(eval) was added to GNU make in 3.80, released in\n> 2002. Which is probably fine by now.\n>\n> If it isn't, one more exotic option would be to push the coccicheck\n> stuff into its own Makefile, and just run it via recursive make. Then\n> anybody doing a vanilla build can do so even with an antique make, but\n> you could only \"make coccicheck\" with something from the last 16 years\n> (but good luck getting ocaml running there).\n>\n> I suspect if we go with the one-spatch-per-source route, though, that we\n> could do this just with regular make rules.\n>\n> -Peff\n"},{"id":"354353","messageId":"20180803062144.GB237521@aiede.svl.corp.google.com","threadId":"48941","inReplyTo":"20180802183145.GA23690@sigill.intra.peff.net","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-03T06:21:44Z","receivedAt":"2018-08-03T06:21:49Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Thu, Aug 02, 2018 at 02:01:55PM -0400, Jeff King wrote:\n\n>> I suspect if we go with the one-spatch-per-source route, though, that we\n>> could do this just with regular make rules.\n>\n> Yeah, it's pretty straightforward:\n> \n> diff --git a/Makefile b/Makefile\n> index d616c0412..86fdcf567 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2674,15 +2674,17 @@ COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> else\n> COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> endif\n> +COCCI_COMBINED = contrib/coccinelle/combined.cocci\n\nI like this approach.\n\n[...]\n> I guess you could even replace \"COCCI_COMBINED\" with \"COCCI_PATCH\" in\n> most of the targets, and that would let people do individual:\n> \n> make COCCI_PATCH=contrib/coccinelle/foo.cocci coccicheck\n\nThe issue here is that the dependencies for foo.cocci become\nunreliable, so I'd rather have a separate target for that (e.g.\ndepending on FORCE) if we go that way.\n\nThanks,\nJonathan\n"},{"id":"354354","messageId":"alpine.DEB.2.20.1808030755350.2446@hadrien","threadId":"48941","inReplyTo":"87bmaktpbt.fsf@evledraar.gmail.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2018-08-03T06:22:24Z","receivedAt":"2018-08-03T06:22:29Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Thu, 2 Aug 2018, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Thu, Aug 02 2018, Jeff King wrote:\n>\n> > On Thu, Aug 02, 2018 at 01:55:22PM +0200, SZEDER Gábor wrote:\n> >\n> >> Let's add a bit of Makefile metaprogramming to generate finer-grained\n> >> make targets applying one semantic patch to only a single source file,\n> >> and specify these as dependencies of the targets applying one semantic\n> >> patch to all source files. This way that shell loop can be avoided,\n> >> semantic patches will only be applied to changed source files, and the\n> >> same semantic patch can be applied in parallel to multiple source\n> >> files.\n\nThis was already possible. Make coccicheck is not supposed to be used\nwith -j, but rather with J=n. That tells Coccinelle to parallelize the\ntreatment of the files internally. In this case, the semantic patch is\nonly parsed once, and then n worker processes are forked to treat the\ndifferent files.\n\n> The only remaining sequential part is aggregating the\n> >> suggested transformations from the individual targets into a single\n> >> patch file, which is comparatively cheap (especially since ideally\n> >> there aren't any suggestions).\n> >>\n> >> This change brings spectacular speedup in the scenario described in\n> >> point (1) above. When the results of a previous 'make coccicheck' are\n> >> there, the time needed to run\n> >>\n> >> touch apply.c ; time make -j4 coccicheck\n> >>\n> >> went from 3m42s to 1.848s, which is just over 99% speedup, yay!, and\n> >> 'apply.c' is the second longest source file in our codebase. In the\n> >> scenario in point (2), running\n> >>\n> >> touch contrib/coccinelle/array.cocci ; time make -j4 coccicheck\n> >>\n> >> went from 56.364s to 35.883s, which is ~36% speedup.\n> >\n> > I really like this direction. The slowness of coccicheck is one of the\n> > things that has prevented me from running it more frequently. And I'm a\n> > big fan of breaking steps down as much as possible into make targets. It\n> > lets make do its job (avoiding repeated work and parallelizing).\n>\n> Yeah, this is great. Also, CC-ing some of the recent contributors to\n> linux.git's coccinelle, perhaps they're interested / have comments.\n\nI have extended the list of recipients with Nicolas Palix and the\nCoccinelle mailing list. In particular, Nicolas should comment on any\nchanges.\n\n>\n> >> All the above timings are best-of-five on a machine with 2 physical\n> >> and 2 logical cores. I don't have the hardware to bring any numbers\n> >> for point (3). The time needed to run 'make -j4 coccicheck' in a\n> >> clean state didn't change, it's ~3m42s both with and without this\n> >> patch.\n> >\n> > On a 40-core (20+20) machine, doing \"make -j40 coccicheck\" from scratch\n> > went from:\n> >\n> > real\t1m25.520s\n> > user\t5m41.492s\n> > sys\t0m26.776s\n> >\n> > to:\n> >\n> > real\t0m24.300s\n> > user\t14m35.084s\n> > sys\t0m50.108s\n\nBy default, when the -j option is not used, make coccicheck causes\nCoccinelle to run on all of the cores of the machine. In my experience,\non a laptop (2 physical cores with hyperthreading), this is basically OK.\nAnd on a server, it is a disaster. On a machine with 80 physical cores\nand hyperthreading, make coccicheck will run each instance of Coccinelle\nsuch that it parallelizes on 160 cores. But in reality, there is not much\ndifference between 20 and 40 cores, and after 40 cores the performance\nstarts to degrade. So basically, using more than half of the physical\ncores on each socket is a loss.\n\n\n> >\n> > I was surprised by the jump in CPU times. Doing \"make -j1 coccicheck\"\n> > with your patch results in:\n> >\n> > real\t5m34.887s\n> > user\t5m5.620s\n> > sys\t0m19.908s\n> >\n> > so it's really the parallelizing that seems to be to blame (possibly\n> > because this CPU boosts from 2.3Ghz to 3.0Ghz, and we're only using 8\n> > threads in the first example).\n> >\n> >> - [RFC]\n> >> With this patch 'make coccicheck's output will look like this\n> >> ('make' apparently doesn't iterate over semantic patches in\n> >> lexicographical order):\n> >>\n> >> SPATCH commit.cocci abspath.c\n> >> SPATCH commit.cocci advice.c\n> >> <... lots of lines ...>\n> >> SPATCH array.cocci http-walker.c\n> >> SPATCH array.cocci remote-curl.c\n> >>\n> >> which means that the number of lines in the output grows from\n> >> \"Nr-of-semantic-patches\" to \"Nr-of-semantic-patches *\n> >> Nr-of-source-files\".\n> >\n> > IMHO this is not really that big a deal. We are doing useful work for\n> > each line, so to me it's just more eye candy (and it's really satisfying\n> > to watch it zip by on the 40-core machine ;) ).\n>\n> FWIW on the 8-cpu box I usually test on this went from 2m30s to 1m50s,\n> so not a huge improvement in time, but nice to have the per-file\n> progress.\n\nCoccinelle already does this when make coccicheck is used with J=n. It\nmakes a subdirectory for the semantic patch that it is currently working\non, and this subdirectory contains stderr.n files that contain HANDLING\nand the name of the file being treated. It also prints information about\nperformance bottlenecks. I think that make coccicheck turns off all this\nreporting by default, but you can get it back with SPFLAGS=\"--quiet\"\n\n> > What if we inverted the current loop? That is, right now we iterate over\n> > the cocci patches at the Makefile level, and then for each target we\n> > iterate over the giant list of source files. Instead, we could teach the\n> > Makefile to iterate over the source files, with a target for each, and\n> > then hit each cocci patch inside there.\n> >\n> > That would give roughly the same output as a normal build. But moreover,\n> > I wonder if we could make things faster by actually combining the cocci\n> > files into a single set of rules to be applied to each source file. That\n> > would mean invoking spatch 1/8 as much. It does give fewer opportunities\n> > for \"make\" to reuse work, but that only matters when the cocci files\n> > change (which is much less frequent than source files changing).\n> >\n> > Doing:\n> >\n> > cat contrib/coccinelle/*.cocci >mega.cocci\n> > make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci\n\nThere was already a COCCI=foo.cocci argument to focus on a single semantic\npatch.\n\nI'm surprised that the above cat command would work. Semantic patch rules\nhave names, and Coccinelle will not be happy isf two rules have the same\nname. Some may also have variables declared in initializers, although\nperhaps the ones in the kernel don't do this. Causing these variables to\nbe shared would not have a good effect.\n\nPutting everything together can also go counter to the optimizations that\nCoccinelle provides. You can speed up spatch a lot on rules that mention\nspecific functions by running id-utils on your code base in advance (sh\ncoccinelle/scripts/idutils_index.sh). The if you have one semantic patch\nthat only applies to files that contain foo and another that only applies\nto files that contain bar, then each will only be applied to its\nrespective files. If you don't have an id-utils index this optimization\nwill be done by Coccinelle by first scanning files for foo and bar, but\nthe index is obviously much faster. If your semantic patch can be\nrelavant to files that contain foo or bar, then the rules from the foo\nsemantic patch (which could be very slow) will also be uselessly applied\nto the bar files. Whether this is relevant in practice depends on the\nspecific semantic patches of course.\n\n> >\n> > gives me:\n> >\n> > real\t0m17.573s\n> > user\t10m56.724s\n> > sys\t0m11.548s\n> >\n> > And that should show an improvement on more normal-sized systems, too,\n> > because we really are eliminating some of the startup overhead.\n> >\n> > The other obvious downside is that you don't get individual patches for\n> > each class of transformation. Do we care? Certainly for a routine \"make\n> > coccicheck\" I primarily want to know:\n> >\n> > - is there something that needs fixing?\n> >\n> > - give me the patch for all fixes\n> >\n> > So I wonder if we'd want to have that be the default, and then perhaps\n> > optionally give some targets to let people run single scripts (or not;\n> > they could probably just run spatch themselves).\n> >\n> >> - [RFC]\n> >> The overhead of applying a semantic patch to all source files\n> >> became larger. 'make coccicheck' currently runs only one shell\n> >> process\n\nNot sure to understand this. To my understanding it runs as many shell\nprocesses as there are cores on the machine.\n\njulia\n\n> and creates two output files for each semantic patch.\n> >> With this patch it will run approx. \"Nr-of-semantic-patches *\n> >> Nr-of-source-files\" shell processes and create twice as many\n> >> output files.\n> >\n> > Doing the \"one big .cocci\" would help with this, too (and again puts us\n> > in the same ballpark as a compile).\n> >\n> >> - [RFC]\n> >> This approach uses $(eval), which we haven't used in any of our\n> >> Makefiles yet. I wonder whether it's portable enough. And it's\n> >> ugly and complicated.\n> >\n> > I looked into this a long time ago for another set of Makefile patches I\n> > was considering. $(eval) was added to GNU make in 3.80, released in\n> > 2002. Which is probably fine by now.\n> >\n> > If it isn't, one more exotic option would be to push the coccicheck\n> > stuff into its own Makefile, and just run it via recursive make. Then\n> > anybody doing a vanilla build can do so even with an antique make, but\n> > you could only \"make coccicheck\" with something from the last 16 years\n> > (but good luck getting ocaml running there).\n> >\n> > I suspect if we go with the one-spatch-per-source route, though, that we\n> > could do this just with regular make rules.\n> >\n> > -Peff\n>"},{"id":"354355","messageId":"alpine.DEB.2.20.1808030824330.2446@hadrien","threadId":"48941","inReplyTo":"87bmaktpbt.fsf@evledraar.gmail.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2018-08-03T06:25:48Z","receivedAt":"2018-08-03T06:26:09Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"By the way, you can see my performance numbers here:\n\nhttps://www.usenix.org/system/files/conference/atc18/atc18-lawall.pdf\n\nPage 8, figures 4 and 5.\n\njulia\n"},{"id":"354357","messageId":"20180803064419.GC237521@aiede.svl.corp.google.com","threadId":"48941","inReplyTo":"alpine.DEB.2.20.1808030755350.2446@hadrien","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-03T06:44:19Z","receivedAt":"2018-08-03T06:44:24Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJulia Lawall wrote:\n\n> This was already possible. Make coccicheck is not supposed to be used\n> with -j, but rather with J=n. That tells Coccinelle to parallelize the\n> treatment of the files internally. In this case, the semantic patch is\n> only parsed once, and then n worker processes are forked to treat the\n> different files.\n\nThanks for this hint.\n\nI wonder if we can make this happen automatically under suitable\nconditions. We can parse -j out of MAKEFLAGS and convert it into a\nJ= argument, but that only solves half the problem: the \"make\"\nparent process would think that the coccinelle run only deserves to\noccupy one job slot.\n\nTechnically we could do all this using a wrapper that pretends to be a\nsubmake \n(prefixing the command with '+', parsing jobserver options from the\nMAKEFLAGS envvar) but it gets ugly.\n\nIt's likely that the best we can do is just to advertise J more\nprominently.\n\n[...]\n>> On Thu, Aug 02 2018, Jeff King wrote:\n\n>>> cat contrib/coccinelle/*.cocci >mega.cocci\n>>> make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci\n>\n> There was already a COCCI=foo.cocci argument to focus on a single semantic\n> patch.\n>\n> I'm surprised that the above cat command would work. Semantic patch rules\n> have names, and Coccinelle will not be happy isf two rules have the same\n> name.\n\nYes, Git's semantic patches (in contrib/coccinelle) tend to be\nrelatively undemanding.\n\n> Some may also have variables declared in initializers, although\n> perhaps the ones in the kernel don't do this. Causing these variables to\n> be shared would not have a good effect.\n\n... oh! You're thinking of the Linux kernel.\n\nIt looks like Linux's scripts/coccicheck has a lot we can crib from.\nThat's where the J envvar and automatic parallelism you mentioned are\nimplemented, too.\n\nSo it sounds to me like at a minimum we should use all of that. ;-)\n\nThanks again for the pointers.\n\nSincerely,\nJonathan\n"},{"id":"354358","messageId":"alpine.DEB.2.20.1808030848160.2446@hadrien","threadId":"48941","inReplyTo":"20180803064419.GC237521@aiede.svl.corp.google.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Julia Lawall","fromEmail":"julia.lawall@lip6.fr","sentAt":"2018-08-03T06:52:38Z","receivedAt":"2018-08-03T06:52:43Z","isPatch":false,"sender":{"key":"julia.lawall@lip6.fr","avatar":null},"body":"\n\nOn Thu, 2 Aug 2018, Jonathan Nieder wrote:\n\n> Hi,\n>\n> Julia Lawall wrote:\n>\n> > This was already possible. Make coccicheck is not supposed to be used\n> > with -j, but rather with J=n. That tells Coccinelle to parallelize the\n> > treatment of the files internally. In this case, the semantic patch is\n> > only parsed once, and then n worker processes are forked to treat the\n> > different files.\n>\n> Thanks for this hint.\n>\n> I wonder if we can make this happen automatically under suitable\n> conditions. We can parse -j out of MAKEFLAGS and convert it into a\n> J= argument,\n\nIt does happen automatically, in the sense that the default with no -j or\nJ=option is to use all the cores on the machine.\n\nWith -j, it seems to have to default to one core internally:\n\nd7059ca0147adcd495f3c5b41f260e1ac55bb679\n\nAlso, if it didn't do that, it would end up with the product of the -j\noption and the number of cores on the machine, which would give very bad\nperformance.\n\n> but that only solves half the problem: the \"make\"\n> parent process would think that the coccinelle run only deserves to\n> occupy one job slot.\n\nYes, it seems to be intended to be run by itself.\n\n> Technically we could do all this using a wrapper that pretends to be a\n> submake \n> (prefixing the command with '+', parsing jobserver options from the\n> MAKEFLAGS envvar) but it gets ugly.\n>\n> It's likely that the best we can do is just to advertise J more\n> prominently.\n>\n> [...]\n> >> On Thu, Aug 02 2018, Jeff King wrote:\n>\n> >>> cat contrib/coccinelle/*.cocci >mega.cocci\n> >>> make -j40 coccicheck COCCI_SEM_PATCHES=mega.cocci\n> >\n> > There was already a COCCI=foo.cocci argument to focus on a single semantic\n> > patch.\n> >\n> > I'm surprised that the above cat command would work. Semantic patch rules\n> > have names, and Coccinelle will not be happy isf two rules have the same\n> > name.\n>\n> Yes, Git's semantic patches (in contrib/coccinelle) tend to be\n> relatively undemanding.\n>\n> > Some may also have variables declared in initializers, although\n> > perhaps the ones in the kernel don't do this. Causing these variables to\n> > be shared would not have a good effect.\n>\n> ... oh! You're thinking of the Linux kernel.\n>\n> It looks like Linux's scripts/coccicheck has a lot we can crib from.\n> That's where the J envvar and automatic parallelism you mentioned are\n> implemented, too.\n>\n> So it sounds to me like at a minimum we should use all of that. ;-)\n\nYes.\n\njulia\n"},{"id":"354383","messageId":"20180803130824.GC4671@sigill.intra.peff.net","threadId":"48941","inReplyTo":"20180803062144.GB237521@aiede.svl.corp.google.com","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T13:08:25Z","receivedAt":"2018-08-03T13:08:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2018 at 11:21:44PM -0700, Jonathan Nieder wrote:\n\n> > diff --git a/Makefile b/Makefile\n> > index d616c0412..86fdcf567 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2674,15 +2674,17 @@ COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> > else\n> > COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> > endif\n> > +COCCI_COMBINED = contrib/coccinelle/combined.cocci\n> \n> I like this approach.\n\nI was pretty pleased with myself, too, but I had a lingering doubt about\nwhether just cat-ing the files was legitimate. It sounds from the\nresponse elsewhere that it's not (but just happens to work now for out\nlimited case). But it also sounds like there may be even better options.\n\n> > I guess you could even replace \"COCCI_COMBINED\" with \"COCCI_PATCH\" in\n> > most of the targets, and that would let people do individual:\n> > \n> > make COCCI_PATCH=contrib/coccinelle/foo.cocci coccicheck\n> \n> The issue here is that the dependencies for foo.cocci become\n> unreliable, so I'd rather have a separate target for that (e.g.\n> depending on FORCE) if we go that way.\n\nCan you be more specific? I don't see how it's unreliable, unless you\nmean that anything relying on \"coccicheck\" would depend on the exact\nvalue of COCCI_PATCH.\n\nBut it may all be moot anyway, based no the responses elsewhere in the\nthread.\n\n-Peff\n"},{"id":"354581","messageId":"20180805230245.GB208811@aiede.svl.corp.google.com","threadId":"48941","inReplyTo":"20180803130824.GC4671@sigill.intra.peff.net","subject":"Re: [PoC] coccinelle: make Coccinelle-related make targets more fine-grained","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-05T23:02:45Z","receivedAt":"2018-08-05T23:02:51Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Thu, Aug 02, 2018 at 11:21:44PM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> I guess you could even replace \"COCCI_COMBINED\" with \"COCCI_PATCH\" in\n>>> most of the targets, and that would let people do individual:\n>>>\n>>> make COCCI_PATCH=contrib/coccinelle/foo.cocci coccicheck\n>>\n>> The issue here is that the dependencies for foo.cocci become\n>> unreliable, so I'd rather have a separate target for that (e.g.\n>> depending on FORCE) if we go that way.\n>\n> Can you be more specific? I don't see how it's unreliable, unless you\n> mean that anything relying on \"coccicheck\" would depend on the exact\n> value of COCCI_PATCH.\n\nYes, that's what I mean: changing which COCCI_PATCH to try would need\nto trigger re-runs.\n\n> But it may all be moot anyway, based no the responses elsewhere in the\n> thread.\n\nSure.\n\nThanks,\nJonathan\n"}]}