mbox series

[v3,00/11] cocci: make "incremental" possible + a ccache-like tool

Message ID cover-v3-00.11-00000000000-20221014T152552Z-avarab@gmail.com (mailing list archive)
Headers show
Series cocci: make "incremental" possible + a ccache-like tool | expand

Message

Ævar Arnfjörð Bjarmason Oct. 14, 2022, 3:31 p.m. UTC
A re-roll of the series to have "make coccicheck" run
incrementally. For the v1 see:
https://lore.kernel.org/git/cover-0.5-00000000000-20220825T141212Z-avarab@gmail.com/

The big change in this belated v3 is that we now run against a
concatenated version of all our *.cocci files. This is something I
discussed with Jeff King at Git Merge, after having confirmed the
viability of that approach on the cocci mailing list.

The result is that even with a from-scratch run of "make coccicheck"
this is faster than on "master", while of course adding the ability to
run incrementally, so a subsequent "coccicheck" is only run against
what we'd recompile.

The churn here is because I had to move/change some variables around
early in the series to make the eventual 08/11 smaller.

There's also improvements here to the bundled "spatchcache", it can
now optionally interoperate with non--very-quiet output (but won't
cache stderr), which is handy for debugging it. It can also be
configured to tell it where the .depend/* files are, which is a small
change to make it posible to use it outside of git.git..

Ævar Arnfjörð Bjarmason (11):
  Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)
  cocci rules: remove unused "F" metavariable from pending rule
  Makefile: add ability to TAB-complete cocci *.patch rules
  Makefile: have "coccicheck" re-run if flags change
  Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" heading
  cocci: split off include-less "tests" from SPATCH_FLAGS
  cocci: split off "--all-includes" from SPATCH_FLAGS
  cocci: make "coccicheck" rule incremental
  cocci: optimistically use COMPUTE_HEADER_DEPENDENCIES
  cocci: run against a generated ALL.cocci
  spatchcache: add a ccache-alike for "spatch"

 .gitignore                                    |   1 +
 Makefile                                      | 159 ++++++++--
 contrib/coccinelle/.gitignore                 |   1 +
 contrib/coccinelle/spatchcache                | 272 ++++++++++++++++++
 .../coccinelle/the_repository.pending.cocci   |   1 -
 shared.mak                                    |  10 +-
 6 files changed, 416 insertions(+), 28 deletions(-)
 create mode 100755 contrib/coccinelle/spatchcache

Range-diff against v2:
 -:  ----------- >  1:  4494c91df9a Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)
 1:  72b6a8e4e0b =  2:  8219b1b12f2 cocci rules: remove unused "F" metavariable from pending rule
 2:  0998948b881 !  3:  6dbfafa08fd Makefile: add ability to TAB-complete cocci *.patch rules
    @@ Makefile: check: $(GENERATED_H)
      		exit 1; \
      	fi
      
    -+COCCI = $(wildcard contrib/coccinelle/*.cocci)
    -+COCCI_PATCHES = $(addsuffix .patch,$(COCCI))
    -+COCCICHECK_PENDING = $(filter %.pending.cocci.patch,$(COCCI_PATCHES))
    -+COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_PATCHES))
    ++COCCI_GLOB = $(wildcard contrib/coccinelle/*.cocci)
    ++COCCI_RULES = $(COCCI_GLOB)
    ++
    ++COCCICHECK_PENDING = $(filter %.pending.cocci,$(COCCI_RULES))
    ++COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_RULES))
    ++
    ++COCCICHECK_PATCHES = $(COCCICHECK:%=%.patch)
    ++COCCICHECK_PATCHES_PENDING = $(COCCICHECK_PENDING:%=%.patch)
     +
      COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
      
     -%.cocci.patch: %.cocci $(COCCI_SOURCES)
    ++COCCI_PATCHES = $(COCCI_RULES:%=%.patch)
     +$(COCCI_PATCHES): $(COCCI_SOURCES)
     +$(COCCI_PATCHES): %.patch: %
      	$(QUIET_SPATCH) \
    @@ Makefile: $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib
      
      coccicheck: coccicheck-test
     -coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
    -+coccicheck: $(COCCICHECK)
    ++coccicheck: $(COCCICHECK_PATCHES)
      
      # See contrib/coccinelle/README
      coccicheck-pending: coccicheck-test
     -coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
    -+coccicheck-pending: $(COCCICHECK_PENDING)
    ++coccicheck-pending: $(COCCICHECK_PATCHES_PENDING)
      
      .PHONY: coccicheck coccicheck-pending
      
    -@@ Makefile: profile-clean:
    - 
    - cocciclean:
    - 	$(RM) -r .build/contrib/coccinelle
    --	$(RM) contrib/coccinelle/*.cocci.patch*
    -+	$(RM) $(COCCI_PATCHES)*
    - 
    - clean: profile-clean coverage-clean cocciclean
    - 	$(RM) -r .build
 3:  63cf9f58d99 !  4:  f779a2d22aa Makefile: have "coccicheck" re-run if flags change
    @@ Makefile: SANITIZE_ADDRESS =
      include config.mak.uname
      -include config.mak.autogen
      -include config.mak
    -@@ Makefile: COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_PATCHES))
    - 
    +@@ Makefile: COCCICHECK_PATCHES_PENDING = $(COCCICHECK_PENDING:%=%.patch)
      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: %
    @@ Makefile: $(COCCI_PATCHES): %.patch: %
      $(COCCI_TEST_RES_GEN): .build/%.res : %.c
      $(COCCI_TEST_RES_GEN): .build/%.res : %.res
      $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci
    +@@ Makefile: profile-clean:
    + 	$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
    + 
    + cocciclean:
    ++	$(RM) GIT-SPATCH-DEFINES
    + 	$(RM) -r .build/contrib/coccinelle
    + 	$(RM) contrib/coccinelle/*.cocci.patch*
    + 
 4:  54d6bae3984 =  5:  ab25b586f38 Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" heading
 5:  4f165bf6128 !  6:  691be73b6fb cocci: split off include-less "tests" from SPATCH_FLAGS
    @@ Makefile: $(COCCI_TEST_RES_GEN): .build/%.res : %.c
      $(COCCI_TEST_RES_GEN): .build/%.res : %.res
      $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci
      	$(call mkdir_p_parent_template)
    --	$(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_FLAGS) \
    -+	$(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_TEST_FLAGS) \
    +-	$(QUIET_SPATCH_TEST)$(SPATCH) $(SPATCH_FLAGS) \
    ++	$(QUIET_SPATCH_TEST)$(SPATCH) $(SPATCH_TEST_FLAGS) \
      		--very-quiet --no-show-diff \
      		--sp-file $< -o $@ \
      		$(@:.build/%.res=%.c) && \
 6:  c74d09f4825 =  7:  2ca5ea5beca cocci: split off "--all-includes" from SPATCH_FLAGS
 7:  120607b5da6 !  8:  2072a508064 cocci: make "coccicheck" rule incremental
    @@ Commit message
     
            This could be mitigated by combining "make -jN" with
            SPATCH_BATCH_SIZE, see 960154b9c17 (coccicheck: optionally batch
    -       spatch invocations, 2019-05-06). But doing so required careful
    -       juggling, as e.g. setting both to 4 would yield 16 workers.
    +       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
    @@ Commit message
            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
    @@ Makefile: TRACK_SPATCH_DEFINES += $(SPATCH)
      	@FLAGS='$(TRACK_SPATCH_DEFINES)'; \
      	    if test x"$$FLAGS" != x"`cat GIT-SPATCH-DEFINES 2>/dev/null`" ; then \
     @@ Makefile: check: $(GENERATED_H)
    - 		exit 1; \
    - 	fi
      
    + 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_PATCHES_PENDING = $(COCCICHECK_PENDING:%=%.patch)
    + COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_RULES))
    ++COCCICHECK_PATCHES = $(COCCICHECK:%=%.patch)
    ++
     +# It's expensive to compute the many=many rules below, only eval them
     +# on $(MAKECMDGOALS) that match these $(COCCI_RULES)
    -+COCCI_RULES =
    -+COCCI_RULES += cocci%
    -+COCCI_RULES += contrib/coccinelle/%
    -+COCCI_RULES += .build/contrib/coccinelle/%
    -+
    - COCCI = $(wildcard contrib/coccinelle/*.cocci)
    -+COCCI_NAMES = $(COCCI:contrib/coccinelle/%.cocci=%)
    - COCCI_PATCHES = $(addsuffix .patch,$(COCCI))
    - COCCICHECK_PENDING = $(filter %.pending.cocci.patch,$(COCCI_PATCHES))
    --COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_PATCHES))
    ++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))
    + 
    + COCCICHECK_PATCHES = $(COCCICHECK:%=%.patch)
    + COCCICHECK_PATCHES_PENDING = $(COCCICHECK_PENDING:%=%.patch)
      
    -+COCCICHECK = $(filter-out $(COCCICHECK_PENDING),$(COCCI_PATCHES))
      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: %
    @@ Makefile: check: $(GENERATED_H)
     +	fi
     +endef
     +
    -+define cocci-matrix-2
    ++define cocci-matrix
     +
    -+$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(c),$(s)))
    -+endef
    -+define cocci-matrix-1
    -+$(foreach c,$(COCCI),$(call cocci-matrix-2,$(c)))
    ++$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(1),$(s)))
     +endef
     +
    -+ifneq ($(filter $(COCCI_RULES),$(MAKECMDGOALS)),)
    -+$(eval $(call cocci-matrix-1))
    ++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_M)cat $$^ >$$@ && \
    ++	$(QUIET_SPATCH_CAT_TMPL)cat $$^ >$$@ && \
     +	if test -s $$@; \
      	then \
     -		echo '    ' SPATCH result: $@; \
    @@ Makefile: check: $(GENERATED_H)
      	fi
     +endef
     +
    -+ifneq ($(filter $(COCCI_RULES),$(MAKECMDGOALS)),)
    ++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
     @@ Makefile: profile-clean:
    - 
      cocciclean:
    + 	$(RM) GIT-SPATCH-DEFINES
      	$(RM) -r .build/contrib/coccinelle
    --	$(RM) $(COCCI_PATCHES)*
    -+	$(RM) $(COCCI_PATCHES)
    +-	$(RM) contrib/coccinelle/*.cocci.patch*
    ++	$(RM) $(COCCICHECK_PATCHES)
    ++	$(RM) $(COCCICHECK_PATCHES_PENDING)
      
      clean: profile-clean coverage-clean cocciclean
      	$(RM) -r .build
     
      ## shared.mak ##
     @@ shared.mak: ifndef V
    - 	QUIET_SP       = @echo '   ' SP $<;
    - 	QUIET_HDR      = @echo '   ' HDR $(<:hcc=h);
      	QUIET_RC       = @echo '   ' RC $@;
    --	QUIET_SPATCH   = @echo '   ' SPATCH $<;
    -+	QUIET_SPATCH   = @echo '   ' SPATCH $@;
    - 	QUIET_SPATCH_T = @echo '   ' SPATCH TEST $(@:.build/%=%);
    -+	QUIET_SPATCH_M = @echo '   ' SPATCH MERGE $$@;
      
    + ## Used in "Makefile": SPATCH
    +-	QUIET_SPATCH			= @echo '   ' SPATCH $<;
    ++	QUIET_SPATCH			= @echo '   ' SPATCH $@;
    + 	QUIET_SPATCH_TEST		= @echo '   ' SPATCH TEST $(@:.build/%=%);
    + 
    ++## Used in "Makefile": SPATCH_*TMPL (quoted for use in "define"'s)
    ++	QUIET_SPATCH_CAT_TMPL		= @echo '   ' SPATCH CAT $$$$^ \>$$@;
    ++
      ## Used in "Documentation/Makefile"
      	QUIET_ASCIIDOC	= @echo '   ' ASCIIDOC $@;
    + 	QUIET_XMLTO	= @echo '   ' XMLTO $@;
 8:  536dce45eef !  9:  739652eada9 cocci: optimistically use COMPUTE_HEADER_DEPENDENCIES
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    +@@ Makefile: SPATCH_INCLUDE_FLAGS = --all-includes
    + SPATCH_FLAGS =
    + SPATCH_TEST_FLAGS =
    + 
    ++# If *.o files are present, have "coccicheck" depend on them, with
    ++# COMPUTE_HEADER_DEPENDENCIES this will speed up the common-case of
    ++# only needing to re-generate coccicheck results for the users of a
    ++# given API if it's changed, and not all files in the project. If
    ++# COMPUTE_HEADER_DEPENDENCIES=no this will be unset too.
    ++SPATCH_USE_O_DEPENDENCIES = YesPlease
    ++
    + # Rebuild 'coccicheck' if $(SPATCH), its flags etc. change
    + TRACK_SPATCH_DEFINES =
    + TRACK_SPATCH_DEFINES += $(SPATCH)
     @@ Makefile: COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
      	$(call mkdir_p_parent_template)
      	$(QUIET_GEN) >$@
      
    -+SPATCH_USE_O_DEPENDENCIES = yes
     +ifeq ($(COMPUTE_HEADER_DEPENDENCIES),no)
     +SPATCH_USE_O_DEPENDENCIES =
     +endif
    -+
      define cocci-rule
      
      ## Rule for .build/$(1).patch/$(2); Params:
    @@ Makefile: COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
      	$$(call mkdir_p_parent_template)
     @@ Makefile: endef
      
    - define cocci-matrix-2
    + define cocci-matrix
      
    --$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(c),$(s)))
    +-$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(1),$(s)))
     +$(foreach s,$(COCCI_SOURCES),$(call cocci-rule,$(c),$(s),$(s:%.c=%.o)))
      endef
    - define cocci-matrix-1
    - $(foreach c,$(COCCI),$(call cocci-matrix-2,$(c)))
    + 
    + ifdef COCCI_GOALS
 -:  ----------- > 10:  52177ea2a68 cocci: run against a generated ALL.cocci
 9:  2b978676a56 ! 11:  f7ca3f9c9af spatchcache: add a ccache-alike for "spatch"
    @@ contrib/coccinelle/spatchcache (new)
     +#
     +# Note that the "--very-quiet" flag is currently critical. The cache
     +# will refuse to cache anything that has output on STDERR (which might
    -+# be errors from spatch). The STDERR (and exit code) could in
    -+# principle be cached (as with ccache), but then the simple structure
    -+# in the Redis cache would need to change, so just supply
    -+# "--very-quiet" for now.
    ++# be errors from spatch), but see spatchCache.cacheWhenStderr below.
    ++#
    ++# The STDERR (and exit code) could in principle be cached (as with
    ++# ccache), but then the simple structure in the Redis cache would need
    ++# to change, so just supply "--very-quiet" for now.
     +#
     +# To use this, simply set SPATCH to
     +# contrib/coccinelle/spatchcache. Then optionally set:
    @@ contrib/coccinelle/spatchcache (new)
     +#
     +# As well as this trace config (debug implies trace):
     +#
    ++#		cacheWhenStderr = true
     +#		trace = false
     +#		debug = false
     +#
    ++# The ".depend/grep.o.d" can also be customized, as a string that will
    ++# be eval'd, it has access to a "$dirname" and "$basename":
    ++#
    ++#	[spatchCache]
    ++#		dependFormat = "$dirname/.depend/${basename%.c}.o.d"
    ++#
     +# Setting "trace" to "true" allows for seeing when we have a cache HIT
     +# or MISS. To debug whether the cache is working do that, and run e.g.:
     +#
    @@ contrib/coccinelle/spatchcache (new)
     +#
     +# A subsequent "make cocciclean && make coccicheck" should then have
     +# all "HIT"'s and "CANTCACHE"'s.
    ++#
    ++# The "spatchCache.cacheWhenStderr" option is critical when using
    ++# spatchCache.{trace,debug} to debug whether something is set in the
    ++# cache, as we'll write to the spatch logs in .build/* we'd otherwise
    ++# always emit a NOCACHE.
     +
     +set -e
     +
    @@ contrib/coccinelle/spatchcache (new)
     +	trace=true
     +fi
     +
    ++cacheWhenStderr=$(git config --bool "spatchCache.cacheWhenStderr")
    ++if test "$cacheWhenStderr" != "true"
    ++then
    ++	cacheWhenStderr=
    ++fi
    ++
     +trace_it () {
     +	if test -z "$trace"
     +	then
    @@ contrib/coccinelle/spatchcache (new)
     +	spatch=spatch
     +fi
     +
    ++dependFormat='$dirname/.depend/${basename%.c}.o.d'
    ++dependFormatCfg=$(git config "spatchCache.dependFormat" || :)
    ++if test -n "$dependFormatCfg"
    ++then
    ++	dependFormat="$dependFormatCfg"
    ++fi
    ++
     +set=$(git config spatchCache.setCmd || :)
     +get=$(git config spatchCache.getCmd || :)
     +
    @@ contrib/coccinelle/spatchcache (new)
     +
     +hash_for_cache() {
     +	# Parameters that should affect the cache
    -+	echo "spatch=$spatch"
     +	echo "args=$args"
    ++	echo "config spatchCache.spatch=$spatch"
    ++	echo "config spatchCache.debug=$debug"
    ++	echo "config spatchCache.trace=$trace"
    ++	echo "config spatchCache.cacheWhenStderr=$cacheWhenStderr"
     +	echo
     +
     +	# Our target file and its dependencies
    @@ contrib/coccinelle/spatchcache (new)
     +fi
     +
     +# Main logic
    -+d=$(dirname "$arg_file")
    -+b=$(basename "$arg_file")
    -+bwoext="${b%.c}"
    -+dep="$d/.depend/$bwoext.o.d"
    ++dirname=$(dirname "$arg_file")
    ++basename=$(basename "$arg_file")
    ++eval "dep=$dependFormat"
     +
     +if ! test -f "$dep"
     +then
    @@ contrib/coccinelle/spatchcache (new)
     +err="$(mktemp)"
     +
     +set +e
    -+"$spatch" "$@" >"$out" 2>"$err"
    ++"$spatch" "$@" >"$out" 2>>"$err"
     +ret=$?
    ++cat "$out"
    ++cat "$err" >&2
     +set -e
     +
    -+if test $ret = 0 && ! test -s "$out" && ! test  -s "$err"
    ++nocache=
    ++if test $ret != 0
    ++then
    ++	nocache="exited non-zero: $ret"
    ++elif test -s "$out"
    ++then
    ++	nocache="had patch output"
    ++elif test -z "$cacheWhenStderr" && test -s "$err"
    ++then
    ++	nocache="had stderr (use --very-quiet or spatchCache.cacheWhenStderr=true?)"
    ++fi
    ++
    ++if test -n "$nocache"
     +then
    -+	rm -f "$out" "$err"
    ++	trace_it "$0: NOCACHE ($nocache): for '$arg_file' with '$arg_sp'"
    ++	exit "$ret"
    ++fi
     +
    -+	trace_it "$0: SET: for '$arg_file' with '$arg_sp'"
    ++trace_it "$0: SET: for '$arg_file' with '$arg_sp'"
     +
    -+	setret=
    -+	if test -z "$set"
    ++setret=
    ++if test -z "$set"
    ++then
    ++	if test $(redis-cli SADD spatch-cache "$sum") = 1
     +	then
    -+		if test $(redis-cli SADD spatch-cache "$sum") = 1
    -+		then
    -+			setret=0
    -+		else
    -+			setret=1
    -+		fi
    ++		setret=0
     +	else
    -+		"$set" "$sum"
    -+		setret=$?
    -+	fi
    -+
    -+	if test "$setret" != 0
    -+	then
    -+		echo "FAILED to set '$sum' in cache!" >&2
    -+		exit 128
    ++		setret=1
     +	fi
     +else
    -+	trace_it "$0: NOCACHE (have changes): for '$arg_file' with '$arg_sp'"
    -+	cat "$out"
    -+	cat "$err" >&2
    -+	rm -f "$out" "$err"
    -+	exit "$ret"
    ++	"$set" "$sum"
    ++	setret=$?
     +fi
    -+rm -f "$out" "$err"
    ++
    ++if test "$setret" != 0
    ++then
    ++	echo "FAILED to set '$sum' in cache!" >&2
    ++	exit 128
    ++fi
    ++
    ++exit "$ret"

Comments

Jeff King Oct. 17, 2022, 5:50 p.m. UTC | #1
On Fri, Oct 14, 2022 at 05:31:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

> The big change in this belated v3 is that we now run against a
> concatenated version of all our *.cocci files. This is something I
> discussed with Jeff King at Git Merge, after having confirmed the
> viability of that approach on the cocci mailing list.

Is that safe? The last time it was brought up on this list (AFAIK) was
in:

  https://lore.kernel.org/git/alpine.DEB.2.20.1808030755350.2446@hadrien/

where Julia said:

  I'm surprised that the above cat command would work.  Semantic patch rules
  have names, and Coccinelle will not be happy isf two rules have the same
  name.  Some may also have variables declared in initializers, although
  perhaps the ones in the kernel don't do this.  Causing these variables to
  be shared would not have a good effect.

  Putting everything together can also go counter to the optimizations that
  Coccinelle provides. [...]

Maybe we don't do any of the things that could trigger problems in our
spatch rules. But it's not clear to me what we're risking. Do you have a
link for further discussion?

-Peff
Ævar Arnfjörð Bjarmason Oct. 17, 2022, 6:36 p.m. UTC | #2
On Mon, Oct 17 2022, Jeff King wrote:

> On Fri, Oct 14, 2022 at 05:31:16PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> The big change in this belated v3 is that we now run against a
>> concatenated version of all our *.cocci files. This is something I
>> discussed with Jeff King at Git Merge, after having confirmed the
>> viability of that approach on the cocci mailing list.
>
> Is that safe? The last time it was brought up on this list (AFAIK) was
> in:
>
>   https://lore.kernel.org/git/alpine.DEB.2.20.1808030755350.2446@hadrien/
>
> where Julia said:
>
>   I'm surprised that the above cat command would work.  Semantic patch rules
>   have names, and Coccinelle will not be happy isf two rules have the same
>   name.  Some may also have variables declared in initializers, although
>   perhaps the ones in the kernel don't do this.  Causing these variables to
>   be shared would not have a good effect.
>
>   Putting everything together can also go counter to the optimizations that
>   Coccinelle provides. [...]
>
> Maybe we don't do any of the things that could trigger problems in our
> spatch rules. But it's not clear to me what we're risking. Do you have a
> link for further discussion?

I think 10/11's commit message should answer your question:
https://lore.kernel.org/git/patch-v3-10.11-52177ea2a68-20221014T152553Z-avarab@gmail.com/

The tl;dr is that it's not safe in the general case, as noted in the
post you & the more recent one I linked to in 10/11.

So, with this series doing:

	perl -pi -e 's/swap/preincrement/g' contrib/coccinelle/swap.cocci

Will error it if you run it with "SPATCH_CONCAT_COCCI=Y", but not with
"SPATCH_CONCAT_COCCI=", as the rule names conflict in the ALL.cocci.

But as 10/11 notes we can just avoid this by not picking conflicting
names, which doesn't seem like an undue burden.

AFAICT we have 5 named rules, and seemingly only 1/4 actually needs its
name, the rest are apparently only using it for self-documentation, and
we could either remove the name, or accomplish the same with a comment:
	
	diff --git a/contrib/coccinelle/hashmap.cocci b/contrib/coccinelle/hashmap.cocci
	index d69e120ccff..c5dbb4557b5 100644
	--- a/contrib/coccinelle/hashmap.cocci
	+++ b/contrib/coccinelle/hashmap.cocci
	@@ -1,4 +1,4 @@
	-@ hashmap_entry_init_usage @
	+@@
	 expression E;
	 struct hashmap_entry HME;
	 @@
	diff --git a/contrib/coccinelle/preincr.cocci b/contrib/coccinelle/preincr.cocci
	index 7fe1e8d2d9a..ae42cb07302 100644
	--- a/contrib/coccinelle/preincr.cocci
	+++ b/contrib/coccinelle/preincr.cocci
	@@ -1,4 +1,4 @@
	-@ preincrement @
	+@@
	 identifier i;
	 @@
	 - ++i > 1
	diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
	index 0970d98ad72..5f06105df6d 100644
	--- a/contrib/coccinelle/strbuf.cocci
	+++ b/contrib/coccinelle/strbuf.cocci
	@@ -1,4 +1,4 @@
	-@ strbuf_addf_with_format_only @
	+@@
	 expression E;
	 constant fmt !~ "%";
	 @@
	diff --git a/contrib/coccinelle/swap.cocci b/contrib/coccinelle/swap.cocci
	index a0934d1fdaf..522177afb66 100644
	--- a/contrib/coccinelle/swap.cocci
	+++ b/contrib/coccinelle/swap.cocci
	@@ -1,4 +1,4 @@
	-@ swap_with_declaration @
	+@@
	 type T;
	 identifier tmp;
	 T a, b;
Junio C Hamano Oct. 17, 2022, 7:08 p.m. UTC | #3
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But as 10/11 notes we can just avoid this by not picking conflicting
> names, which doesn't seem like an undue burden.

As long as it is clearly documented to make it well known to
everybody who would attempt to touch or add .cocci files, I am OK
with that direction.

> AFAICT we have 5 named rules, and seemingly only 1/4 actually needs its
> name, the rest are apparently only using it for self-documentation, and
> we could either remove the name, or accomplish the same with a comment:

Well, if they are for self-documentation, they'd better be unique,
or they are not self-documenting enough, no ;-)?
Jeff King Oct. 17, 2022, 7:18 p.m. UTC | #4
On Mon, Oct 17, 2022 at 08:36:46PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Maybe we don't do any of the things that could trigger problems in our
> > spatch rules. But it's not clear to me what we're risking. Do you have a
> > link for further discussion?
> 
> I think 10/11's commit message should answer your question:
> https://lore.kernel.org/git/patch-v3-10.11-52177ea2a68-20221014T152553Z-avarab@gmail.com/
> 
> The tl;dr is that it's not safe in the general case, as noted in the
> post you & the more recent one I linked to in 10/11.

Thanks. Holy cow, the coccinelle list is hard to use compared to
lore/public-inbox. The direct link to the thread in question is:

  https://sympa.inria.fr/sympa/arc/cocci/2022-09/msg00001.html

(you'll need to click a javascript button to see it, though).

> So, with this series doing:
> 
> 	perl -pi -e 's/swap/preincrement/g' contrib/coccinelle/swap.cocci
> 
> Will error it if you run it with "SPATCH_CONCAT_COCCI=Y", but not with
> "SPATCH_CONCAT_COCCI=", as the rule names conflict in the ALL.cocci.
> 
> But as 10/11 notes we can just avoid this by not picking conflicting
> names, which doesn't seem like an undue burden.

Yeah, that seems OK, then. In fact, I'd be fine with guidance in the
README saying "don't bother with a name if you don't need it".

-Peff