[Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Peter Maydell]

Add a new Coccinelle script which replaces uses of the inplace
byteswapping functions *_to_cpus() and cpu_to_*s() with their
not-in-place equivalents. This is useful for where the swapping
is done on members of a packed struct -- taking the address
of the member to pass it to an inplace function is undefined
behaviour in C.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Richard asked for a coccinelle script in the scripts/coccinelle
directory, so here's a patch to add it.

 scripts/coccinelle/inplace-byteswaps.cocci | 65 ++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 scripts/coccinelle/inplace-byteswaps.cocci

diff --git a/scripts/coccinelle/inplace-byteswaps.cocci b/scripts/coccinelle/inplace-byteswaps.cocci
new file mode 100644
index 00000000000..a869a90cbfd
--- /dev/null
+++ b/scripts/coccinelle/inplace-byteswaps.cocci
@@ -0,0 +1,65 @@
+// Replace uses of in-place byteswapping functions with calls to the
+// equivalent not-in-place functions.  This is necessary to avoid
+// undefined behaviour if the expression being swapped is a field in a
+// packed struct.
+
+@@
+expression E;
+@@
+-be16_to_cpus(&E);
++E = be16_to_cpu(E);
+@@
+expression E;
+@@
+-be32_to_cpus(&E);
++E = be32_to_cpu(E);
+@@
+expression E;
+@@
+-be64_to_cpus(&E);
++E = be64_to_cpu(E);
+@@
+expression E;
+@@
+-cpu_to_be16s(&E);
++E = cpu_to_be16(E);
+@@
+expression E;
+@@
+-cpu_to_be32s(&E);
++E = cpu_to_be32(E);
+@@
+expression E;
+@@
+-cpu_to_be64s(&E);
++E = cpu_to_be64(E);
+@@
+expression E;
+@@
+-le16_to_cpus(&E);
++E = le16_to_cpu(E);
+@@
+expression E;
+@@
+-le32_to_cpus(&E);
++E = le32_to_cpu(E);
+@@
+expression E;
+@@
+-le64_to_cpus(&E);
++E = le64_to_cpu(E);
+@@
+expression E;
+@@
+-cpu_to_le16s(&E);
++E = cpu_to_le16(E);
+@@
+expression E;
+@@
+-cpu_to_le32s(&E);
++E = cpu_to_le32(E);
+@@
+expression E;
+@@
+-cpu_to_le64s(&E);
++E = cpu_to_le64(E);
-- 
2.19.0

Re: [Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Eric Blake]

On 10/9/18 1:16 PM, Peter Maydell wrote:
> Add a new Coccinelle script which replaces uses of the inplace
> byteswapping functions *_to_cpus() and cpu_to_*s() with their
> not-in-place equivalents. This is useful for where the swapping
> is done on members of a packed struct -- taking the address
> of the member to pass it to an inplace function is undefined
> behaviour in C.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Richard asked for a coccinelle script in the scripts/coccinelle
> directory, so here's a patch to add it.
> 
>   scripts/coccinelle/inplace-byteswaps.cocci | 65 ++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>   create mode 100644 scripts/coccinelle/inplace-byteswaps.cocci
> 
> diff --git a/scripts/coccinelle/inplace-byteswaps.cocci b/scripts/coccinelle/inplace-byteswaps.cocci
> new file mode 100644
> index 00000000000..a869a90cbfd
> --- /dev/null
> +++ b/scripts/coccinelle/inplace-byteswaps.cocci
> @@ -0,0 +1,65 @@
> +// Replace uses of in-place byteswapping functions with calls to the
> +// equivalent not-in-place functions.  This is necessary to avoid
> +// undefined behaviour if the expression being swapped is a field in a
> +// packed struct.
> +
> +@@
> +expression E;
> +@@
> +-be16_to_cpus(&E);
> ++E = be16_to_cpu(E);
> +@@
> +expression E;
> +@@
> +-be32_to_cpus(&E);
> ++E = be32_to_cpu(E);

It's probably possible to shorten this as:

@@
expression E;
@@
(
-be16_to_cpus(&E);
+E = be16_to_cpu(E);
|
-be32_to_cpus(&E);
+E = be32_to_cpu(E);
...
)

But I'm also okay if the long form gets checked in.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Peter Maydell]

On 9 October 2018 at 19:23, Eric Blake <eblake@redhat.com> wrote:
> On 10/9/18 1:16 PM, Peter Maydell wrote:
>>
>> Add a new Coccinelle script which replaces uses of the inplace
>> byteswapping functions *_to_cpus() and cpu_to_*s() with their
>> not-in-place equivalents. This is useful for where the swapping
>> is done on members of a packed struct -- taking the address
>> of the member to pass it to an inplace function is undefined
>> behaviour in C.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Richard asked for a coccinelle script in the scripts/coccinelle
>> directory, so here's a patch to add it.
>>
>>   scripts/coccinelle/inplace-byteswaps.cocci | 65 ++++++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>   create mode 100644 scripts/coccinelle/inplace-byteswaps.cocci
>>
>> diff --git a/scripts/coccinelle/inplace-byteswaps.cocci
>> b/scripts/coccinelle/inplace-byteswaps.cocci
>> new file mode 100644
>> index 00000000000..a869a90cbfd
>> --- /dev/null
>> +++ b/scripts/coccinelle/inplace-byteswaps.cocci
>> @@ -0,0 +1,65 @@
>> +// Replace uses of in-place byteswapping functions with calls to the
>> +// equivalent not-in-place functions.  This is necessary to avoid
>> +// undefined behaviour if the expression being swapped is a field in a
>> +// packed struct.
>> +
>> +@@
>> +expression E;
>> +@@
>> +-be16_to_cpus(&E);
>> ++E = be16_to_cpu(E);
>> +@@
>> +expression E;
>> +@@
>> +-be32_to_cpus(&E);
>> ++E = be32_to_cpu(E);
>
>
> It's probably possible to shorten this as:

This is kind of why I didn't post it as a patch to go in
scripts/ initially :-)  I suspected it was possible to
shorten it (maybe there's even coccinelle syntax to allow the
function names to be substituted to be specified with
a regex?), but the dumb-and-simple cut-and-paste for every
function being replaced worked and it didn't seem worth
spending even ten minutes messing around trying to improve
what is fundamentally a throwaway script...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Philippe Mathieu-Daudé]

On 09/10/2018 20:35, Peter Maydell wrote:
> On 9 October 2018 at 19:23, Eric Blake <eblake@redhat.com> wrote:
>> On 10/9/18 1:16 PM, Peter Maydell wrote:
>>>
>>> Add a new Coccinelle script which replaces uses of the inplace
>>> byteswapping functions *_to_cpus() and cpu_to_*s() with their
>>> not-in-place equivalents. This is useful for where the swapping
>>> is done on members of a packed struct -- taking the address
>>> of the member to pass it to an inplace function is undefined
>>> behaviour in C.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Richard asked for a coccinelle script in the scripts/coccinelle
>>> directory, so here's a patch to add it.
>>>
>>>   scripts/coccinelle/inplace-byteswaps.cocci | 65 ++++++++++++++++++++++
>>>   1 file changed, 65 insertions(+)
>>>   create mode 100644 scripts/coccinelle/inplace-byteswaps.cocci
>>>
>>> diff --git a/scripts/coccinelle/inplace-byteswaps.cocci
>>> b/scripts/coccinelle/inplace-byteswaps.cocci
>>> new file mode 100644
>>> index 00000000000..a869a90cbfd
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/inplace-byteswaps.cocci
>>> @@ -0,0 +1,65 @@
>>> +// Replace uses of in-place byteswapping functions with calls to the
>>> +// equivalent not-in-place functions.  This is necessary to avoid
>>> +// undefined behaviour if the expression being swapped is a field in a
>>> +// packed struct.
>>> +
>>> +@@
>>> +expression E;
>>> +@@
>>> +-be16_to_cpus(&E);
>>> ++E = be16_to_cpu(E);
>>> +@@
>>> +expression E;
>>> +@@
>>> +-be32_to_cpus(&E);
>>> ++E = be32_to_cpu(E);
>>
>>
>> It's probably possible to shorten this as:
> 
> This is kind of why I didn't post it as a patch to go in
> scripts/ initially :-)  I suspected it was possible to
> shorten it (maybe there's even coccinelle syntax to allow the
> function names to be substituted to be specified with
> a regex?), but the dumb-and-simple cut-and-paste for every
> function being replaced worked and it didn't seem worth
> spending even ten minutes messing around trying to improve
> what is fundamentally a throwaway script...

Julia Lawall once [*] said:

Even for 6 functions, I would suggest to write out the function names in
the pattern matching code rather than using regular expressions.  If the
names are explicit, then Coccinelle can do some filtering, either based
on an index made with idutils or glimpse (see the coccinelle scripts
directory for tools for making these indices) or based on grep, to drop
without parsing files that are not relevant.  It doesn't do this for
regular expressions.  So you will get a faster result if the names are
explicit in the pattern code.

[*] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03004.html

Re: [Qemu-devel] [PATCH] coccinelle: new inplace-byteswaps.cocci to remove inplace-byteswapping calls

[Richard Henderson]

On 10/9/18 11:16 AM, Peter Maydell wrote:
> Add a new Coccinelle script which replaces uses of the inplace
> byteswapping functions *_to_cpus() and cpu_to_*s() with their
> not-in-place equivalents. This is useful for where the swapping
> is done on members of a packed struct -- taking the address
> of the member to pass it to an inplace function is undefined
> behaviour in C.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Richard asked for a coccinelle script in the scripts/coccinelle
> directory, so here's a patch to add it.
> 
>  scripts/coccinelle/inplace-byteswaps.cocci | 65 ++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 scripts/coccinelle/inplace-byteswaps.cocci

Thanks,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~