[Cocci] cocci: char* indexing style for linux

[Cocci] cocci: char* indexing style for linux

[Joe Perches]

Hi.

I tried this:

$ spatch --version
spatch version 1.0.0-rc21 without Python support and with PCRE support

$ cat char_index.cocci
@@
unsigned char * foo;
expression e;
@@

-	*(foo + e)
+	foo[e]

@@
char * foo;
expression e;
@@

-	*(foo + e)
+	foo[e]

$

with

$ spatch --recursive-includes -sp-file char_index.cocci drivers/net/ethernet/intel/e1000/e1000_ethtool.c

and got (after a very long time)

diff = 
--- drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ /tmp/cocci-output-31725-95778b-e1000_ethtool.c
@@ -1395,9 +1395,9 @@ static int e1000_check_lbtest_frame(stru
 				    unsigned int frame_size)
 {
 	frame_size &= ~1;
-	if (*(skb->data + 3) == 0xFF) {
-		if ((*(skb->data + frame_size / 2 + 10) == 0xBE) &&
-		   (*(skb->data + frame_size / 2 + 12) == 0xAF)) {
+	if (skb->data[3] == 0xFF) {
+		if ((skb->data + frame_size / 2[10] == 0xBE) &&
+		   (skb->data + frame_size / 2[12] == 0xAF)) {
 			return 0;
 		}
 	}
Note: processing took   51.2s: drivers/net/ethernet/intel/e1000/e1000_ethtool.c
$

(originally I used spatch version 1.0.0-rc14,
 it took > 300 seconds, so that's improvement)

The suggested conversion to "sk->data + frame_size / 2[10]"
is not correct.

Can I make the char * foo test match the shortest
match or the statement s match the longest match
somehow?

I tried it with --disable-multi-pass and got the same
suggestion.

Is there another magic incantation?

cheers, Joe

[Cocci] cocci: char* indexing style for linux

[Joe Perches]

On Sat, 2014-08-16 at 13:06 -0700, Joe Perches wrote:
> The suggested conversion to "sk->data + frame_size / 2[10]"
> is not correct.

Actually, that's rc14's uncompilable output.
I started the email with 14 and upgraded before sending.
rc21 does suggest a compilable output.
I scanned it visually and didn't notice.

rc21 suggests:

(skb->data + frame_size / 2)[10]

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

On Sat, 16 Aug 2014, Joe Perches wrote:

> On Sat, 2014-08-16 at 13:06 -0700, Joe Perches wrote:
> > The suggested conversion to "sk->data + frame_size / 2[10]"
> > is not correct.
>
> Actually, that's rc14's uncompilable output.
> I started the email with 14 and upgraded before sending.
> rc21 does suggest a compilable output.
> I scanned it visually and didn't notice.
>
> rc21 suggests:
>
> (skb->data + frame_size / 2)[10]

So everything is fine now?  There was indeed an improvement with respect
to precendence of operators along the way.

julia

[Cocci] cocci: char* indexing style for linux

[Joe Perches]

On Sun, 2014-08-17 at 05:11 -0500, Julia Lawall wrote:
> 
> On Sat, 16 Aug 2014, Joe Perches wrote:
> 
> > On Sat, 2014-08-16 at 13:06 -0700, Joe Perches wrote:
> > > The suggested conversion to "sk->data + frame_size / 2[10]"
> > > is not correct.
> >
> > Actually, th[at's rc14's uncompilable output.
> > I started the email with 14 and upgraded before sending.
> > rc21 does suggest a compilable output.
> > I scanned it visually and didn't notice.
> >
> > rc21 suggests:
> >
> > (skb->data + frame_size / 2)[10]
> 
> So everything is fine now?  There was indeed an improvement with respect
> to precendence of operators along the way.

It's compilable, but not the desired conversion.

I would prefer to be able to convert
	*(skb->data + frame_size / 2 + 10)
not to
	(skb->data + frame_size / 2)[10]
but to
	skb->data[frame_size / 2 + 10]

but I don't know how.

[Cocci] cocci: char* indexing style for linux

[SF Markus Elfring]

> I would prefer to be able to convert
> 	*(skb->data + frame_size / 2 + 10)
> not to
> 	(skb->data + frame_size / 2)[10]
> but to
> 	skb->data[frame_size / 2 + 10]
>
> but I don't know how.

How do you think about a bit more fine-tuning for the desired semantic
patch?

Can it be that the pattern "*(foo + e)" is too generic for any source
code place?
Would you like to try out the specification of a few more elements
instead of a single expression here?

Regards,
Markus

[Cocci] cocci: char* indexing style for linux

[Joe Perches]

On Sun, 2014-08-17 at 18:09 +0200, SF Markus Elfring wrote:
> > I would prefer to be able to convert
> > 	*(skb->data + frame_size / 2 + 10)
> > not to
> > 	(skb->data + frame_size / 2)[10]
> > but to
> > 	skb->data[frame_size / 2 + 10]
> >
> > but I don't know how.
> 
> How do you think about a bit more fine-tuning for the desired semantic
> patch?

Not sure I understand what you are requesting.

> Can it be that the pattern "*(foo + e)" is too generic for any source
> code place?

I don't think so as any type or combination of
arithmetic operations would then need to be specified
by the spatch input script.

> Would you like to try out the specification of a few more elements
> instead of a single expression here?

Not really.

I think a precedence specification capability like first
pointer match followed by longest arithmetic match would
perhaps be a better generic facility.

[Cocci] Conversion of some pointer arithmetic to array indexing style

[SF Markus Elfring]

> I don't think so as any type or combination of
> arithmetic operations would then need to be specified
> by the spatch input script.

I imagine that it will be occasionally useful to be more specific in
such semantic patterns.

@pointer_arithmetic@
identifier var, member, add;
int num1, num2;
@@
- *(var->member + add / num1 + num2)
+ var->member[add / num1 + num2]


> I think a precedence specification capability like first
> pointer match followed by longest arithmetic match would
> perhaps be a better generic facility.
>

I find it also nice when a semantic pattern could be written in a very
generic way. But I see that there are some technical challenges so that
false positives can be excluded.

Regards,
Markus

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

On Sun, 17 Aug 2014, Joe Perches wrote:

> On Sun, 2014-08-17 at 18:09 +0200, SF Markus Elfring wrote:
> > > I would prefer to be able to convert
> > > 	*(skb->data + frame_size / 2 + 10)
> > > not to
> > > 	(skb->data + frame_size / 2)[10]
> > > but to
> > > 	skb->data[frame_size / 2 + 10]
> > >
> > > but I don't know how.
> >
> > How do you think about a bit more fine-tuning for the desired semantic
> > patch?
>
> Not sure I understand what you are requesting.
>
> > Can it be that the pattern "*(foo + e)" is too generic for any source
> > code place?
>
> I don't think so as any type or combination of
> arithmetic operations would then need to be specified
> by the spatch input script.
>
> > Would you like to try out the specification of a few more elements
> > instead of a single expression here?
>
> Not really.
>
> I think a precedence specification capability like first
> pointer match followed by longest arithmetic match would
> perhaps be a better generic facility.

Isn't the problem already solved? If not, I will look into it.

julia

[Cocci] Conversion of some pointer arithmetic to array indexing style

[Julia Lawall]

On Sun, 17 Aug 2014, SF Markus Elfring wrote:

> > I don't think so as any type or combination of
> > arithmetic operations would then need to be specified
> > by the spatch input script.
>
> I imagine that it will be occasionally useful to be more specific in
> such semantic patterns.
>
> @pointer_arithmetic@
> identifier var, member, add;
> int num1, num2;
> @@
> - *(var->member + add / num1 + num2)
> + var->member[add / num1 + num2]

I don't see the need to be specific about the division, but it is true
that the associativity of + is annoying: whatever it is, there are always
cases ehere it is not what you want,  You can match things like this with

*(var->member + ...)

but there is no way to pick up on an expression that matches the ..., so
you have tomake special cases.  ON the other hand, you could see what
special cases are needed for your software with the above pattern.

julia

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

Here is a possibility.  It is susceptible to "already tagged token" errors
when there are two dereferences in the sum.  Assuming that at most one of
them has a pointer type, it should be easy to fix.  I can do that toorrow.
But doing that would require type information, which would require type
information, so it oculd makesense to do what is wanted with this more
efficient rule first.

julia

@@
expression e,x;
identifier f;
@@

+ e->f[
- *(
   <+...
(
-  (e->f + x)
+  x
|
+  x
-  (x + e->f)
)
   ...+>
-  )
+  ]

[Cocci] Conversion of some pointer arithmetic to array indexing style

[SF Markus Elfring]

> @@
> expression e,x;
> identifier f;
> @@
> 
> + e->f[
> - *(
>    <+...
> (
> -  (e->f + x)
> +  x
> |
> +  x
> -  (x + e->f)
> )
>    ...+>
> -  )
> +  ]

Does the issue "isomorphism" matter in the shown addition?

Regards,
Markus

[Cocci] cocci: char* indexing style for linux

[Lars-Peter Clausen]

On 08/17/2014 07:42 PM, Julia Lawall wrote:
>
>
> On Sun, 17 Aug 2014, Joe Perches wrote:
>
>> On Sun, 2014-08-17 at 18:09 +0200, SF Markus Elfring wrote:
>>>> I would prefer to be able to convert
>>>> 	*(skb->data + frame_size / 2 + 10)
>>>> not to
>>>> 	(skb->data + frame_size / 2)[10]
>>>> but to
>>>> 	skb->data[frame_size / 2 + 10]
>>>>
>>>> but I don't know how.
>>>
>>> How do you think about a bit more fine-tuning for the desired semantic
>>> patch?
>>
>> Not sure I understand what you are requesting.
>>
>>> Can it be that the pattern "*(foo + e)" is too generic for any source
>>> code place?
>>
>> I don't think so as any type or combination of
>> arithmetic operations would then need to be specified
>> by the spatch input script.
>>
>>> Would you like to try out the specification of a few more elements
>>> instead of a single expression here?
>>
>> Not really.
>>
>> I think a precedence specification capability like first
>> pointer match followed by longest arithmetic match would
>> perhaps be a better generic facility.
>
> Isn't the problem already solved? If not, I will look into it.

I think for these kinds of problems it would be helpful to have a 
metavariable type that is somewhere in the middle of idexpression and 
expression. Something that matches an identifier or accessing a field in an 
identifier, or a field of a field of an identifier and so on.

In the past I had cocci scripts where I ended up doing

(
id->fld1
|
id->fld1.fld2
|
id->fld1.fld2.fld3
...
)

to kind of make this work.

- Lars

[Cocci] Conversion of some pointer arithmetic to array indexing style

[Julia Lawall]

On Mon, 18 Aug 2014, SF Markus Elfring wrote:

> > @@
> > expression e,x;
> > identifier f;
> > @@
> >
> > + e->f[
> > - *(
> >    <+...
> > (
> > -  (e->f + x)
> > +  x
> > |
> > +  x
> > -  (x + e->f)
> > )
> >    ...+>
> > -  )
> > +  ]
>
> Does the issue "isomorphism" matter in the shown addition?

I don'tknw if there is one for the commutativity of addition.  If there is
then the disjunction is not needed.

"matter" implies that something could go wrong.  Nothing could go wrong,
whether the rule is redundnt or not.

julia

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

On Mon, 18 Aug 2014, Lars-Peter Clausen wrote:

> On 08/17/2014 07:42 PM, Julia Lawall wrote:
> >
> >
> > On Sun, 17 Aug 2014, Joe Perches wrote:
> >
> > > On Sun, 2014-08-17 at 18:09 +0200, SF Markus Elfring wrote:
> > > > > I would prefer to be able to convert
> > > > > 	*(skb->data + frame_size / 2 + 10)
> > > > > not to
> > > > > 	(skb->data + frame_size / 2)[10]
> > > > > but to
> > > > > 	skb->data[frame_size / 2 + 10]
> > > > >
> > > > > but I don't know how.
> > > >
> > > > How do you think about a bit more fine-tuning for the desired semantic
> > > > patch?
> > >
> > > Not sure I understand what you are requesting.
> > >
> > > > Can it be that the pattern "*(foo + e)" is too generic for any source
> > > > code place?
> > >
> > > I don't think so as any type or combination of
> > > arithmetic operations would then need to be specified
> > > by the spatch input script.
> > >
> > > > Would you like to try out the specification of a few more elements
> > > > instead of a single expression here?
> > >
> > > Not really.
> > >
> > > I think a precedence specification capability like first
> > > pointer match followed by longest arithmetic match would
> > > perhaps be a better generic facility.
> >
> > Isn't the problem already solved? If not, I will look into it.
>
> I think for these kinds of problems it would be helpful to have a metavariable
> type that is somewhere in the middle of idexpression and expression. Something
> that matches an identifier or accessing a field in an identifier, or a field
> of a field of an identifier and so on.
>
> In the past I had cocci scripts where I ended up doing
>
> (
> id->fld1
> |
> id->fld1.fld2
> |
> id->fld1.fld2.fld3
> ...
> )
>
> to kind of make this work.

It's probably true,  But I don't know what one would call it, and I think
that if one had a name for it, no one would understand it...

julia

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

Actually, isn't the whoe structure field the wrong way to go about it
here?  Don't we just want a pointer-typed expression, of whatever form it
has?  That will unfortunately require lots of type information, but I
don't see how to do it otherewise, because any kind of variable or
structure field reference can also be an integer.  There isno reason why
eg the structure field reference is the array.

expression *e; matches a pointer.

julia

[Cocci] cocci: char* indexing style for linux

[Lars-Peter Clausen]

On 08/18/2014 11:49 AM, Julia Lawall wrote:
> Actually, isn't the whoe structure field the wrong way to go about it
> here?  Don't we just want a pointer-typed expression, of whatever form it
> has?  That will unfortunately require lots of type information, but I
> don't see how to do it otherewise, because any kind of variable or
> structure field reference can also be an integer.  There isno reason why
> eg the structure field reference is the array.
>
> expression *e; matches a pointer.

The problem is that (foo->bar + 10) with foo->bar being a pointer is still a 
expression of the pointer type and so e will match the whole expression 
rather than just matching foo->bar.

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

On Mon, 18 Aug 2014, Lars-Peter Clausen wrote:

> On 08/18/2014 11:49 AM, Julia Lawall wrote:
> > Actually, isn't the whoe structure field the wrong way to go about it
> > here?  Don't we just want a pointer-typed expression, of whatever form it
> > has?  That will unfortunately require lots of type information, but I
> > don't see how to do it otherewise, because any kind of variable or
> > structure field reference can also be an integer.  There isno reason why
> > eg the structure field reference is the array.
> >
> > expression *e; matches a pointer.
>
> The problem is that (foo->bar + 10) with foo->bar being a pointer is still a
> expression of the pointer type and so e will match the whole expression rather
> than just matching foo->bar.

OK, that's true.

How about then

(
e1 . at e2 f + x
|
e1 ->@e2 f + x
)

where e2 has pointer typw (or array type?)

e2 on the . or -> will pick up the entire innermost enclosing expression.

julia

[Cocci] cocci: char* indexing style for linux

[Lars-Peter Clausen]

On 08/18/2014 11:49 AM, Julia Lawall wrote:
> Actually, isn't the whoe structure field the wrong way to go about it
> here?  Don't we just want a pointer-typed expression, of whatever form it
> has?  That will unfortunately require lots of type information, but I
> don't see how to do it otherewise, because any kind of variable or
> structure field reference can also be an integer.  There isno reason why
> eg the structure field reference is the array.
>
> expression *e; matches a pointer.
>
> julia
>

The problem is that foo->bar + 10 is still a pointer expression of the same 
type as bar, so e will match the whole ting and not just foo->bar as intended.

- Lars

[Cocci] Conversion of some pointer arithmetic to array indexing style

[SF Markus Elfring]

>> Does the issue "isomorphism" matter in the shown addition? 
> I don'tknw if there is one for the commutativity of addition.

I try to clarify this implementation detail a bit more.


> If there is then the disjunction is not needed.

I wondered if such a repetition can be avoided in the semantic patch.

Regards,
Markus

[Cocci] Clarification for array indexing style

[SF Markus Elfring]

> How about then
>
> (
> e1 . at e2 f + x
> |
> e1 ->@e2 f + x
> )
>
> where e2 has pointer typw (or array type?)
>
> e2 on the . or -> will pick up the entire innermost enclosing expression.
>

Would such a semantic patch approach handle also longer reference chains?

Regards,
Markus

[Cocci] Clarification for array indexing style

[Julia Lawall]

On Mon, 18 Aug 2014, SF Markus Elfring wrote:

> > How about then
> >
> > (
> > e1 . at e2 f + x
> > |
> > e1 ->@e2 f + x
> > )
> >
> > where e2 has pointer typw (or array type?)
> >
> > e2 on the . or -> will pick up the entire innermost enclosing expression.
> >
>
> Would such a semantic patch approach handle also longer reference chains?

Yes, I am assuming that e1 has been declared to be an arbitrary
expression.

julia

[Cocci] cocci: char* indexing style for linux

[SF Markus Elfring]

>> I think for these kinds of problems it would be helpful to have a metavariable
>> type that is somewhere in the middle of idexpression and expression. Something
>> that matches an identifier or accessing a field in an identifier, or a field
>> of a field of an identifier and so on.
>>
>> In the past I had cocci scripts where I ended up doing
>>
>> (
>> id->fld1
>> |
>> id->fld1.fld2
>> |
>> id->fld1.fld2.fld3
>> ...
>> )
>>
>> to kind of make this work.
> It's probably true,  But I don't know what one would call it, and I think
> that if one had a name for it, no one would understand it...

Do you dare to suggest another type name for a metavariable in the
semantic patch language?

Regards,
Markus

[Cocci] cocci: char* indexing style for linux

[Julia Lawall]

On Mon, 18 Aug 2014, SF Markus Elfring wrote:

>
> >> I think for these kinds of problems it would be helpful to have a metavariable
> >> type that is somewhere in the middle of idexpression and expression. Something
> >> that matches an identifier or accessing a field in an identifier, or a field
> >> of a field of an identifier and so on.
> >>
> >> In the past I had cocci scripts where I ended up doing
> >>
> >> (
> >> id->fld1
> >> |
> >> id->fld1.fld2
> >> |
> >> id->fld1.fld2.fld3
> >> ...
> >> )
> >>
> >> to kind of make this work.
> > It's probably true,  But I don't know what one would call it, and I think
> > that if one had a name for it, no one would understand it...
>
> Do you dare to suggest another type name for a metavariable in the
> semantic patch language?

Now that we have the feature of an expression metavariable that matches
the innermost enclosing expression that contains a token, I don't think it
is necessary.

julia