Re: [XEN PATCH 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/10/2023 12:06 am, Nicola Vetrini wrote:
> On 10/10/2023 18:00, Andrew Cooper wrote:
>> On 10/10/2023 9:02 am, Stefano Stabellini wrote:
>>> On Fri, 6 Oct 2023, Nicola Vetrini wrote:
>>>> The essential type of the result of an inequality operator is
>>>> essentially boolean, therefore it shouldn't be used as an argument of
>>>> the multiplication operator, which expects an integer.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>> ---
>>>>  xen/include/xen/compat.h | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
>>>> index f2ce5bb3580a..5ffee6a9fed1 100644
>>>> --- a/xen/include/xen/compat.h
>>>> +++ b/xen/include/xen/compat.h
>>>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>>>      return x == c; \
>>>>  }
>>>>
>>>> +#define SIZE_NEQUAL(a, b) \
>>>> +    (sizeof(a) != sizeof(b) ? 1 : 0)
>>>>  #define CHECK_SIZE(name) \
>>>> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ##
>>>> _t) != \
>>>> -                                         sizeof(compat_ ## name ##
>>>> _t)) * 2]
>>>> +    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name
>>>> ## _t, \
>>>> +                                                     compat_ ##
>>>> name ## _t)) * 2]
>>>>  #define CHECK_SIZE_(k, n) \
>>>> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
>>>> -                                          sizeof(k compat_ ## n))
>>>> * 2]
>>>> +    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
>>>> +                                                      k compat_ ##
>>>> n)) * 2]
>>> I think this style is easier to read but I'll let the x86 maintainers
>>> decide
>>>
>>>     typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \
>>>                                      sizeof(compat_ ## name ## _t))
>>> ? 1 : -1]
>>>
>>> Also am I reading this correctly that we are using -1 as array index? I
>>> must have made a calculation mistake?
>>
>> This is a NIH BUILD_BUG_ON().  It should be rewritten as
>>
>> BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...));
>>
>> which will use _Static_assert() in modern compilers.
>>
>> ~Andrew
>
> Ok, thanks.
>

I'm going to go out on a limb and say that every other pattern that
looks like this probably wants converting to a BUILD_BUG_ON().

This code quite possibly predates the introduction of BUILD_BUG_ON(),
but we want to end up using BUILD_BUG_ON() everywhere because it's the
construct that uses _Static_assert() wherever possible.

~Andrew