[Cocci] Matching an absence of statements before the closing brace of a function?

[Cocci] Matching an absence of statements before the closing brace of a function?

[Josh Triplett]

I wanted to write a semantic patch that matched (and removed) "return;"
at the end of a void function.  I've attached the full .cocci file
written for coccicheck, but the key bit looks like this:

@@
identifier fn;
@@
void fn ( ... )
{
...
- return;
}

However, that patch also produces results like this:

--- a/drivers/scsi/aacraid/dpcsup.c
+++ b/drivers/scsi/aacraid/dpcsup.c
@@ -255,7 +255,6 @@ static void aac_aif_callback(void *conte
 	    cpu_to_le32(NoMoreAifDataAvailable)) {
 		aac_fib_complete(fibptr);
 		aac_fib_free(fibptr);
-		return;
 	}
 
 	aac_intr_normal(dev, 0, 1, 0, fibptr->hw_fib_va);

I'm guessing that either coccinelle didn't pair the braces (so that the
'}' matches close braces other than the one matching fn's opening brace)
or coccinelle allowed extra statements before the logical end of the
function despite the lack of '...'.

How can I write this patch such that it requires an absence of
statements between the return and the end of the function?

Note that matching the "return;" in the following would be fine from a
control-flow perspective:

void f(void)
{
	if (e) {
		return;
	}
}

But matching the "return;" in the following is not OK:

void f(void)
{
	if (e) {
		return;
	}
	f2();
}

- Josh Triplett
-------------- next part --------------
/// void functions don't need return statements at the end.
//
// Confidence: High
// Options: --no-includes --include-headers

virtual patch
virtual report
virtual context

@r1 depends on patch@
identifier fn;
@@
void fn ( ... )
{
...
- return;
}

@r2 depends on report || context@
identifier fn;
position p;
@@

void fn ( ... )
{
...
return;@p
}


@script:python depends on report@
p << r2.p;
fn << r2.fn;
@@

msg = "WARNING: unnecessary return@end of function '%s'" % fn
coccilib.report.print_report(p[0], msg)

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

On Sat, 2 Nov 2013, Josh Triplett wrote:

> I wanted to write a semantic patch that matched (and removed) "return;"
> at the end of a void function.  I've attached the full .cocci file
> written for coccicheck, but the key bit looks like this:
>
> @@
> identifier fn;
> @@
> void fn ( ... )
> {
> ...
> - return;
> }
>
> However, that patch also produces results like this:
>
> --- a/drivers/scsi/aacraid/dpcsup.c
> +++ b/drivers/scsi/aacraid/dpcsup.c
> @@ -255,7 +255,6 @@ static void aac_aif_callback(void *conte
>  	    cpu_to_le32(NoMoreAifDataAvailable)) {
>  		aac_fib_complete(fibptr);
>  		aac_fib_free(fibptr);
> -		return;
>  	}
>
>  	aac_intr_normal(dev, 0, 1, 0, fibptr->hw_fib_va);
>
> I'm guessing that either coccinelle didn't pair the braces (so that the
> '}' matches close braces other than the one matching fn's opening brace)
> or coccinelle allowed extra statements before the logical end of the
> function despite the lack of '...'.

The problem is that Coccinlle works on control-flow paths, and a return is
always at the end of a control-flow path...

> How can I write this patch such that it requires an absence of
> statements between the return and the end of the function?
>
> Note that matching the "return;" in the following would be fine from a
> control-flow perspective:
>
> void f(void)
> {
> 	if (e) {
> 		return;
> 	}
> }
>
> But matching the "return;" in the following is not OK:
>
> void f(void)
> {
> 	if (e) {
> 		return;
> 	}
> 	f2();
> }

Try the following:

@start@
identifier f;
position p;
@@
f(...) {@p ... }

@bad@
position p, s != start.p;
@@

{@s <... return;@p ...> }

@ef@
position p;
statement S;
@@

(
while (...) return;@p
|
if (...) return;@p else S
)

@@
position p != {bad.p,ef.p};
@@

- return;@p

julia

[Cocci] if (...) {} semantic patch

[Julia Lawall]

I think that there are more side-effecting expressions that you should
consider, eg E += E2, etc.

The following lines from the SmPL lexer shows the operators that are
supported:

  | "-="           { start_line true; mkassign Ast.Minus lexbuf }
  | "+="           { start_line true; mkassign Ast.Plus lexbuf }
  | "*="           { start_line true; mkassign Ast.Mul lexbuf }
  | "/="           { start_line true; mkassign Ast.Div lexbuf }
  | "%="           { start_line true; mkassign Ast.Mod lexbuf }
  | "&="           { start_line true; mkassign Ast.And lexbuf }
  | "|="           { start_line true; mkassign Ast.Or lexbuf }
  | "^="           { start_line true; mkassign Ast.Xor lexbuf }
  | ">?="           { start_line true; mkassign Ast.Max lexbuf }
  | "<?="           { start_line true; mkassign Ast.Min lexbuf }
  | "<<="          { start_line true; mkassign Ast.DecLeft lexbuf }
  | ">>="          { start_line true; mkassign Ast.DecRight lexbuf }

Also, if the output from -D patch is sometimes undesirable, it would be
useful to mention that in the initial comments about the semantic patch,
so that the user knows that there is something to look for.  The
convention seems to be to put such comments after //#

julia

[Cocci] Matching an absence of statements before the closing brace of a function?

[Josh Triplett]

On Sat, Nov 02, 2013 at 03:27:35PM +0100, Julia Lawall wrote:
> On Sat, 2 Nov 2013, Josh Triplett wrote:
> > I wanted to write a semantic patch that matched (and removed) "return;"
> > at the end of a void function.  I've attached the full .cocci file
> > written for coccicheck, but the key bit looks like this:
> >
> > @@
> > identifier fn;
> > @@
> > void fn ( ... )
> > {
> > ...
> > - return;
> > }
> >
> > However, that patch also produces results like this:
> >
> > --- a/drivers/scsi/aacraid/dpcsup.c
> > +++ b/drivers/scsi/aacraid/dpcsup.c
> > @@ -255,7 +255,6 @@ static void aac_aif_callback(void *conte
> >  	    cpu_to_le32(NoMoreAifDataAvailable)) {
> >  		aac_fib_complete(fibptr);
> >  		aac_fib_free(fibptr);
> > -		return;
> >  	}
> >
> >  	aac_intr_normal(dev, 0, 1, 0, fibptr->hw_fib_va);
> >
> > I'm guessing that either coccinelle didn't pair the braces (so that the
> > '}' matches close braces other than the one matching fn's opening brace)
> > or coccinelle allowed extra statements before the logical end of the
> > function despite the lack of '...'.
> 
> The problem is that Coccinlle works on control-flow paths, and a return is
> always at the end of a control-flow path...

Not sure I fully understand what you mean.  I do understand why
coccinelle works on control-flow paths rather than literal code, and
that makes it much more powerful, but I'm still not sure how coccinelle
manages to match the patch above to the code above.  Do the braces in
the semantic patch above not pair with each other?  If not, why not?  If
so, how does "return; }" match something between the "return;" and the
"}"?

> Try the following:
> 
> @start@
> identifier f;
> position p;
> @@
> f(...) {@p ... }
> 
> @bad@
> position p, s != start.p;
> @@
> 
> {@s <... return;@p ...> }
> 
> @ef@
> position p;
> statement S;
> @@
> 
> (
> while (...) return;@p
> |
> if (...) return;@p else S
> )
> 
> @@
> position p != {bad.p,ef.p};
> @@
> 
> - return;@p

Interesting; I didn't know about the ability to exclude specific
positions that way.  However, that doesn't quite work, since it still
matches code like this:

--- a/arch/arm/mach-pxa/palmtreo.c
+++ b/arch/arm/mach-pxa/palmtreo.c
@@ -502,7 +502,6 @@ void __init treo680_gpio_init(void)
 	/* driving this low turns LCD on */
 	gpio_set_value(GPIO_NR_TREO680_LCD_EN_N, 0);
 
-	return;
  fail:
 	pr_err("gpio %d initialization failed\n", gpio);
 	gpio_free(GPIO_NR_TREO680_LCD_POWER);


I tried adding a third condition to the ef rule, like this:

@ef@
position p;
statement S;
@@
(
while (...) return;@p
|
if (...) return;@p else S
|
return;@p
S
)

However, that didn't work either.  I even tried explicitly matching
labels, which seems like it should be unnecessary:

@ef@
position p;
statement S;
identifier l;
@@
(
while (...) return;@p
|
if (...) return;@p else S
|
return;@p
l: S
)

But the incorrect match above still occurred.

- Josh Triplett

[Cocci] if (...) {} semantic patch

[Josh Triplett]

On Sat, Nov 02, 2013 at 03:46:52PM +0100, Julia Lawall wrote:
> I think that there are more side-effecting expressions that you should
> consider, eg E += E2, etc.
> 
> The following lines from the SmPL lexer shows the operators that are
> supported:
> 
>   | "-="           { start_line true; mkassign Ast.Minus lexbuf }
>   | "+="           { start_line true; mkassign Ast.Plus lexbuf }
>   | "*="           { start_line true; mkassign Ast.Mul lexbuf }
>   | "/="           { start_line true; mkassign Ast.Div lexbuf }
>   | "%="           { start_line true; mkassign Ast.Mod lexbuf }
>   | "&="           { start_line true; mkassign Ast.And lexbuf }
>   | "|="           { start_line true; mkassign Ast.Or lexbuf }
>   | "^="           { start_line true; mkassign Ast.Xor lexbuf }
>   | ">?="           { start_line true; mkassign Ast.Max lexbuf }
>   | "<?="           { start_line true; mkassign Ast.Min lexbuf }
>   | "<<="          { start_line true; mkassign Ast.DecLeft lexbuf }
>   | ">>="          { start_line true; mkassign Ast.DecRight lexbuf }

Interesting.  I went by the example in
http://coccinelle.lip6.fr/rules/bugon.html , which doesn't include
those.  I had assumed that the compound assignment operators were
subject to some kind of isomorphism that made matching them unnecessary.

You might consider providing some shortcut for matching code that has
side effects.

> Also, if the output from -D patch is sometimes undesirable, it would be
> useful to mention that in the initial comments about the semantic patch,
> so that the user knows that there is something to look for.  The
> convention seems to be to put such comments after //#

Fair enough.

A couple of examples of the type of suboptimal output I meant in the
commit message:

--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -296,7 +296,8 @@ static void __init u8500_init_machine(vo
 		snowball_pinmaps_init();
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+"))
 		hrefv60_pinmaps_init();
-	else if (of_machine_is_compatible("st-ericsson,ccu9540")) {}
+	else {of_machine_is_compatible("st-ericsson,ccu9540");
+}
 		/* TODO: Add pinmaps for ccu9540 board. */
 
 	/* automatically probe child nodes of dbx5x0 devices */

Leaving aside the issue that of_machine_is_compatible has no side
effects, the formatting is clearly wrong there.

--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3232,9 +3232,7 @@ void pm8001_bytes_dmaed(struct pm8001_hb
 		id->dev_type = phy->identify.device_type;
 		id->initiator_bits = SAS_PROTOCOL_ALL;
 		id->target_bits = phy->identify.target_port_protocols;
-	} else if (phy->phy_type & PORT_TYPE_SATA) {
-		/*Nothing*/
-	}
+	} else {}
 	PM8001_MSG_DBG(pm8001_ha, pm8001_printk("phy %d byte dmaded.\n", i));
 
 	sas_phy->frame_rcvd_size = phy->frame_rcvd_size;

This one should have the else dropped entirely.

--- a/arch/mn10300/kernel/signal.c
+++ b/arch/mn10300/kernel/signal.c
@@ -413,8 +413,7 @@ static void do_signal(struct pt_regs *re
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
-		if (handle_signal(signr, &info, &ka, regs) == 0) {
-		}
+		handle_signal(signr, &info, &ka, regs) == 0;
 
 		return;
 	}

Here, the " == 0" is unnecessary.

- Josh Triplett

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

On Sat, 2 Nov 2013, Josh Triplett wrote:

> On Sat, Nov 02, 2013 at 03:27:35PM +0100, Julia Lawall wrote:
> > On Sat, 2 Nov 2013, Josh Triplett wrote:
> > > I wanted to write a semantic patch that matched (and removed) "return;"
> > > at the end of a void function.  I've attached the full .cocci file
> > > written for coccicheck, but the key bit looks like this:
> > >
> > > @@
> > > identifier fn;
> > > @@
> > > void fn ( ... )
> > > {
> > > ...
> > > - return;
> > > }
> > >
> > > However, that patch also produces results like this:
> > >
> > > --- a/drivers/scsi/aacraid/dpcsup.c
> > > +++ b/drivers/scsi/aacraid/dpcsup.c
> > > @@ -255,7 +255,6 @@ static void aac_aif_callback(void *conte
> > >  	    cpu_to_le32(NoMoreAifDataAvailable)) {
> > >  		aac_fib_complete(fibptr);
> > >  		aac_fib_free(fibptr);
> > > -		return;
> > >  	}
> > >
> > >  	aac_intr_normal(dev, 0, 1, 0, fibptr->hw_fib_va);
> > >
> > > I'm guessing that either coccinelle didn't pair the braces (so that the
> > > '}' matches close braces other than the one matching fn's opening brace)
> > > or coccinelle allowed extra statements before the logical end of the
> > > function despite the lack of '...'.
> >
> > The problem is that Coccinlle works on control-flow paths, and a return is
> > always at the end of a control-flow path...
>
> Not sure I fully understand what you mean.  I do understand why
> coccinelle works on control-flow paths rather than literal code, and
> that makes it much more powerful, but I'm still not sure how coccinelle
> manages to match the patch above to the code above.  Do the braces in
> the semantic patch above not pair with each other?  If not, why not?  If
> so, how does "return; }" match something between the "return;" and the
> "}"?

The return is followed by all of the nested }s to get it out of the
function.  I think that there is something special for these }s to skip
over them, so that the return always seems to be next to the final }.

> > Try the following:
> >
> > @start@
> > identifier f;
> > position p;
> > @@
> > f(...) {@p ... }
> >
> > @bad@
> > position p, s != start.p;
> > @@
> >
> > {@s <... return;@p ...> }
> >
> > @ef@
> > position p;
> > statement S;
> > @@
> >
> > (
> > while (...) return;@p
> > |
> > if (...) return;@p else S
> > )
> >
> > @@
> > position p != {bad.p,ef.p};
> > @@
> >
> > - return;@p
>
> Interesting; I didn't know about the ability to exclude specific
> positions that way.  However, that doesn't quite work, since it still
> matches code like this:

OK, good point.  I will try to find something else.

return;
S

doesn't work, because no statement follows a return.  A return is just
followed by a sequence of closing braces.

You can try this:

@@
@@

- return;
+ RETURN;

@r exists@
identifier f;
position p;
@@

f(...) {
  ...
  RETURN at p;
}

@@
position r.p;
@@

- RETURN at p;

@@
@@

- RETURN;
+ return;

[Cocci] if (...) {} semantic patch

[Julia Lawall]

> --- a/arch/arm/mach-ux500/cpu-db8500.c
> +++ b/arch/arm/mach-ux500/cpu-db8500.c
> @@ -296,7 +296,8 @@ static void __init u8500_init_machine(vo
>  		snowball_pinmaps_init();
>  	} else if (of_machine_is_compatible("st-ericsson,hrefv60+"))
>  		hrefv60_pinmaps_init();
> -	else if (of_machine_is_compatible("st-ericsson,ccu9540")) {}
> +	else {of_machine_is_compatible("st-ericsson,ccu9540");
> +}

I was initially mystified as to how the call to of_machine_is_compatible
got put into the {}, but that is not what happened.  It is worried that
the call to of_machine_is_compatible represents more than one statement,
and it is trying to protect the new else, by wrapping a {} around it.

I've been working on the management of these {}s, and I would have thought
that the indentation would come out better, so this is a useful example
for me :)

> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3232,9 +3232,7 @@ void pm8001_bytes_dmaed(struct pm8001_hb
>  		id->dev_type = phy->identify.device_type;
>  		id->initiator_bits = SAS_PROTOCOL_ALL;
>  		id->target_bits = phy->identify.target_port_protocols;
> -	} else if (phy->phy_type & PORT_TYPE_SATA) {
> -		/*Nothing*/
> -	}
> +	} else {}

This the semantic patch would have to do explicitly.  The specified
transformation only worked on the if, not the else in the context.

>  	PM8001_MSG_DBG(pm8001_ha, pm8001_printk("phy %d byte dmaded.\n", i));
>
>  	sas_phy->frame_rcvd_size = phy->frame_rcvd_size;
>
> This one should have the else dropped entirely.
>
> --- a/arch/mn10300/kernel/signal.c
> +++ b/arch/mn10300/kernel/signal.c
> @@ -413,8 +413,7 @@ static void do_signal(struct pt_regs *re
>
>  	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
>  	if (signr > 0) {
> -		if (handle_signal(signr, &info, &ka, regs) == 0) {
> -		}
> +		handle_signal(signr, &info, &ka, regs) == 0;
>
>  		return;
>  	}
>
> Here, the " == 0" is unnecessary.

Likewise, here the semantic patch has to do something explicitly.  It
could have some special cases for common cases like == 0.

But anyway, I think that supporting the patch option is not a good idea in
this case.

julia

[Cocci] Matching an absence of statements before the closing brace of a function?

[Josh Triplett]

On Sat, Nov 02, 2013 at 04:02:28PM +0100, Julia Lawall wrote:
> On Sat, 2 Nov 2013, Josh Triplett wrote:
> > On Sat, Nov 02, 2013 at 03:27:35PM +0100, Julia Lawall wrote:
> > > On Sat, 2 Nov 2013, Josh Triplett wrote:
> > > > I wanted to write a semantic patch that matched (and removed) "return;"
> > > > at the end of a void function.  I've attached the full .cocci file
> > > > written for coccicheck, but the key bit looks like this:
> > > >
> > > > @@
> > > > identifier fn;
> > > > @@
> > > > void fn ( ... )
> > > > {
> > > > ...
> > > > - return;
> > > > }
> > > >
> > > > However, that patch also produces results like this:
> > > >
> > > > --- a/drivers/scsi/aacraid/dpcsup.c
> > > > +++ b/drivers/scsi/aacraid/dpcsup.c
> > > > @@ -255,7 +255,6 @@ static void aac_aif_callback(void *conte
> > > >  	    cpu_to_le32(NoMoreAifDataAvailable)) {
> > > >  		aac_fib_complete(fibptr);
> > > >  		aac_fib_free(fibptr);
> > > > -		return;
> > > >  	}
> > > >
> > > >  	aac_intr_normal(dev, 0, 1, 0, fibptr->hw_fib_va);
> > > >
> > > > I'm guessing that either coccinelle didn't pair the braces (so that the
> > > > '}' matches close braces other than the one matching fn's opening brace)
> > > > or coccinelle allowed extra statements before the logical end of the
> > > > function despite the lack of '...'.
> > >
> > > The problem is that Coccinlle works on control-flow paths, and a return is
> > > always at the end of a control-flow path...
> >
> > Not sure I fully understand what you mean.  I do understand why
> > coccinelle works on control-flow paths rather than literal code, and
> > that makes it much more powerful, but I'm still not sure how coccinelle
> > manages to match the patch above to the code above.  Do the braces in
> > the semantic patch above not pair with each other?  If not, why not?  If
> > so, how does "return; }" match something between the "return;" and the
> > "}"?
> 
> The return is followed by all of the nested }s to get it out of the
> function.  I think that there is something special for these }s to skip
> over them, so that the return always seems to be next to the final }.

Huh.  Odd.

> > > Try the following:
> > >
> > > @start@
> > > identifier f;
> > > position p;
> > > @@
> > > f(...) {@p ... }
> > >
> > > @bad@
> > > position p, s != start.p;
> > > @@
> > >
> > > {@s <... return;@p ...> }
> > >
> > > @ef@
> > > position p;
> > > statement S;
> > > @@
> > >
> > > (
> > > while (...) return;@p
> > > |
> > > if (...) return;@p else S
> > > )
> > >
> > > @@
> > > position p != {bad.p,ef.p};
> > > @@
> > >
> > > - return;@p
> >
> > Interesting; I didn't know about the ability to exclude specific
> > positions that way.  However, that doesn't quite work, since it still
> > matches code like this:
> 
> OK, good point.  I will try to find something else.
> 
> return;
> S
> 
> doesn't work, because no statement follows a return.  A return is just
> followed by a sequence of closing braces.

In the control-flow sense, sure.  However, in addition to making this
particular case difficult, that also makes it difficult to use
coccinelle to detect dead code after a return.

> You can try this:
> 
> @@
> @@
> 
> - return;
> + RETURN;
> 
> @r exists@
> identifier f;
> position p;
> @@
> 
> f(...) {
>   ...
>   RETURN at p;
> }
> 
> @@
> position r.p;
> @@
> 
> - RETURN at p;
> 
> @@
> @@
> 
> - RETURN;
> + return;

Cute.  I'm not sure why you need the two separate rules with matching
positions; consolidating the two middle rules into one seems to work as
well.  I did have to add a rule for when the return immediately followed
a label, to make sure there was a statement to target.

However, this seems to have two issues.  First, to some extent I
*wanted* the control-flow matching, to handle cases like this without
having to do so explicitly:

void f(void)
{
	if (...) {
		foo();
-		return;
	}
}

Second, this seems to lead to many formatting issues:

--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -895,7 +895,7 @@ static void print_other_cpu_stall(struct
 
 	rcu_print_detail_task_stall(rsp);
 
-	force_quiescent_state(rsp);  /* Kick them all. */
+	force_quiescent_state(rsp);
 }
 
 static void print_cpu_stall(struct rcu_state *rsp)

No idea why coccinelle touched this comment.  Do functions have an
implicit "return;" at the end that got patched here?

--- a/lib/xz/xz_crc32.c
+++ b/lib/xz/xz_crc32.c
@@ -43,7 +43,7 @@ XZ_EXTERN void xz_crc32_init(void)
 		xz_crc32_table[i] = r;
 	}
 
-	return;
+	RETURN;
 }
 
 XZ_EXTERN uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)

No idea what happened here to prevent the RETURN; from getting
substituted back.


Also, I noticed some cases where the return; in question immediately
followed a label, so removing it would give that label no statement to
jump to.  I tried handling that explicitly:

@r1 depends on patch exists@
identifier f, l;
statement S;
position p;
@@
void f(...)
{
...
(
l:
- RETURN;@p
  S
|
l:
- RETURN at p
  ;
|
- RETURN;@p
)
}

However, while this did produce the desired result when the return
immeidately followed the label, it produced odd results in other cases:

--- a/crypto/seqiv.c
+++ b/crypto/seqiv.c
@@ -45,6 +45,7 @@ static void seqiv_complete2(struct skcip
 
 out:
 	kfree(subreq->info);
+	return;
 }
 
 static void seqiv_complete(struct crypto_async_request *base, int err)

Many instances like this where coccinelle added "return;" where none
existed before.

- Josh Triplett

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

> > @r exists@
> > identifier f;
> > position p;
> > @@
> >
> > f(...) {
> >   ...
> >   RETURN at p;
> > }
> >
> > @@
> > position r.p;
> > @@
> >
> > - RETURN at p;
> >
> > @@
> > @@
> >
> > - RETURN;
> > + return;
>
> Cute.  I'm not sure why you need the two separate rules with matching
> positions; consolidating the two middle rules into one seems to work as
> well.  I did have to add a rule for when the return immediately followed
> a label, to make sure there was a statement to target.

I was worried that putting the rules together would cause an already
tagged token error, when there are several paths to the return, via gotos.

> However, this seems to have two issues.  First, to some extent I
> *wanted* the control-flow matching, to handle cases like this without
> having to do so explicitly:
>
> void f(void)
> {
> 	if (...) {
> 		foo();
> -		return;
> 	}
> }

This return is clearly not just before the ending brace...
You could have a separate rule for this case, but then there is still the
case where there are two nested ifs, etc.

> Second, this seems to lead to many formatting issues:
>
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -895,7 +895,7 @@ static void print_other_cpu_stall(struct
>
>  	rcu_print_detail_task_stall(rsp);
>
> -	force_quiescent_state(rsp);  /* Kick them all. */
> +	force_quiescent_state(rsp);
>  }
>
>  static void print_cpu_stall(struct rcu_state *rsp)
>
> No idea why coccinelle touched this comment.  Do functions have an
> implicit "return;" at the end that got patched here?

Yes, functions have an implicit return.  I was hoping that it wouldn't get
matched if the return was not written inside braces, but that doesn't seem
to have been successful...

> --- a/lib/xz/xz_crc32.c
> +++ b/lib/xz/xz_crc32.c
> @@ -43,7 +43,7 @@ XZ_EXTERN void xz_crc32_init(void)
>  		xz_crc32_table[i] = r;
>  	}
>
> -	return;
> +	RETURN;
>  }
>
>  XZ_EXTERN uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)
>
> No idea what happened here to prevent the RETURN; from getting
> substituted back.

Maybe a parsing problem?  I will look into it.

> Also, I noticed some cases where the return; in question immediately
> followed a label, so removing it would give that label no statement to
> jump to.  I tried handling that explicitly:
>
> @r1 depends on patch exists@
> identifier f, l;
> statement S;
> position p;
> @@
> void f(...)
> {
> ...
> (
> l:
> - RETURN;@p
>   S
> |
> l:
> - RETURN at p
>   ;
> |
> - RETURN;@p
> )
> }
>
> However, while this did produce the desired result when the return
> immeidately followed the label, it produced odd results in other cases:
>
> --- a/crypto/seqiv.c
> +++ b/crypto/seqiv.c
> @@ -45,6 +45,7 @@ static void seqiv_complete2(struct skcip
>
>  out:
>  	kfree(subreq->info);
> +	return;
>  }
>
>  static void seqiv_complete(struct crypto_async_request *base, int err)
>
> Many instances like this where coccinelle added "return;" where none
> existed before.

I don't think this has to do with the label issue, but with the implicit
return issue.

julia

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

In the case where there is:

l: return;

at the end of a function, are you sure that you really want to replace the
return by ;?  I'm not sure that it is really an improvement...

One could replace jumps to such labels by direct returned but perhaps the
goto was added because there might be a need for more interesting code at
the end of the function in the future.

In any case, here is my current suggestion, which does convert l: return;
to l: ;

x@
position p;
@@

return at p;

@@
position x.p;
@@

- return at p;
+ RETURN;

@exists@
identifier f;
identifier l;
@@

f(...) {
  ...
l:
- RETURN
  ;
}

@exists@
identifier f;
@@

f(...) {
  ...
- RETURN;
}

@@
@@

- RETURN;
+ return;

julia

[Cocci] Matching an absence of statements before the closing brace of a function?

[Josh Triplett]

On Sat, Nov 02, 2013 at 05:02:40PM +0100, Julia Lawall wrote:
> > > @r exists@
> > > identifier f;
> > > position p;
> > > @@
> > >
> > > f(...) {
> > >   ...
> > >   RETURN at p;
> > > }
> > >
> > > @@
> > > position r.p;
> > > @@
> > >
> > > - RETURN at p;
> > >
> > > @@
> > > @@
> > >
> > > - RETURN;
> > > + return;
> >
> > Cute.  I'm not sure why you need the two separate rules with matching
> > positions; consolidating the two middle rules into one seems to work as
> > well.  I did have to add a rule for when the return immediately followed
> > a label, to make sure there was a statement to target.
> 
> I was worried that putting the rules together would cause an already
> tagged token error, when there are several paths to the return, via gotos.
> 
> > However, this seems to have two issues.  First, to some extent I
> > *wanted* the control-flow matching, to handle cases like this without
> > having to do so explicitly:
> >
> > void f(void)
> > {
> > 	if (...) {
> > 		foo();
> > -		return;
> > 	}
> > }
> 
> This return is clearly not just before the ending brace...

No, but it's semantically at the end of the function.

> You could have a separate rule for this case, but then there is still the
> case where there are two nested ifs, etc.

Right, hence why I'd really like coccinelle's control-flow handling to
handle the case above.  I do want to match "return;" at the logical end
of the function, in a control-flow sense, *without* taking into account
that the "return;" itself is the logical end of the function in a
control-flow sense, because I can't remove it unless the point it
appears at would already be the logical end of the function.

> > Second, this seems to lead to many formatting issues:
> >
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -895,7 +895,7 @@ static void print_other_cpu_stall(struct
> >
> >  	rcu_print_detail_task_stall(rsp);
> >
> > -	force_quiescent_state(rsp);  /* Kick them all. */
> > +	force_quiescent_state(rsp);
> >  }
> >
> >  static void print_cpu_stall(struct rcu_state *rsp)
> >
> > No idea why coccinelle touched this comment.  Do functions have an
> > implicit "return;" at the end that got patched here?
> 
> Yes, functions have an implicit return.  I was hoping that it wouldn't get
> matched if the return was not written inside braces, but that doesn't seem
> to have been successful...

I can see why it would make sense to have an implicit return, but yeah.

> > --- a/lib/xz/xz_crc32.c
> > +++ b/lib/xz/xz_crc32.c
> > @@ -43,7 +43,7 @@ XZ_EXTERN void xz_crc32_init(void)
> >  		xz_crc32_table[i] = r;
> >  	}
> >
> > -	return;
> > +	RETURN;
> >  }
> >
> >  XZ_EXTERN uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)
> >
> > No idea what happened here to prevent the RETURN; from getting
> > substituted back.
> 
> Maybe a parsing problem?  I will look into it.

Thanks!

> > Also, I noticed some cases where the return; in question immediately
> > followed a label, so removing it would give that label no statement to
> > jump to.  I tried handling that explicitly:
> >
> > @r1 depends on patch exists@
> > identifier f, l;
> > statement S;
> > position p;
> > @@
> > void f(...)
> > {
> > ...
> > (
> > l:
> > - RETURN;@p
> >   S
> > |
> > l:
> > - RETURN at p
> >   ;
> > |
> > - RETURN;@p
> > )
> > }
> >
> > However, while this did produce the desired result when the return
> > immeidately followed the label, it produced odd results in other cases:
> >
> > --- a/crypto/seqiv.c
> > +++ b/crypto/seqiv.c
> > @@ -45,6 +45,7 @@ static void seqiv_complete2(struct skcip
> >
> >  out:
> >  	kfree(subreq->info);
> > +	return;
> >  }
> >
> >  static void seqiv_complete(struct crypto_async_request *base, int err)
> >
> > Many instances like this where coccinelle added "return;" where none
> > existed before.
> 
> I don't think this has to do with the label issue, but with the implicit
> return issue.

It only occurred when I changed the patch to the version that handled
labels, though.

- Josh Triplett

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

> > You could have a separate rule for this case, but then there is still the
> > case where there are two nested ifs, etc.
>
> Right, hence why I'd really like coccinelle's control-flow handling to
> handle the case above.  I do want to match "return;" at the logical end
> of the function, in a control-flow sense, *without* taking into account
> that the "return;" itself is the logical end of the function in a
> control-flow sense, because I can't remove it unless the point it
> appears at would already be the logical end of the function.

I don't see a solution for this case...

julia

[Cocci] Matching an absence of statements before the closing brace of a function?

[Julia Lawall]

> --- a/lib/xz/xz_crc32.c
> +++ b/lib/xz/xz_crc32.c
> @@ -43,7 +43,7 @@ XZ_EXTERN void xz_crc32_init(void)
>  		xz_crc32_table[i] = r;
>  	}
>  
> -	return;
> +	RETURN;
>  }
>  
>  XZ_EXTERN uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)

This is indeed a parsing problem.  When it has the whole file, when it
comes to the definition of xz_crc32 it is able to detect that XZ_EXTERN is
something that it should ignore.  But after it makes the transformation in
xz_crc32_init, it reparses just that function and does not obtain this
information.

Perhaps it is possible to fix.  But I have the impression that I undid
something in this regard, because it is not always desirable to use the
macro information inferred from one part of a file in parsing things
elsewhere in the file, so fixing this problem may break something else...

julia