Re: [XEN PATCH][for-4.19 8/9] xen/types: address Rule 10.1 for DECLARE_BITMAP use

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

On 10/10/2023 14:15, Julien Grall wrote:
> On 10/10/2023 13:13, Julien Grall wrote:
>> 
>> 
>> On 10/10/2023 13:07, Nicola Vetrini wrote:
>>> 
>>>>>>> 
>>>>>>> I agree on everything Julien's wrote and I was about to suggest 
>>>>>>> to use a
>>>>>>> SAF comment to suppress the warning because it is clearer than a 
>>>>>>> int
>>>>>>> cast.
>>>>>>> 
>>>>>>> But then I realized that even if BITS_TO_LONGS was fixed, 
>>>>>>> wouldn't still
>>>>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>>>>> 
>>>>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>>>>> regardless,
>>>>>>> either to int or unsigned int or whatever to be used in 
>>>>>>> DECLARE_BITMAP?
>>>>>>> 
>>>>>>> 
>>>>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking 
>>>>>>> int
>>>>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>>>>> being
>>>>>>> an enum.
>>>>>>> 
>>>>>>> If I got it right, then I would go with the cast to int (like 
>>>>>>> done in
>>>>>>> this patch) with a decent comment on top of it.
>>>>>> 
>>>>>> I might be missing something here. But why should we use cast 
>>>>>> rather than /*
>>>>>> SAF-X */ just above? I would have expected we wanted to highlight 
>>>>>> the
>>>>>> violation rather than hiding it.
>>>>> 
>>>>> My understanding is that the cast is required when converting an 
>>>>> enum
>>>>> type to an integer type and vice versa. The idea is that we 
>>>>> shouldn't do
>>>>> implicit conversions as they are error prone, only explicit 
>>>>> conversions
>>>>> are allowed between enum and integers.
>>>> 
>>>> We have a lot of places in Xen using implicit conversion from enum 
>>>> to
>>>> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
>>>> report all of them? If not, then why?
>>>> 
>>> 
>>> Can you give some pointers as to where this enum is used in 
>>> arithmetic operations?
>> 
>> I can't think of any right now.
> 
> Obviously, right after I pressed send, I remember of one in
> __acpi_map_table() (x86 code).
> 
> FIX_ACPI_END is an enum, assigned to an 'int' and then used in 
> arithmetics.
> 
> Cheers,

A couple of remarks:
- that file is part of the exclude-list.json, therefore it's not bound 
to be MISRA compliant
   right now;
- assignment to a different essential type category is dealt with by 
Rule 10.3
   (The value of an expression shall not be assigned to an object with a 
narrower
    essential type or of a different essential type category), so perhaps
   Luca's script does indeed capture it with gcc compilation flags.

Aside from this, if you feel that a deviation comment is a better 
choice, I'm ok with
dealing with it in this way.

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