From: Julia Lawall <julia.lawall@lip6.fr>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, cocci@systeme.lip6.fr
Subject: Re: [Cocci] excluding a function from coccinelle transformation
Date: Fri, 24 Aug 2018 17:00:38 -0400 (EDT) [thread overview]
Message-ID: <alpine.DEB.2.21.1808241658430.2378@hadrien> (raw)
In-Reply-To: <20180824205349.GA31853@sigill.intra.peff.net>
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
>
prev parent reply other threads:[~2018-08-24 21:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-24 6:42 excluding a function from coccinelle transformation Jeff King
2018-08-24 11:04 ` [Cocci] " Julia Lawall
2018-08-24 20:53 ` Jeff King
2018-08-24 21:00 ` Julia Lawall [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.1808241658430.2378@hadrien \
--to=julia.lawall@lip6.fr \
--cc=cocci@systeme.lip6.fr \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).