Date: Fri, 5 Jun 2015 18:16:20 +0200 (CEST)
From: Julia Lawall <julia.lawall@lip6.fr>
To: Fabian Frederick <fabf@skynet.be>
cc: Nicolas Palix <nicolas.palix@imag.fr>, Michal Marek <mmarek@suse.cz>,
        Gilles Muller <Gilles.Muller@lip6.fr>, linux-kernel@vger.kernel.org,
        cocci@systeme.lip6.fr
Subject: Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci
In-Reply-To: <660708757.633754.1433518363194.open-xchange@webmail.nmp.proximus.be>
Message-ID: <alpine.DEB.2.10.1506051813390.2388@hadrien>
References: <1432060501-9463-1-git-send-email-fabf@skynet.be> <alpine.DEB.2.02.1505311131150.2084@localhost6.localdomain6> <660708757.633754.1433518363194.open-xchange@webmail.nmp.proximus.be>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1833268373-1433520980=:2388"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5270
Lines: 194

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1833268373-1433520980=:2388
Content-Type: TEXT/PLAIN; charset=UTF-8
Content-Transfer-Encoding: 8BIT

On Fri, 5 Jun 2015, Fabian Frederick wrote:

>
>
> > On 31 May 2015 at 11:42 Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > I propose the extended version below (not currently coccicheck friendly). 
> > the extra features are:
> >
> > 1.  The original version requires i1 and i2 to be identifiers, eg x and y. 
> > This doesn't address the case where they are terms line x->a or x[b].  The
> > fact that the original code contains assignments with both i1 and i2 on
> > the left hand side is enough to ensure that they are appropriate arguments
> > for swap.  So they can be changed to expression metavariables.
> >
> > 2. The original patch rule always removed the tmp variable.  This is not
> > valid if the tmp variable is used for something else.  The new semantic
> > patch separates the introduction of swap (rule r) from the removal of the
> > variable declaration (rule ok and the one folowing).  The rule ok checks
> > that this is a function containing an introduced call to swap, and then
> > the rule after removes the declaration if the variable is not used for
> > anything else.  Note that the name of the tmp variable is remembered in
> > the invalid three-argument version of sawp.  This is then cleaned up in
> > the rule below.
> >
> > 3. The original patch always removed the initialization of the tmp
> > variable.  Actually, some code uses the tmp variable afterwards to refer
> > to the old value.  In the new semantic patch, the first set of rules
> > considers the cases where the tmp variable is not used, and the last rule
> > is for the case where the tmp variable is stll needed.  No cleaning up of
> > the declaration is needed in that case.
> >
> > There is one regression as compared to the original semantic patch: In the
> > file lib/mpi/mpi-pow.c, the temporary variable is not needed after the
> > change, but it is also not removed.  It is declared within a loop, and
> > Coccinelle does not realize that it is not needed afterwards, because it
> > is needed on subsequent loop iterations.  Trying to adjust the semantic
> > patch to address this issue made it much slower and didn't fix the
> > problem.  Perhaps it is easier to rely on gcc to give an unused variable
> > warning, and to clean it up then.
> >
> > Fabian, if you are o with this, do you want to sgenify it ans submit a new
> > patch?
>
> Hello Julia,
>
>         Interesting improvements :) Do we really need tmp variable in swap() ?
> I tried the version below which removes swap(x,y,tmp)->swap(x,y) rule
> and had the same result on drivers branch plus it solved a missing tmp.
> Maybe you want to avoid out of context swap() calls ?
>
> Regards,
> Fabian
>
> @r@
> expression i1, i2, E;
> identifier tmp;
> @@
>
> - tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
>   ... when != tmp
> ? tmp = E
>
> @ok exists@
> type t1;
> identifier r.tmp;
> expression i1,i2;
> position p;
> @@
>
> t1@p tmp;
> ...
> swap(i1, i2);

There is no connection between tmp and swap here.  Tmp does have the same
name as some variable used in a previous swap, but there is no guarantee
that this swap here is the one that was introduced by the previous rule.
Unfortunately, when you make a transformation, all information about
positions is lost.  So there is no way to put a position variable on a
generated swap and ensure that the one matched here is exactly the same.

Maybe you could go a little closer to ensuring that property by inheriting
i1 and i2 as well, and not just tmp.

julia

>
> @@
> expression i1,i2;
> identifier tmp;
> type t1;
> position ok.p;
> @@
>
> -t1@p tmp;
>  <... when strict
>       when != tmp
>  swap(i1, i2);
>  ...>
>
>
> // tmp variable still needed
>
> @@
> expression i1, i2;
> identifier tmp;
> @@
>
>   tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
>
>
> >
> > thanks,
> > julia
> >
> > // it may be possible to remove the tmp variable
> >
> > @r@
> > expression i1, i2, E;
> > identifier tmp;
> > @@
> >
> > - tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2, tmp);
> >   ... when != tmp
> > ? tmp = E
> >
> > @ok exists@
> > type t1;
> > identifier r.tmp;
> > expression i1,i2;
> > position p;
> > @@
> >
> > t1@p tmp;
> > ...
> > swap(i1, i2, tmp);
> >
> > @@
> > expression i1,i2;
> > identifier tmp;
> > type t1;
> > position ok.p;
> > @@
> >
> > -t1@p tmp;
> >  <... when strict
> >       when != tmp
> >  swap(i1, i2, tmp);
> >  ...>
> >
> > @depends on r@
> > expression i1,i2;
> > identifier tmp;
> > @@
> >
> > swap(i1,i2
> > - ,tmp
> >  )
> >
> > // tmp variable still needed
> >
> > @@
> > expression i1, i2;
> > identifier tmp;
> > @@
> >
> >   tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2);
>
--8323329-1833268373-1433520980=:2388--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/