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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v3 00/11] cocci: make "incremental" possible + a ccache-like tool
Date: Mon, 17 Oct 2022 20:36:46 +0200	[thread overview]
Message-ID: <221017.86czaqjnhy.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y02V0E0JRSNa4Sb9@coredump.intra.peff.net>

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;