[PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Fabian Frederick]

Operations like

int a, b, tmp;

tmp = a;
a = b;
b = tmp;

can be replaced by

int a, b;

swap(a, b);

This uses kernel.h macro definition and simplifies the code.

Thanks to Julia Lawall for suggestions about expressions and sgen

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 scripts/coccinelle/misc/swap.cocci | 62 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 scripts/coccinelle/misc/swap.cocci

diff --git a/scripts/coccinelle/misc/swap.cocci b/scripts/coccinelle/misc/swap.cocci
new file mode 100644
index 0000000..0f0929c
--- /dev/null
+++ b/scripts/coccinelle/misc/swap.cocci
@@ -0,0 +1,62 @@
+/// Use swap macro instead of complete operation to simplify the code
+///
+// # Generated with sgen
+//
+// Confidence: Moderate
+// Copyright: (C) 2015 Fabian Frederick. GPLv2.
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@r depends on patch && !context && !org && !report@
+identifier i1, i2, tmp;
+type t1;
+@@
+
+- t1 tmp;
+<+... when any
+- tmp = i1;
+- i1 = i2;
+- i2 = tmp;
++ swap(i1, i2);
+...+>
+
+
+// ----------------------------------------------------------------------------
+
+@r_context depends on !patch && (context || org || report)@
+type t1;
+identifier i1, i2, tmp;
+position j0, j1;
+@@
+
+*  t1 tmp@j0;
+<+... when any
+*  tmp@j1 = i1;
+*  i1 = i2;
+*  i2 = tmp;
+...+>
+
+// ----------------------------------------------------------------------------
+
+@script:python r_org depends on org@
+j0 << r_context.j0;
+j1 << r_context.j1;
+@@
+
+msg = "swap.cocci"
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python r_report depends on report@
+j0 << r_context.j0;
+j1 << r_context.j1;
+@@
+
+msg = "WARNING: use swap() and remove temporary variable if it's not used elsewhere around line %s." % (j1[0].line)
+coccilib.report.print_report(j0[0], msg)
+
-- 
2.4.0

Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Julia Lawall]

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?

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);

Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Julia Lawall]

Hmm, I do get more unused variable warnings than expected.  I will have to 
look into it.

julia

On Sun, 31 May 2015, Julia Lawall 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?
> 
> 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);
> 

Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Fabian Frederick]

> 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);

@@
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);

Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4721 bytes --]

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);
>