git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v5 08/13] cocci: make "coccicheck" rule incremental

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 1, 2022, 22:35 UTC
Message-ID
<patch-v5-08.13-ee9ad433ab5-20221101T222616Z-avarab@gmail.com>
In-Reply-To
<cover-v5-00.13-00000000000-20221101T222616Z-avarab@gmail.com>

Optimize the very slow "coccicheck" target to take advantage of incremental rebuilding, and fix outstanding dependency problems with the existing rule.

The rule is now faster both on the initial run as we can make better use of GNU make's parallelism than the old ad-hoc combination of make's parallelism combined with $(SPATCH_BATCH_SIZE) and/or the "--jobs" argument to "spatch(1)".

It also makes us *much* faster when incrementally building, it's now viable to "make coccicheck" as topic branches are merged down.

The rule didn't use FORCE (or its equivalents) before, so a:
	make coccicheck
	make coccicheck

Would report nothing to do on the second iteration. But all of our patch output depended on all $(COCCI_SOURCES) files, therefore e.g.:

    make -W grep.c coccicheck

Would do a full re-run, i.e. a a change in a single file would force us to do a full re-run.

The reason for this (not the initial rationale, but my analysis) is:
* Since we create a single "*.cocci.patch+" we don't know where to
  pick up where we left off, or how to incrementally merge e.g. a
  "grep.c" change with an existing *.cocci.patch.
* We've been carrying forward the dependency on the *.c files since
  63f0a758a06 (add coccicheck make target, 2016-09-15) the rule was
  initially added as a sort of poor man's dependency discovery.
  As we don't include other *.c files depending on other *.c files
  has always been broken, as could be trivially demonstrated
  e.g. with:
       make coccicheck
       make -W strbuf.h coccicheck
  However, depending on the corresponding *.c files has been doing
  something, namely that *if* an API change modified both *.c and *.h
  files we'd catch the change to the *.h we care about via the *.c
  being changed.
  For API changes that happened only via *.h files we'd do the wrong
  thing before this change, but e.g. for function additions (not
  "static inline" ones) catch the *.h change by proxy.
Now we'll instead:
 * Create a <RULE>/<FILE> pair in the .build directory, E.g. for
   swap.cocci and grep.c we'll create
   .build/contrib/coccinelle/swap.cocci.patch/grep.c.
   That file is the diff we'll apply for that <RULE>-<FILE>
   combination, if there's no changes to me made (the common case)
   it'll be an empty file.
 * Our generated *.patch
   file (e.g. contrib/coccinelle/swap.cocci.patch) is now a simple "cat
   $^" of all of all of the <RULE>/<FILE> files for a given <RULE>.
   In the case discussed above of "grep.c" being changed we'll do the
   full "cat" every time, so they resulting *.cocci.patch will always
   be correct and up-to-date, even if it's "incrementally updated".
   See 1cc0425a27c (Makefile: have "make pot" not "reset --hard",
   2022-05-26) for another recent rule that used that technique.
As before we'll:
 * End up generating a contrib/coccinelle/swap.cocci.patch, if we
   "fail" by creating a non-empty patch we'll still exit with a zero
   exit code.
   Arguably we should move to a more Makefile-native way of doing
   this, i.e. fail early, and if we want all of the "failed" changes
   we can use "make -k", but as the current
   "ci/run-static-analysis.sh" expects us to behave this way let's
   keep the existing behavior of exhaustively discovering all cocci
   changes, and only failing if spatch itself errors out.
Further implementation details & notes:
 * Before this change running "make coccicheck" would by default end
   up pegging just one CPU at the very end for a while, usually as
   we'd finish whichever *.cocci rule was the most expensive.
   This could be mitigated by combining "make -jN" with
   SPATCH_BATCH_SIZE, see 960154b9c17 (coccicheck: optionally batch
   spatch invocations, 2019-05-06).
   There will be cases where getting rid of "SPATCH_BATCH_SIZE" makes
   things worse, but a from-scratch "make coccicheck" with the default
   of SPATCH_BATCH_SIZE=1 (and tweaking it doesn't make a difference)
   is faster (~3m36s v.s. ~3m56s) with this approach, as we can feed
   the CPU more work in a less staggered way.
 * Getting rid of "SPATCH_BATCH_SIZE" particularly helps in cases
   where the default of 1 yields parallelism under "make coccicheck",
   but then running e.g.:
       make -W contrib/coccinelle/swap.cocci coccicheck
   I.e. before that would use only one CPU core, until the user
   remembered to adjust "SPATCH_BATCH_SIZE" differently than the
   setting that makes sense when doing a non-incremental run of "make
   coccicheck".
 * Before the "make coccicheck" rule would have to clean
   "contrib/coccinelle/*.cocci.patch*", since we'd create "*+" and
   "*.log" files there. Now those are created in
   .build/contrib/coccinelle/, which is covered by the "cocciclean" rule
   already.
Outstanding issues & future work:
 * We could get rid of "--all-includes" in favor of manually
   specifying a list of includes to give to "spatch(1)".
   As noted upthread of [1] a naïve removal of "--all-includes" will
   result in broken *.cocci patches, but if we know the exhaustive
   list of includes via COMPUTE_HEADER_DEPENDENCIES we don't need to
   re-scan for them, we could grab the headers to include from the
   .depend.d/<file>.o.d and supply them with the "--include" option to
   spatch(1).q
1. https://lore.kernel.org/git/87ft18tcog.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                      | 81 ++++++++++++++++++++++++-----------
 contrib/coccinelle/.gitignore |  2 +-
 shared.mak                    |  3 +-
 3 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/Makefile b/Makefile
index 25f0c7e837d..4bedff26285 100644
--- a/Makefile
+++ b/Makefile
@@ -1306,10 +1306,6 @@ SANITIZE_ADDRESS =
 SPATCH_INCLUDE_FLAGS = --all-includes
 SPATCH_FLAGS =
 SPATCH_TEST_FLAGS =
-# Setting SPATCH_BATCH_SIZE higher will
-# usually result in less CPU usage at the cost of higher peak memory.
-# Setting it to 0 will feed all files in a single spatch invocation.
-SPATCH_BATCH_SIZE = 1
 
 # Rebuild 'coccicheck' if $(SPATCH), its flags etc. change
 TRACK_SPATCH_DEFINES =
@@ -1317,7 +1313,6 @@ TRACK_SPATCH_DEFINES += $(SPATCH)
 TRACK_SPATCH_DEFINES += $(SPATCH_INCLUDE_FLAGS)
 TRACK_SPATCH_DEFINES += $(SPATCH_FLAGS)
 TRACK_SPATCH_DEFINES += $(SPATCH_TEST_FLAGS)
-TRACK_SPATCH_DEFINES += $(SPATCH_BATCH_SIZE)
 GIT-SPATCH-DEFINES: FORCE
 	@FLAGS='$(TRACK_SPATCH_DEFINES)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-SPATCH-DEFINES 2>/dev/null`" ; then \
@@ -3163,6 +3158,7 @@ check: $(GENERATED_H)
 
 COCCI_GLOB = $(wildcard contrib/coccinelle/*.cocci)
 COCCI_RULES = $(COCCI_GLOB)
+COCCI_NAMES = $(COCCI_RULES:contrib/coccinelle/%.cocci=%)
 
 COCCICHECK_PENDING = $(filter %.pending.cocci,$(COCCI_RULES))
 COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_RULES))
@@ -3170,32 +3166,65 @@ COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_RULES))
 COCCICHECK_PATCHES = $(COCCICHECK:%=%.patch)
 COCCICHECK_PATCHES_PENDING = $(COCCICHECK_PENDING:%=%.patch)
 
+# It's expensive to compute the many=many rules below, only eval them
+# on $(MAKECMDGOALS) that match these $(COCCI_RULES)
+COCCI_RULES_GLOB =
+COCCI_RULES_GLOB += cocci%
+COCCI_RULES_GLOB += .build/contrib/coccinelle/%
+COCCI_RULES_GLOB += $(COCCICHECK_PATCHES)
+COCCI_RULES_GLOB += $(COCCICHEC_PATCHES_PENDING)
+COCCI_GOALS = $(filter $(COCCI_RULES_GLOB),$(MAKECMDGOALS))
+
 COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
 
-COCCI_PATCHES = $(COCCI_RULES:%=%.patch)
-$(COCCI_PATCHES): GIT-SPATCH-DEFINES
-$(COCCI_PATCHES): $(COCCI_SOURCES)
-$(COCCI_PATCHES): %.patch: %
-	$(QUIET_SPATCH) \
-	if test $(SPATCH_BATCH_SIZE) = 0; then \
-		limit=; \
-	else \
-		limit='-n $(SPATCH_BATCH_SIZE)'; \
-	fi; \
-	if ! echo $(COCCI_SOURCES) | xargs $$limit \
-		$(SPATCH) $(SPATCH_FLAGS) \
-		$(SPATCH_INCLUDE_FLAGS) \
-		--sp-file $< --patch . \
-		>$@+ 2>$@.log; \
+.build/contrib/coccinelle/FOUND_H_SOURCES: $(FOUND_H_SOURCES)
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN) >$@
+
+define cocci-rule
+
+## Rule for .build/$(1).patch/$(2); Params:
+# $(1) = e.g. "free.cocci"
+# $(2) = e.g. "grep.c"
+COCCI_$(1:contrib/coccinelle/%.cocci=%) += .build/$(1).patch/$(2)
+.build/$(1).patch/$(2): GIT-SPATCH-DEFINES
+.build/$(1).patch/$(2): .build/contrib/coccinelle/FOUND_H_SOURCES
+.build/$(1).patch/$(2): $(1)
+.build/$(1).patch/$(2): .build/$(1).patch/% : %
+	$$(call mkdir_p_parent_template)
+	$$(QUIET_SPATCH)if ! $$(SPATCH) $$(SPATCH_FLAGS) \
+		$$(SPATCH_INCLUDE_FLAGS) \
+		--sp-file $(1) --patch . $$< \
+		>$$@ 2>$$@.log; \
 	then \
-		cat $@.log; \
+		echo "ERROR when applying '$(1)' to '$$<'; '$$@.log' follows:"; \
+		cat $$@.log; \
 		exit 1; \
-	fi; \
-	mv $@+ $@; \
-	if test -s $@; \
+	fi
+endef
+
+define cocci-matrix
+
+$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(1),$(s)))
+endef
+
+ifdef COCCI_GOALS
+$(eval $(foreach c,$(COCCI_RULES),$(call cocci-matrix,$(c))))
+endif
+
+define spatch-rule
+
+contrib/coccinelle/$(1).cocci.patch: $$(COCCI_$(1))
+	$$(QUIET_SPATCH_CAT)cat $$^ >$$@ && \
+	if test -s $$@; \
 	then \
-		echo '    ' SPATCH result: $@; \
+		echo '    ' SPATCH result: $$@; \
 	fi
+endef
+
+ifdef COCCI_GOALS
+$(eval $(foreach n,$(COCCI_NAMES),$(call spatch-rule,$(n))))
+endif
 
 COCCI_TEST_RES_GEN = $(addprefix .build/,$(COCCI_TEST_RES))
 $(COCCI_TEST_RES_GEN): GIT-SPATCH-DEFINES
@@ -3487,7 +3516,7 @@ profile-clean:
 cocciclean:
 	$(RM) GIT-SPATCH-DEFINES
 	$(RM) -r .build/contrib/coccinelle
-	$(RM) contrib/coccinelle/*.cocci.patch*
+	$(RM) contrib/coccinelle/*.cocci.patch
 
 clean: profile-clean coverage-clean cocciclean
 	$(RM) -r .build
diff --git a/contrib/coccinelle/.gitignore b/contrib/coccinelle/.gitignore
index d3f29646dc3..1d45c0a40c8 100644
--- a/contrib/coccinelle/.gitignore
+++ b/contrib/coccinelle/.gitignore
@@ -1 +1 @@
-*.patch*
+*.patch
diff --git a/shared.mak b/shared.mak
index 96b06acc455..f437073e48c 100644
--- a/shared.mak
+++ b/shared.mak
@@ -71,8 +71,9 @@ ifndef V
 	QUIET_RC       = @echo '   ' RC $@;
 
 ## Used in "Makefile": SPATCH
-	QUIET_SPATCH			= @echo '   ' SPATCH $<;
+	QUIET_SPATCH			= @echo '   ' SPATCH $@;
 	QUIET_SPATCH_TEST		= @echo '   ' SPATCH TEST $(@:.build/%=%);
+	QUIET_SPATCH_CAT		= @echo '   ' SPATCH CAT $$^ \>$@;
 
 ## Used in "Documentation/Makefile"
 	QUIET_ASCIIDOC	= @echo '   ' ASCIIDOC $@;
-- 
2.38.0.1280.g8136eb6fab2
Previous: Ævar Arnfjörð BjarmasonNext: SZEDER Gábor
Message 62 of 72 in “cocci: make "incremental" possible + a ccache-like tool”
  1. 0/5 cocci: make "incremental" possible + a ccache-like toolÆvar Arnfjörð Bjarmason, Aug 25, 2022
  2. 1/5 Makefile: add ability to TAB-complete cocci *.patch rulesÆvar Arnfjörð Bjarmason, Aug 25, 2022
  3. 2/5 Makefile: have "coccicheck" re-run if flags changeÆvar Arnfjörð Bjarmason, Aug 25, 2022
  4. SZEDER GáborAug 25, 2022
  5. 4/5 cocci: make incremental compilation even fasterÆvar Arnfjörð Bjarmason, Aug 25, 2022
  6. 3/5 cocci: make "coccicheck" rule incrementalÆvar Arnfjörð Bjarmason, Aug 25, 2022
  7. SZEDER GáborAug 25, 2022
  8. Ævar Arnfjörð BjarmasonAug 25, 2022
  9. SZEDER GáborAug 26, 2022
  10. 5/5 spatchcache: add a ccache-alike for "spatch"Ævar Arnfjörð Bjarmason, Aug 25, 2022
  11. 0/9 cocci: make "incremental" possible + a ccache-like toolÆvar Arnfjörð Bjarmason, Aug 31, 2022
  12. 2/9 Makefile: add ability to TAB-complete cocci *.patch rulesÆvar Arnfjörð Bjarmason, Aug 31, 2022
  13. 3/9 Makefile: have "coccicheck" re-run if flags changeÆvar Arnfjörð Bjarmason, Aug 31, 2022
  14. 1/9 cocci rules: remove unused "F" metavariable from pending ruleÆvar Arnfjörð Bjarmason, Aug 31, 2022
  15. 4/9 Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" headingÆvar Arnfjörð Bjarmason, Aug 31, 2022
  16. 5/9 cocci: split off include-less "tests" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Aug 31, 2022
  17. 6/9 cocci: split off "--all-includes" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Aug 31, 2022
  18. 8/9 cocci: optimistically use COMPUTE_HEADER_DEPENDENCIESÆvar Arnfjörð Bjarmason, Aug 31, 2022
  19. 7/9 cocci: make "coccicheck" rule incrementalÆvar Arnfjörð Bjarmason, Aug 31, 2022
  20. SZEDER GáborSep 1, 2022
  21. Ævar Arnfjörð BjarmasonSep 1, 2022
  22. 9/9 spatchcache: add a ccache-alike for "spatch"Ævar Arnfjörð Bjarmason, Aug 31, 2022
  23. 00/11 cocci: make "incremental" possible + a ccache-like toolÆvar Arnfjörð Bjarmason, Oct 14, 2022
  24. 01/11 Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)Ævar Arnfjörð Bjarmason, Oct 14, 2022
  25. 02/11 cocci rules: remove unused "F" metavariable from pending ruleÆvar Arnfjörð Bjarmason, Oct 14, 2022
  26. 05/11 Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" headingÆvar Arnfjörð Bjarmason, Oct 14, 2022
  27. Taylor BlauOct 14, 2022
  28. 03/11 Makefile: add ability to TAB-complete cocci *.patch rulesÆvar Arnfjörð Bjarmason, Oct 14, 2022
  29. 07/11 cocci: split off "--all-includes" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Oct 14, 2022
  30. 04/11 Makefile: have "coccicheck" re-run if flags changeÆvar Arnfjörð Bjarmason, Oct 14, 2022
  31. 06/11 cocci: split off include-less "tests" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Oct 14, 2022
  32. 08/11 cocci: make "coccicheck" rule incrementalÆvar Arnfjörð Bjarmason, Oct 14, 2022
  33. 09/11 cocci: optimistically use COMPUTE_HEADER_DEPENDENCIESÆvar Arnfjörð Bjarmason, Oct 14, 2022
  34. 11/11 spatchcache: add a ccache-alike for "spatch"Ævar Arnfjörð Bjarmason, Oct 14, 2022
  35. 10/11 cocci: run against a generated ALL.cocciÆvar Arnfjörð Bjarmason, Oct 14, 2022
  36. Jeff KingOct 17, 2022
  37. Ævar Arnfjörð BjarmasonOct 17, 2022
  38. Junio C HamanoOct 17, 2022
  39. Jeff KingOct 17, 2022
  40. 00/12 cocci: make "incremental" possible + a ccache-like toolÆvar Arnfjörð Bjarmason, Oct 26, 2022
  41. 01/12 Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)Ævar Arnfjörð Bjarmason, Oct 26, 2022
  42. 02/12 cocci rules: remove unused "F" metavariable from pending ruleÆvar Arnfjörð Bjarmason, Oct 26, 2022
  43. 03/12 Makefile: add ability to TAB-complete cocci *.patch rulesÆvar Arnfjörð Bjarmason, Oct 26, 2022
  44. 04/12 Makefile: have "coccicheck" re-run if flags changeÆvar Arnfjörð Bjarmason, Oct 26, 2022
  45. 05/12 Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" headingÆvar Arnfjörð Bjarmason, Oct 26, 2022
  46. 07/12 cocci: split off "--all-includes" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Oct 26, 2022
  47. 06/12 cocci: split off include-less "tests" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Oct 26, 2022
  48. 08/12 cocci: make "coccicheck" rule incrementalÆvar Arnfjörð Bjarmason, Oct 26, 2022
  49. 09/12 cocci: optimistically use COMPUTE_HEADER_DEPENDENCIESÆvar Arnfjörð Bjarmason, Oct 26, 2022
  50. 10/12 cocci rules: remove <id>'s from rules that don't need themÆvar Arnfjörð Bjarmason, Oct 26, 2022
  51. 11/12 cocci: run against a generated ALL.cocciÆvar Arnfjörð Bjarmason, Oct 26, 2022
  52. SZEDER GáborOct 28, 2022
  53. 12/12 spatchcache: add a ccache-alike for "spatch"Ævar Arnfjörð Bjarmason, Oct 26, 2022
  54. 00/13 cocci: make "incremental" possible + a ccache-like toolÆvar Arnfjörð Bjarmason, Nov 1, 2022
  55. 01/13 Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)Ævar Arnfjörð Bjarmason, Nov 1, 2022
  56. 02/13 cocci rules: remove unused "F" metavariable from pending ruleÆvar Arnfjörð Bjarmason, Nov 1, 2022
  57. 03/13 Makefile: add ability to TAB-complete cocci *.patch rulesÆvar Arnfjörð Bjarmason, Nov 1, 2022
  58. 04/13 Makefile: have "coccicheck" re-run if flags changeÆvar Arnfjörð Bjarmason, Nov 1, 2022
  59. 06/13 cocci: split off include-less "tests" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Nov 1, 2022
  60. 05/13 Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" headingÆvar Arnfjörð Bjarmason, Nov 1, 2022
  61. 07/13 cocci: split off "--all-includes" from SPATCH_FLAGSÆvar Arnfjörð Bjarmason, Nov 1, 2022
  62. 08/13 cocci: make "coccicheck" rule incrementalÆvar Arnfjörð Bjarmason, Nov 1, 2022
  63. SZEDER GáborNov 9, 2022
  64. 10/13 Makefile: copy contrib/coccinelle/*.cocci to build/Ævar Arnfjörð Bjarmason, Nov 1, 2022
  65. SZEDER GáborNov 9, 2022
  66. Ævar Arnfjörð BjarmasonNov 9, 2022
  67. Makefile: don't create a ".build/.build/" for cocci, fix outputÆvar Arnfjörð Bjarmason, Nov 10, 2022
  68. Taylor BlauNov 11, 2022
  69. 09/13 cocci: optimistically use COMPUTE_HEADER_DEPENDENCIESÆvar Arnfjörð Bjarmason, Nov 1, 2022
  70. 11/13 cocci rules: remove <id>'s from rules that don't need themÆvar Arnfjörð Bjarmason, Nov 1, 2022
  71. 12/13 cocci: run against a generated ALL.cocciÆvar Arnfjörð Bjarmason, Nov 1, 2022
  72. 13/13 spatchcache: add a ccache-alike for "spatch"Ævar Arnfjörð Bjarmason, Nov 1, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.