Re: [Cocci] excluding a function from coccinelle transformation

From: Jeff King <peff@peff.net>
Date: Aug 24, 2018, 20:53 UTC
Message-ID: <20180824205349.GA31853@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.21.1808240652370.2344@hadrien>

On Fri, Aug 24, 2018 at 07:04:27AM -0400, Julia Lawall wrote:

Show 23 quoted lines

> On Fri, 24 Aug 2018, Jeff King wrote:
> 
> > In Git's Coccinelle patches, we sometimes want to suppress a
> > transformation inside a particular function. For example, in finding
> > conversions of hashcmp() to oidcmp(), we should not convert the call in
> > oidcmp() itself, since that would cause infinite recursion. We write the
> > semantic patch like this:
> >
> >   @@
> >   identifier f != oidcmp;
> >   expression E1, E2;
> >   @@
> >     f(...) {...
> >   - hashcmp(E1->hash, E2->hash)
> >   + oidcmp(E1, E2)
> >     ...}
> 
> The problem is with how how ... works.  For transformation, A ... B
> requires that B occur on every execution path starting with A, unless that
> execution path ends up in error handling code.
> (eg, if (...) { ... return; }).  Here your A is the start if the function.
> So you need a call to hashcmp on every path through the function, which
> fails when you add ifs.

Thank you! This explanation (and the one below about A and B not appearing in the matched region) helped my understanding tremendously.

> What you want is what you ended up using, which is <... P ...> which
> allows zero or more occurrences of P.

And now this makes much more sense (I stumbled onto it through brute force, but now I understand _why_ it works).

> However, this can all be very expensive, because you are matching paths
> through the function definition which you don't really care about.  All
> you care about here is the name.  So another approach is

Yeah, it is. Using the pre-1.0.7 version, the original patch runs in ~1.3 minutes on my machine. With "<... P ...>" it's almost 4 minutes. Your python suggestion runs in about 1.5 minutes.

Curiously, 1.0.4 runs the original patch in only 24 seconds, and the angle-bracket one takes 52 seconds. I'm not sure if something changed in coccinelle, or if my build is simply less optimized (my 1.0.4 is from the Debian package, and I'm building 1.0.7 from source; I had trouble building 1.0.4 from source).

Show 7 quoted lines

> @@
> position p : script:python() { p[0].current_element != "oldcmp" };
> expression E1,E2;
> @@
> 
> - hashcmp(E1->hash, E2->hash)
> + oidcmp(E1, E2)

Aha, this is exactly the magic I was hoping for. I agree this is the best way to express it. I just had to tweak the patch to include the position:

  - hashcmp@p(E1->hash, E2->hash)

and it worked great. Unfortunately, Debian's spatch is not built with python support. :(

I'm not sure if we (the Git project) want to make the jump to requiring a more specific spatch. OTOH, only a handful of developers actually run it, and the python support does seem quite useful. And 1.0.4 is rather old at this point.

Again, thanks very much for your response. I have a much better understanding of what's going on now, and what our options are for moving forward.

-Peff