[Cocci] Help with SMPL

[Cocci] Help with SMPL

[Peter Senna Tschudin]

Dear List,

I'm trying to make a semantic patch for the case described on the patch:
http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html

Some relevant code snippets:

http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L482
-- // --
struct em28xx {
...
int model;              /* index in the device_data struct */
...
struct em28xx_board board;
...
}
-- // --

http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L690
-- // --
...
extern struct em28xx_board em28xx_boards[];
...
-- // --

I'm trying the semantic patch:
@@
type T;
expression to,from,flag;
@@
- memcpy((T)&to,(T)&from,flag);
+ to = from;

But it is not working. How can I specify "to" and "from" for match
cases like this?

Thanks,

Peter
-- 
Peter

[Cocci] Help with SMPL

[Lars-Peter Clausen]

On 09/26/2012 04:36 PM, Peter Senna Tschudin wrote:
> Dear List,
> 
> I'm trying to make a semantic patch for the case described on the patch:
> http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html
> 
> Some relevant code snippets:
> 
> http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L482
> -- // --
> struct em28xx {
> ...
> int model;              /* index in the device_data struct */
> ...
> struct em28xx_board board;
> ...
> }
> -- // --
> 
> http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L690
> -- // --
> ...
> extern struct em28xx_board em28xx_boards[];
> ...
> -- // --
> 
> I'm trying the semantic patch:
> @@
> type T;
> expression to,from,flag;
> @@
> - memcpy((T)&to,(T)&from,flag);
> + to = from;
> 
> But it is not working. How can I specify "to" and "from" for match
> cases like this?

Do you want the types of "to" and "from" to match? Then you could do
something like:

@@
type T;
T to;
T from;
expression size;
@@
-memcpy(&to, &from, size);
+to = from;

- Lars

[Cocci] Help with SMPL

[Ezequiel Garcia]

Hi,

On Wed, Sep 26, 2012 at 12:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 09/26/2012 04:36 PM, Peter Senna Tschudin wrote:
>> Dear List,
>>
>> I'm trying to make a semantic patch for the case described on the patch:
>> http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html
>>

More cases like these:

http://patchwork.linuxtv.org/patch/13100/

This is part of a whole patchset with subject:

media: XXX: Replace struct memcpy with struct assignment

The type of src and dst structs should match, but in some buggy code
that may not be true.

Hope it helps,
Ezequiel.

[Cocci] Help with SMPL

[Lars-Peter Clausen]

On 09/26/2012 05:40 PM, Ezequiel Garcia wrote:
> Hi,
> 
> On Wed, Sep 26, 2012 at 12:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 09/26/2012 04:36 PM, Peter Senna Tschudin wrote:
>>> Dear List,
>>>
>>> I'm trying to make a semantic patch for the case described on the patch:
>>> http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html
>>>
> 
> More cases like these:
> 
> http://patchwork.linuxtv.org/patch/13100/
> 
> This is part of a whole patchset with subject:
> 
> media: XXX: Replace struct memcpy with struct assignment
> 
> The type of src and dst structs should match, but in some buggy code
> that may not be true.

I suppose these should not be auto-converted since there is clearly a bug
somewhere. The following cocci patch can be used to find locations where
memcpy is used and the types of "to" and "from" do not match, but the size
parameter is sizeof one of them.

@r1@
type T;
T to;
T from;
position p;
@@
memcpy at p(&to, &from,
(
 sizeof(T)
|
 sizeof(to)
|
 sizeof(from)
)
 );

@r2@
type T;
T to;
const T from;
position p;
@@
memcpy at p(&to, &from,
(
 sizeof(T)
|
 sizeof(to)
|
 sizeof(from)
)
 );


@@
position p != {r1.p,r2.p};
type T1, T2;
T1 to;
T2 from;
@@
*memcpy at p(&to, &from,
(
 sizeof(T1)
|
 sizeof(T2)
|
 sizeof(to)
|
 sizeof(from)
)
*);

[Cocci] Help with SMPL

[Ezequiel Garcia]

On Wed, Sep 26, 2012 at 1:01 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 09/26/2012 05:40 PM, Ezequiel Garcia wrote:
>> Hi,
>>
>> On Wed, Sep 26, 2012 at 12:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 09/26/2012 04:36 PM, Peter Senna Tschudin wrote:
>>>> Dear List,
>>>>
>>>> I'm trying to make a semantic patch for the case described on the patch:
>>>> http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html
>>>>
>>
>> More cases like these:
>>
>> http://patchwork.linuxtv.org/patch/13100/
>>
>> This is part of a whole patchset with subject:
>>
>> media: XXX: Replace struct memcpy with struct assignment
>>
>> The type of src and dst structs should match, but in some buggy code
>> that may not be true.
>
> I suppose these should not be auto-converted since there is clearly a bug
> somewhere. The following cocci patch can be used to find locations where
> memcpy is used and the types of "to" and "from" do not match, but the size
> parameter is sizeof one of them.
>

True. This should not be auto-converted but rather be used as warning:
"Hey! you're doing some very bad things here"

Thanks,
Ezequiel.

[Cocci] Help with SMPL

[Peter Senna Tschudin]

> I suppose these should not be auto-converted since there is clearly a bug
> somewhere. The following cocci patch can be used to find locations where
> memcpy is used and the types of "to" and "from" do not match, but the size
> parameter is sizeof one of them.

Yes, I wanted to generate an index of possible candidates for manual
checking. Thanks for the second patch.


-- 
Peter

[Cocci] Help with SMPL

[Peter Senna Tschudin]

On Wed, Sep 26, 2012 at 5:27 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 09/26/2012 04:36 PM, Peter Senna Tschudin wrote:
>> Dear List,
>>
>> I'm trying to make a semantic patch for the case described on the patch:
>> http://www.mail-archive.com/linux-media at vger.kernel.org/msg52660.html
>>
>> Some relevant code snippets:
>>
>> http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L482
>> -- // --
>> struct em28xx {
>> ...
>> int model;              /* index in the device_data struct */
>> ...
>> struct em28xx_board board;
>> ...
>> }
>> -- // --
>>
>> http://lxr.free-electrons.com/source/drivers/media/video/em28xx/em28xx.h#L690
>> -- // --
>> ...
>> extern struct em28xx_board em28xx_boards[];
>> ...
>> -- // --
>>
>> I'm trying the semantic patch:
>> @@
>> type T;
>> expression to,from,flag;
>> @@
>> - memcpy((T)&to,(T)&from,flag);
>> + to = from;
>>
>> But it is not working. How can I specify "to" and "from" for match
>> cases like this?
>
> Do you want the types of "to" and "from" to match? Then you could do
> something like:
>
> @@
> type T;
> T to;
> T from;
> expression size;
> @@
> -memcpy(&to, &from, size);
> +to = from;

Works perfectly! Thank you!

>
> - Lars



-- 
Peter

[Cocci] Help with SmPL

[Peter Senna Tschudin]

This thread started on previous list...

I'm using the Cocci patch for finding unnecessary semicolons:
@r_case@
position p;
@@
switch (...)
{
case ...: ...;@p
}

@r_default@
position p;
@@
switch (...)
{
default: ...;@p
}

@r1@
statement S;
position p1;
position p != {r_case.p, r_default.p};
identifier label;
@@
(
label:;
|
S at p1;@p
)

@script:python r2@
p << r1.p;
p_case << r_case.p;
p1 << r1.p1;
@@
if p[0].line != p1[0].line_end:
        cocci.include_match(False)
@@
position r1.p;
@@
-;@p

I've identified two problems:

1 - Macros. It is very common to remove ";" after a call to a macro like on:

diff -u -p a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -2690,10 +2690,10 @@ isdn_tty_cmd_ATand(char **p, modem_info
                p[0]++;
                i = isdn_getnum(p);
                if ((i < 0) || (i > ISDN_SERIAL_XMIT_MAX))
-                       PARSE_ERROR1;
+                       PARSE_ERROR1

or

diff -u -p a/drivers/input/keyboard/sh_keysc.c
b/drivers/input/keyboard/sh_keysc.c
--- a/drivers/input/keyboard/sh_keysc.c
+++ b/drivers/input/keyboard/sh_keysc.c
@@ -90,8 +90,8 @@ static irqreturn_t sh_keysc_isr(int irq,
        int keyout_nr = sh_keysc_mode[pdata->mode].keyout;
        int keyin_nr = sh_keysc_mode[pdata->mode].keyin;
        DECLARE_BITMAP(keys, SH_KEYSC_MAXKEYS);
-       DECLARE_BITMAP(keys0, SH_KEYSC_MAXKEYS);
-       DECLARE_BITMAP(keys1, SH_KEYSC_MAXKEYS);
+       DECLARE_BITMAP(keys0, SH_KEYSC_MAXKEYS)
+       DECLARE_BITMAP(keys1, SH_KEYSC_MAXKEYS)
        unsigned char keyin_set, tmp;
        int i, k, n;

2 - Empty switch case / default, like on:
diff -u -p a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
--- a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
@@ -60,7 +60,6 @@ static uint16_t compute_tps(struct dtv_f
        case FEC_1_2:
        case FEC_AUTO:
        default:
-               /* tps |= (0 << 7) */;
        }

In this case, if the semicolon is removed, gcc complains:
drivers/media/usb/dvb-usb/cinergyT2-fe.c:62:2: error: label at end of
compound statement

Any ideas?

Thanks,

Peter

On Fri, Aug 3, 2012 at 11:10 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 3 Aug 2012, Lars-Peter Clausen wrote:
>
>> On 08/03/2012 06:36 PM, Julia Lawall wrote:
>>>>
>>>> This one is a bit tricky, but it takes care of the case where cocci
>>>> can't
>>>> parse the statement inform of the semicolon.
>>>
>>>
>>> A simpler approach would be as follows:
>>>
>>>> @r1@
>>>> statement S;
>>>> position p;
>>>
>>>   position p1;
>>>>
>>>> @@
>>>> S
>>>
>>>
>>> change to S at p1
>>>
>>>> ;@p
>>>>
>>>> @script:python r2@
>>>> p << r1.p;
>>>> @@
>>>
>>>
>>> Change to call cocci.include_match(False) if p1[0].line_end != p[0].line
>>
>>
>> Hm, that's a good idea. But I guess it may produce false positives for
>> something like foo(); not_parseable(...);, but that's probably bad coding
>> style anyway.
>>
>> And this still leaves us with the false positives for case:; and default:;
>>
>> Maybe this could work:
>> @r_case@
>> position p;
>> @@
>> switch (...)
>> {
>> case ...: ...;@p
>> }
>
>
> I don't think you need the ... in front of the ;
>
> julia
>
>
>> @r_default@
>> position p;
>> @@
>> switch (...)
>> {
>> default: ...;@p
>> }
>>
>> @r1@
>> statement S;
>> position p1;
>> position p != {r_case.p, r_default.p};
>> identifier label;
>> @@
>> (
>> label:;
>> |
>> S at p1;@p
>> )
>>
>> @script:python r2@
>> p << r1.p;
>> p_case << r_case.p;
>> p1 << r1.p1;
>> @@
>> if p[0].line != p1[0].line_end:
>>         cocci.include_match(False)
>>
>> @@
>> position r1.p;
>> @@
>> -;@p
>>
> _______________________________________________
> Cocci mailing list
> Cocci at diku.dk
> http://lists.diku.dk/mailman/listinfo/cocci
> (Web access from inside DIKUs LAN only)



-- 
Peter

[Cocci] Help with SmPL

[Håkon Løvdal]

On 27 September 2012 17:29, Peter Senna Tschudin <peter.senna@gmail.com> wrote:
> 2 - Empty switch case / default, like on:
> diff -u -p a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
> b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
> --- a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
> @@ -60,7 +60,6 @@ static uint16_t compute_tps(struct dtv_f
>         case FEC_1_2:
>         case FEC_AUTO:
>         default:
> -               /* tps |= (0 << 7) */;
>         }
>
> In this case, if the semicolon is removed, gcc complains:
> drivers/media/usb/dvb-usb/cinergyT2-fe.c:62:2: error: label at end of
> compound statement
>
> Any ideas?

Gcc complains because it finds a label not followed by a statement
(the removed semicolon served that role before).
For this specific case I would remove the semicolon after the comment
and then insert a break statement after, e.g.

        case FEC_1_2:
        case FEC_AUTO:
        default:
                /* tps |= (0 << 7) */
                break;
        }


BR H?kon L?vdal

[Cocci] Help with SmPL

[Håkon Løvdal]

On 27 September 2012 18:30, H?kon L?vdal <hlovdal@gmail.com> wrote:
> On 27 September 2012 17:29, Peter Senna Tschudin <peter.senna@gmail.com> wrote:
>> 2 - Empty switch case / default, like on:
>> diff -u -p a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
>> b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
>> --- a/drivers/media/usb/dvb-usb/cinergyT2-fe.c
>> +++ b/drivers/media/usb/dvb-usb/cinergyT2-fe.c
>> @@ -60,7 +60,6 @@ static uint16_t compute_tps(struct dtv_f
>>         case FEC_1_2:
>>         case FEC_AUTO:
>>         default:
>> -               /* tps |= (0 << 7) */;
>>         }
>>
>> In this case, if the semicolon is removed, gcc complains:
>> drivers/media/usb/dvb-usb/cinergyT2-fe.c:62:2: error: label at end of
>> compound statement
>>
>> Any ideas?
>
> Gcc complains because it finds a label not followed by a statement
> (the removed semicolon served that role before).
> For this specific case I would remove the semicolon after the comment
> and then insert a break statement after, e.g.
>
>         case FEC_1_2:
>         case FEC_AUTO:
>         default:
>                 /* tps |= (0 << 7) */
>                 break;
>         }
>

I see now more what your problem is. I suspect that the trouble is
related to the labels
in front of default. When I test with the following source code


        void test(int i)
        {
                switch (i) {
                case 0:
                        /**/;
                default:
                        /**/;
                }

                switch (i) {
                case 0:
                        /**/;
                case 1:
                default:
                        /**/;
                }
        }

and the following coccinelle script


        @r_case_statement@
        statement S;
        position p;
        @@
        switch(...)
        {
        case ...: S;@p
        }


        @r_default_statement@
        statement S;
        position p;
        @@
        switch(...)
        {
        default: S;@p
        }


        @r_case_nostatement@
        position p1 != r_case_statement.p;
        @@
        switch(...)
        {
        *case ...: ...;@p1
        }


        @r_default_nostatement@
        position p1 != r_case_statement.p;
        @@
        switch(...)
        {
        *default: ...;@p1
        }

the output is


init_defs_builtins: /usr/share/coccinelle/standard.h
HANDLING: main.c
diff =
        --- main.c
        +++ /tmp/cocci-output-3854-986c14-main.c
        @@ -2,15 +2,9 @@
         void test(int i)
         {
                 switch (i) {
        -        case 0:
        -                /**/;
        -        default:
        -                /**/;
                 }

                 switch (i) {
        -        case 0:
        -                /**/;
                 case 1:
                 default:
                         /**/;


Notice the missing match on the second default label. I do not know
what to change to make
coccinelle match this as well.

BR H?kon L?vdal