On Fri, 24 Aug 2018, Jeff King wrote:
> On Fri, Aug 24, 2018 at 07:04:27AM -0400, Julia Lawall wrote:
>
> > 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).
I don't remember the exact status of 1.0.4. It is possible that an
optimization was found to pose problems and was removed in the meantime.
<... ...> can be useful when you expect it to eg match an if branch. For
a function with over 1000 lines and many conditionals, it might not be a
good idea. Actually, the main problem is with loops. If there is a loop
in the function the performance can be much slower.
julia
>
> > @@
> > 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
>
