On Sat, Nov 02, 2013 at 05:02:40PM +0100, Julia Lawall wrote:
> > > @r exists@
> > > identifier f;
> > > position p;
> > > @@
> > >
> > > f(...) {
> > > ...
> > > RETURN@p;
> > > }
> > >
> > > @@
> > > position r.p;
> > > @@
> > >
> > > - RETURN@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@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 mailing list
[email protected]
https://systeme.lip6.fr/mailman/listinfo/cocci
