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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@bugseng.com>
>>> ---
>>>  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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)