| Message ID | 9642dcb4ab51ec9eaeedf16056fbcd946a3efbea.1696514677.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
Hi Nicola, On 06/10/2023 09:26, Nicola Vetrini wrote: > Given its use in the declaration > 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument > 'bits' has essential type 'enum iommu_feature', which is not > allowed by the Rule as an operand to the addition operator > in macro 'BITS_TO_LONGS'. > > A comment in BITS_TO_LONGS is added to make it clear that > values passed are meant to be positive. I am confused. If the value is meant to be positive. Then... > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/include/xen/iommu.h | 2 +- > xen/include/xen/types.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 0e747b0bbc1c..34aa0b9b5b81 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -360,7 +360,7 @@ struct domain_iommu { > #endif > > /* Features supported by the IOMMU */ > - DECLARE_BITMAP(features, IOMMU_FEAT_count); > + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); ... why do we cast to (int) rather than (unsigned int)? Also, I think this cast deserve a comment on top because this is not a very obvious one. > > /* Does the guest share HAP mapping with the IOMMU? */ > bool hap_pt_share; > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h > index aea259db1ef2..936e83d333a0 100644 > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h > @@ -22,6 +22,7 @@ typedef signed long ssize_t; > > typedef __PTRDIFF_TYPE__ ptrdiff_t; > > +/* Users of this macro are expected to pass a positive value */ > #define BITS_TO_LONGS(bits) \ > (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) > #define DECLARE_BITMAP(name,bits) \ Cheers,
On 06/10/2023 11:34, Julien Grall wrote: > Hi Nicola, > > On 06/10/2023 09:26, Nicola Vetrini wrote: >> Given its use in the declaration >> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >> 'bits' has essential type 'enum iommu_feature', which is not >> allowed by the Rule as an operand to the addition operator >> in macro 'BITS_TO_LONGS'. >> >> A comment in BITS_TO_LONGS is added to make it clear that >> values passed are meant to be positive. > > I am confused. If the value is meant to be positive. Then... > >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/include/xen/iommu.h | 2 +- >> xen/include/xen/types.h | 1 + >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 0e747b0bbc1c..34aa0b9b5b81 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -360,7 +360,7 @@ struct domain_iommu { >> #endif >> /* Features supported by the IOMMU */ >> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); > > ... why do we cast to (int) rather than (unsigned int)? Also, I think > this cast deserve a comment on top because this is not a very obvious > one. > >> /* Does the guest share HAP mapping with the IOMMU? */ >> bool hap_pt_share; >> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >> index aea259db1ef2..936e83d333a0 100644 >> --- a/xen/include/xen/types.h >> +++ b/xen/include/xen/types.h >> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >> typedef __PTRDIFF_TYPE__ ptrdiff_t; >> +/* Users of this macro are expected to pass a positive value */ >> #define BITS_TO_LONGS(bits) \ >> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >> #define DECLARE_BITMAP(name,bits) \ > > Cheers, See [1] for the reason why I did so. I should have mentioned that in the commit notes, sorry. In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of build errors and possibly other essential type violations. If this is to be fixed that way, the effort required is far greater. Either way, a comment on top of can be added, along the lines of: Leaving this as an enum would violate MISRA C:2012 Rule 10.1 [1] https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/
Hi Nicola, On 06/10/2023 11:10, Nicola Vetrini wrote: > On 06/10/2023 11:34, Julien Grall wrote: >> Hi Nicola, >> >> On 06/10/2023 09:26, Nicola Vetrini wrote: >>> Given its use in the declaration >>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >>> 'bits' has essential type 'enum iommu_feature', which is not >>> allowed by the Rule as an operand to the addition operator >>> in macro 'BITS_TO_LONGS'. >>> >>> A comment in BITS_TO_LONGS is added to make it clear that >>> values passed are meant to be positive. >> >> I am confused. If the value is meant to be positive. Then... >> >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> xen/include/xen/iommu.h | 2 +- >>> xen/include/xen/types.h | 1 + >>> 2 files changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>> index 0e747b0bbc1c..34aa0b9b5b81 100644 >>> --- a/xen/include/xen/iommu.h >>> +++ b/xen/include/xen/iommu.h >>> @@ -360,7 +360,7 @@ struct domain_iommu { >>> #endif >>> /* Features supported by the IOMMU */ >>> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >>> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >> >> ... why do we cast to (int) rather than (unsigned int)? Also, I think >> this cast deserve a comment on top because this is not a very obvious >> one. >> >>> /* Does the guest share HAP mapping with the IOMMU? */ >>> bool hap_pt_share; >>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >>> index aea259db1ef2..936e83d333a0 100644 >>> --- a/xen/include/xen/types.h >>> +++ b/xen/include/xen/types.h >>> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >>> typedef __PTRDIFF_TYPE__ ptrdiff_t; >>> +/* Users of this macro are expected to pass a positive value */ >>> #define BITS_TO_LONGS(bits) \ >>> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >>> #define DECLARE_BITMAP(name,bits) \ >> >> Cheers, > > See [1] for the reason why I did so. I should have mentioned that in the > commit notes, sorry. > In short, making BITS_TO_LONGS essentially unsigned would cause a > cascade of build errors and > possibly other essential type violations. Can you share some of the errors? > If this is to be fixed that > way, the effort required > is far greater. Either way, a comment on top of can be added, along the > lines of: > > Leaving this as an enum would violate MISRA C:2012 Rule 10.1 I read this as you are simply trying to silence your tool. I think this the wrong approach as you are now making the code confusing for the reader (you pass a signed int to a function that is supposed to take a positive number). I appreciate that this will result to more violations at the beginning. But the whole point of MISRA is to make the code better. If this is too complex to solve now, then a possible option is to deviate for the time being. Cheers, > > [1] > https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/ >
On Fri, 6 Oct 2023, Julien Grall wrote: > Hi Nicola, > > On 06/10/2023 11:10, Nicola Vetrini wrote: > > On 06/10/2023 11:34, Julien Grall wrote: > > > Hi Nicola, > > > > > > On 06/10/2023 09:26, Nicola Vetrini wrote: > > > > Given its use in the declaration > > > > 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument > > > > 'bits' has essential type 'enum iommu_feature', which is not > > > > allowed by the Rule as an operand to the addition operator > > > > in macro 'BITS_TO_LONGS'. > > > > > > > > A comment in BITS_TO_LONGS is added to make it clear that > > > > values passed are meant to be positive. > > > > > > I am confused. If the value is meant to be positive. Then... > > > > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > > > --- > > > > xen/include/xen/iommu.h | 2 +- > > > > xen/include/xen/types.h | 1 + > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > > > > index 0e747b0bbc1c..34aa0b9b5b81 100644 > > > > --- a/xen/include/xen/iommu.h > > > > +++ b/xen/include/xen/iommu.h > > > > @@ -360,7 +360,7 @@ struct domain_iommu { > > > > #endif > > > > /* Features supported by the IOMMU */ > > > > - DECLARE_BITMAP(features, IOMMU_FEAT_count); > > > > + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); > > > > > > ... why do we cast to (int) rather than (unsigned int)? Also, I think > > > this cast deserve a comment on top because this is not a very obvious > > > one. > > > > > > > /* Does the guest share HAP mapping with the IOMMU? */ > > > > bool hap_pt_share; > > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h > > > > index aea259db1ef2..936e83d333a0 100644 > > > > --- a/xen/include/xen/types.h > > > > +++ b/xen/include/xen/types.h > > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t; > > > > typedef __PTRDIFF_TYPE__ ptrdiff_t; > > > > +/* Users of this macro are expected to pass a positive value */ > > > > #define BITS_TO_LONGS(bits) \ > > > > (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) > > > > #define DECLARE_BITMAP(name,bits) \ > > > > > > Cheers, > > > > See [1] for the reason why I did so. I should have mentioned that in the > > commit notes, sorry. > > In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of > > build errors and > > possibly other essential type violations. > Can you share some of the errors? > > > If this is to be fixed that way, the effort required > > is far greater. Either way, a comment on top of can be added, along the > > lines of: > > > > Leaving this as an enum would violate MISRA C:2012 Rule 10.1 > > I read this as you are simply trying to silence your tool. I think this the > wrong approach as you are now making the code confusing for the reader (you > pass a signed int to a function that is supposed to take a positive number). > > I appreciate that this will result to more violations at the beginning. But > the whole point of MISRA is to make the code better. > > If this is too complex to solve now, then a possible option is to deviate for > the time being. 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.
On 06/10/2023 16:47, Julien Grall wrote: > Hi Nicola, > > On 06/10/2023 11:10, Nicola Vetrini wrote: >> On 06/10/2023 11:34, Julien Grall wrote: >>> Hi Nicola, >>> >>> On 06/10/2023 09:26, Nicola Vetrini wrote: >>>> Given its use in the declaration >>>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >>>> 'bits' has essential type 'enum iommu_feature', which is not >>>> allowed by the Rule as an operand to the addition operator >>>> in macro 'BITS_TO_LONGS'. >>>> >>>> A comment in BITS_TO_LONGS is added to make it clear that >>>> values passed are meant to be positive. >>> >>> I am confused. If the value is meant to be positive. Then... >>> >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> xen/include/xen/iommu.h | 2 +- >>>> xen/include/xen/types.h | 1 + >>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>>> index 0e747b0bbc1c..34aa0b9b5b81 100644 >>>> --- a/xen/include/xen/iommu.h >>>> +++ b/xen/include/xen/iommu.h >>>> @@ -360,7 +360,7 @@ struct domain_iommu { >>>> #endif >>>> /* Features supported by the IOMMU */ >>>> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >>>> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >>> >>> ... why do we cast to (int) rather than (unsigned int)? Also, I think >>> this cast deserve a comment on top because this is not a very obvious >>> one. >>> >>>> /* Does the guest share HAP mapping with the IOMMU? */ >>>> bool hap_pt_share; >>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >>>> index aea259db1ef2..936e83d333a0 100644 >>>> --- a/xen/include/xen/types.h >>>> +++ b/xen/include/xen/types.h >>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >>>> typedef __PTRDIFF_TYPE__ ptrdiff_t; >>>> +/* Users of this macro are expected to pass a positive value */ >>>> #define BITS_TO_LONGS(bits) \ >>>> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >>>> #define DECLARE_BITMAP(name,bits) \ >>> >>> Cheers, >> >> See [1] for the reason why I did so. I should have mentioned that in >> the commit notes, sorry. >> In short, making BITS_TO_LONGS essentially unsigned would cause a >> cascade of build errors and >> possibly other essential type violations. > Can you share some of the errors? > I don't have that build log at hand, but you can reproduce them by defining #define BYTES_PER_LONG (_AC(1,U) << LONG_BYTEORDER) #define BITS_PER_LONG (BYTES_PER_LONG << 3) #define BITS_TO_LONGS(bits) \ (((bits)+BITS_PER_LONG-1U)/BITS_PER_LONG) and then fiddle a bit with the signedness of the constants in xen/arch/x86/include/asm/page-bits.h. They are essentially pointer comparison errors in the instances of 'min' and 'max' that involve those macros or macros derived from those, which make the build fail because they (rightfully) trigger a warning from gcc. >> If this is to be fixed that way, the effort required >> is far greater. Either way, a comment on top of can be added, along >> the lines of: >> >> Leaving this as an enum would violate MISRA C:2012 Rule 10.1 > > I read this as you are simply trying to silence your tool. I think > this the wrong approach as you are now making the code confusing for > the reader (you pass a signed int to a function that is supposed to > take a positive number). > > I appreciate that this will result to more violations at the > beginning. But the whole point of MISRA is to make the code better. > > If this is too complex to solve now, then a possible option is to > deviate for the time being. > > Cheers, > You have a point in that is sort of hides a deeper issue that is constituted by the signedness of the macro, but I'd suggest adding to this patch a comment explaining what needs to be done, rather than a deviation comment that hides the violations. The reason is that in the former case, if you put an unsigned argument too big to fit in an integer it will generate a new report, while if a negative argument is used, there is a warning comment on the macro definition (not an ideal countermeasure, though). >> >> [1] >> https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/ >>
On 07/10/2023 03:04, Stefano Stabellini wrote: > On Fri, 6 Oct 2023, Julien Grall wrote: >> Hi Nicola, >> >> On 06/10/2023 11:10, Nicola Vetrini wrote: >> > On 06/10/2023 11:34, Julien Grall wrote: >> > > Hi Nicola, >> > > >> > > On 06/10/2023 09:26, Nicola Vetrini wrote: >> > > > Given its use in the declaration >> > > > 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >> > > > 'bits' has essential type 'enum iommu_feature', which is not >> > > > allowed by the Rule as an operand to the addition operator >> > > > in macro 'BITS_TO_LONGS'. >> > > > >> > > > A comment in BITS_TO_LONGS is added to make it clear that >> > > > values passed are meant to be positive. >> > > >> > > I am confused. If the value is meant to be positive. Then... >> > > >> > > > >> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> > > > --- >> > > > xen/include/xen/iommu.h | 2 +- >> > > > xen/include/xen/types.h | 1 + >> > > > 2 files changed, 2 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> > > > index 0e747b0bbc1c..34aa0b9b5b81 100644 >> > > > --- a/xen/include/xen/iommu.h >> > > > +++ b/xen/include/xen/iommu.h >> > > > @@ -360,7 +360,7 @@ struct domain_iommu { >> > > > #endif >> > > > /* Features supported by the IOMMU */ >> > > > - DECLARE_BITMAP(features, IOMMU_FEAT_count); >> > > > + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >> > > >> > > ... why do we cast to (int) rather than (unsigned int)? Also, I think >> > > this cast deserve a comment on top because this is not a very obvious >> > > one. >> > > >> > > > /* Does the guest share HAP mapping with the IOMMU? */ >> > > > bool hap_pt_share; >> > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >> > > > index aea259db1ef2..936e83d333a0 100644 >> > > > --- a/xen/include/xen/types.h >> > > > +++ b/xen/include/xen/types.h >> > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t; >> > > > typedef __PTRDIFF_TYPE__ ptrdiff_t; >> > > > +/* Users of this macro are expected to pass a positive value */ >> > > > #define BITS_TO_LONGS(bits) \ >> > > > (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >> > > > #define DECLARE_BITMAP(name,bits) \ >> > > >> > > Cheers, >> > >> > See [1] for the reason why I did so. I should have mentioned that in the >> > commit notes, sorry. >> > In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of >> > build errors and >> > possibly other essential type violations. >> Can you share some of the errors? >> >> > If this is to be fixed that way, the effort required >> > is far greater. Either way, a comment on top of can be added, along the >> > lines of: >> > >> > Leaving this as an enum would violate MISRA C:2012 Rule 10.1 >> >> I read this as you are simply trying to silence your tool. I think >> this the >> wrong approach as you are now making the code confusing for the reader >> (you >> pass a signed int to a function that is supposed to take a positive >> number). >> >> I appreciate that this will result to more violations at the >> beginning. But >> the whole point of MISRA is to make the code better. >> >> If this is too complex to solve now, then a possible option is to >> deviate for >> the time being. > > 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? > Indeed. MISRA interprets enums as unordered sets of constants. > 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? > Correct. See also my reply to Julien's message: right now value-preserving conversions do not generate violations, but a non-value preserving conversion from unsigned to signed will generate one. > > 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.
On 07/10/2023 02:04, Stefano Stabellini wrote: > On Fri, 6 Oct 2023, Julien Grall wrote: >> Hi Nicola, >> >> On 06/10/2023 11:10, Nicola Vetrini wrote: >>> On 06/10/2023 11:34, Julien Grall wrote: >>>> Hi Nicola, >>>> >>>> On 06/10/2023 09:26, Nicola Vetrini wrote: >>>>> Given its use in the declaration >>>>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >>>>> 'bits' has essential type 'enum iommu_feature', which is not >>>>> allowed by the Rule as an operand to the addition operator >>>>> in macro 'BITS_TO_LONGS'. >>>>> >>>>> A comment in BITS_TO_LONGS is added to make it clear that >>>>> values passed are meant to be positive. >>>> >>>> I am confused. If the value is meant to be positive. Then... >>>> >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> xen/include/xen/iommu.h | 2 +- >>>>> xen/include/xen/types.h | 1 + >>>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644 >>>>> --- a/xen/include/xen/iommu.h >>>>> +++ b/xen/include/xen/iommu.h >>>>> @@ -360,7 +360,7 @@ struct domain_iommu { >>>>> #endif >>>>> /* Features supported by the IOMMU */ >>>>> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >>>>> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >>>> >>>> ... why do we cast to (int) rather than (unsigned int)? Also, I think >>>> this cast deserve a comment on top because this is not a very obvious >>>> one. >>>> >>>>> /* Does the guest share HAP mapping with the IOMMU? */ >>>>> bool hap_pt_share; >>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >>>>> index aea259db1ef2..936e83d333a0 100644 >>>>> --- a/xen/include/xen/types.h >>>>> +++ b/xen/include/xen/types.h >>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >>>>> typedef __PTRDIFF_TYPE__ ptrdiff_t; >>>>> +/* Users of this macro are expected to pass a positive value */ >>>>> #define BITS_TO_LONGS(bits) \ >>>>> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >>>>> #define DECLARE_BITMAP(name,bits) \ >>>> >>>> Cheers, >>> >>> See [1] for the reason why I did so. I should have mentioned that in the >>> commit notes, sorry. >>> In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of >>> build errors and >>> possibly other essential type violations. >> Can you share some of the errors? >> >>> If this is to be fixed that way, the effort required >>> is far greater. Either way, a comment on top of can be added, along the >>> lines of: >>> >>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1 >> >> I read this as you are simply trying to silence your tool. I think this the >> wrong approach as you are now making the code confusing for the reader (you >> pass a signed int to a function that is supposed to take a positive number). >> >> I appreciate that this will result to more violations at the beginning. But >> the whole point of MISRA is to make the code better. >> >> If this is too complex to solve now, then a possible option is to deviate for >> the time being. > > 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. Ultimately, the code is mantained by Jan. So this is his call. Cheers,
On Mon, 9 Oct 2023, Julien Grall wrote: > On 07/10/2023 02:04, Stefano Stabellini wrote: > > On Fri, 6 Oct 2023, Julien Grall wrote: > > > Hi Nicola, > > > > > > On 06/10/2023 11:10, Nicola Vetrini wrote: > > > > On 06/10/2023 11:34, Julien Grall wrote: > > > > > Hi Nicola, > > > > > > > > > > On 06/10/2023 09:26, Nicola Vetrini wrote: > > > > > > Given its use in the declaration > > > > > > 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument > > > > > > 'bits' has essential type 'enum iommu_feature', which is not > > > > > > allowed by the Rule as an operand to the addition operator > > > > > > in macro 'BITS_TO_LONGS'. > > > > > > > > > > > > A comment in BITS_TO_LONGS is added to make it clear that > > > > > > values passed are meant to be positive. > > > > > > > > > > I am confused. If the value is meant to be positive. Then... > > > > > > > > > > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > > > > > --- > > > > > > xen/include/xen/iommu.h | 2 +- > > > > > > xen/include/xen/types.h | 1 + > > > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > > > > > > index 0e747b0bbc1c..34aa0b9b5b81 100644 > > > > > > --- a/xen/include/xen/iommu.h > > > > > > +++ b/xen/include/xen/iommu.h > > > > > > @@ -360,7 +360,7 @@ struct domain_iommu { > > > > > > #endif > > > > > > /* Features supported by the IOMMU */ > > > > > > - DECLARE_BITMAP(features, IOMMU_FEAT_count); > > > > > > + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); > > > > > > > > > > ... why do we cast to (int) rather than (unsigned int)? Also, I think > > > > > this cast deserve a comment on top because this is not a very obvious > > > > > one. > > > > > > > > > > > /* Does the guest share HAP mapping with the IOMMU? */ > > > > > > bool hap_pt_share; > > > > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h > > > > > > index aea259db1ef2..936e83d333a0 100644 > > > > > > --- a/xen/include/xen/types.h > > > > > > +++ b/xen/include/xen/types.h > > > > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t; > > > > > > typedef __PTRDIFF_TYPE__ ptrdiff_t; > > > > > > +/* Users of this macro are expected to pass a positive value */ > > > > > > #define BITS_TO_LONGS(bits) \ > > > > > > (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) > > > > > > #define DECLARE_BITMAP(name,bits) \ > > > > > > > > > > Cheers, > > > > > > > > See [1] for the reason why I did so. I should have mentioned that in the > > > > commit notes, sorry. > > > > In short, making BITS_TO_LONGS essentially unsigned would cause a > > > > cascade of > > > > build errors and > > > > possibly other essential type violations. > > > Can you share some of the errors? > > > > > > > If this is to be fixed that way, the effort required > > > > is far greater. Either way, a comment on top of can be added, along the > > > > lines of: > > > > > > > > Leaving this as an enum would violate MISRA C:2012 Rule 10.1 > > > > > > I read this as you are simply trying to silence your tool. I think this > > > the > > > wrong approach as you are now making the code confusing for the reader > > > (you > > > pass a signed int to a function that is supposed to take a positive > > > number). > > > > > > I appreciate that this will result to more violations at the beginning. > > > But > > > the whole point of MISRA is to make the code better. > > > > > > If this is too complex to solve now, then a possible option is to deviate > > > for > > > the time being. > > > > 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. So we need either (int) or (unsigned int). The question is which one between the two, and theoretically (unsigned int) is better but due to the reasons above (int) is the simplest choice. Yes, instead of the cast we can also add a SAF-x comment, which refers to a deviation that says something along the lines "we could fix this with a cast but we prefer a deviation because it makes the code easier to read". In general my personal preference would be to use a cast, because we shouldn't implicitly convert enums to integers. However, in this specific case where there is int vs. unsigned int discussion, I can see that also SAF-x could be a way to go.
On 10/10/2023 02:09, Stefano Stabellini wrote: > On Mon, 9 Oct 2023, Julien Grall wrote: >> On 07/10/2023 02:04, Stefano Stabellini wrote: >>> On Fri, 6 Oct 2023, Julien Grall wrote: >>>> Hi Nicola, >>>> >>>> On 06/10/2023 11:10, Nicola Vetrini wrote: >>>>> On 06/10/2023 11:34, Julien Grall wrote: >>>>>> Hi Nicola, >>>>>> >>>>>> On 06/10/2023 09:26, Nicola Vetrini wrote: >>>>>>> Given its use in the declaration >>>>>>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >>>>>>> 'bits' has essential type 'enum iommu_feature', which is not >>>>>>> allowed by the Rule as an operand to the addition operator >>>>>>> in macro 'BITS_TO_LONGS'. >>>>>>> >>>>>>> A comment in BITS_TO_LONGS is added to make it clear that >>>>>>> values passed are meant to be positive. >>>>>> >>>>>> I am confused. If the value is meant to be positive. Then... >>>>>> >>>>>>> >>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>>>> --- >>>>>>> xen/include/xen/iommu.h | 2 +- >>>>>>> xen/include/xen/types.h | 1 + >>>>>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644 >>>>>>> --- a/xen/include/xen/iommu.h >>>>>>> +++ b/xen/include/xen/iommu.h >>>>>>> @@ -360,7 +360,7 @@ struct domain_iommu { >>>>>>> #endif >>>>>>> /* Features supported by the IOMMU */ >>>>>>> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >>>>>>> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >>>>>> >>>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I think >>>>>> this cast deserve a comment on top because this is not a very obvious >>>>>> one. >>>>>> >>>>>>> /* Does the guest share HAP mapping with the IOMMU? */ >>>>>>> bool hap_pt_share; >>>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >>>>>>> index aea259db1ef2..936e83d333a0 100644 >>>>>>> --- a/xen/include/xen/types.h >>>>>>> +++ b/xen/include/xen/types.h >>>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >>>>>>> typedef __PTRDIFF_TYPE__ ptrdiff_t; >>>>>>> +/* Users of this macro are expected to pass a positive value */ >>>>>>> #define BITS_TO_LONGS(bits) \ >>>>>>> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >>>>>>> #define DECLARE_BITMAP(name,bits) \ >>>>>> >>>>>> Cheers, >>>>> >>>>> See [1] for the reason why I did so. I should have mentioned that in the >>>>> commit notes, sorry. >>>>> In short, making BITS_TO_LONGS essentially unsigned would cause a >>>>> cascade of >>>>> build errors and >>>>> possibly other essential type violations. >>>> Can you share some of the errors? >>>> >>>>> If this is to be fixed that way, the effort required >>>>> is far greater. Either way, a comment on top of can be added, along the >>>>> lines of: >>>>> >>>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1 >>>> >>>> I read this as you are simply trying to silence your tool. I think this >>>> the >>>> wrong approach as you are now making the code confusing for the reader >>>> (you >>>> pass a signed int to a function that is supposed to take a positive >>>> number). >>>> >>>> I appreciate that this will result to more violations at the beginning. >>>> But >>>> the whole point of MISRA is to make the code better. >>>> >>>> If this is too complex to solve now, then a possible option is to deviate >>>> for >>>> the time being. >>> >>> 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? > > So we need either (int) or (unsigned int). The question is which one > between the two, and theoretically (unsigned int) is better but due to > the reasons above (int) is the simplest choice. > > Yes, instead of the cast we can also add a SAF-x comment, which refers > to a deviation that says something along the lines "we could fix this > with a cast but we prefer a deviation because it makes the code easier > to read". > > In general my personal preference would be to use a cast, because we > shouldn't implicitly convert enums to integers. See above. I'd like to understand whether we are going to sprinkle the code with cast. If so, I am afraid I am against this solution. Cheers,
>>>> >>>> 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? From a cursory glace I can see equality comparisons and as arguments to the array subscript operator, which are both compliant. >> >> So we need either (int) or (unsigned int). The question is which one >> between the two, and theoretically (unsigned int) is better but due to >> the reasons above (int) is the simplest choice. >> >> Yes, instead of the cast we can also add a SAF-x comment, which refers >> to a deviation that says something along the lines "we could fix this >> with a cast but we prefer a deviation because it makes the code easier >> to read". >> >> In general my personal preference would be to use a cast, because we >> shouldn't implicitly convert enums to integers. > > See above. I'd like to understand whether we are going to sprinkle the > code with cast. If so, I am afraid I am against this solution. > > Cheers,
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. But reply from Stefano suggested that it was necessary anytime we were using 'enum' as an '(unsigned) int'. > From a cursory glace I can see equality comparisons and > as arguments to the array subscript operator, which are both compliant. How about assigning an enum to an '(unsigned) int : X'? (where X is the number of bits. Cheers,
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,
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.
On 10/10/2023 12:53, Julien Grall wrote: > On 10/10/2023 02:09, Stefano Stabellini wrote: >> On Mon, 9 Oct 2023, Julien Grall wrote: >>> On 07/10/2023 02:04, Stefano Stabellini wrote: >>>> On Fri, 6 Oct 2023, Julien Grall wrote: >>>>> Hi Nicola, >>>>> >>>>> On 06/10/2023 11:10, Nicola Vetrini wrote: >>>>>> On 06/10/2023 11:34, Julien Grall wrote: >>>>>>> Hi Nicola, >>>>>>> >>>>>>> On 06/10/2023 09:26, Nicola Vetrini wrote: >>>>>>>> Given its use in the declaration >>>>>>>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument >>>>>>>> 'bits' has essential type 'enum iommu_feature', which is not >>>>>>>> allowed by the Rule as an operand to the addition operator >>>>>>>> in macro 'BITS_TO_LONGS'. >>>>>>>> >>>>>>>> A comment in BITS_TO_LONGS is added to make it clear that >>>>>>>> values passed are meant to be positive. >>>>>>> >>>>>>> I am confused. If the value is meant to be positive. Then... >>>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>>>>> --- >>>>>>>> xen/include/xen/iommu.h | 2 +- >>>>>>>> xen/include/xen/types.h | 1 + >>>>>>>> 2 files changed, 2 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>>>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644 >>>>>>>> --- a/xen/include/xen/iommu.h >>>>>>>> +++ b/xen/include/xen/iommu.h >>>>>>>> @@ -360,7 +360,7 @@ struct domain_iommu { >>>>>>>> #endif >>>>>>>> /* Features supported by the IOMMU */ >>>>>>>> - DECLARE_BITMAP(features, IOMMU_FEAT_count); >>>>>>>> + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); >>>>>>> >>>>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I >>>>>>> think >>>>>>> this cast deserve a comment on top because this is not a very >>>>>>> obvious >>>>>>> one. >>>>>>> >>>>>>>> /* Does the guest share HAP mapping with the IOMMU? */ >>>>>>>> bool hap_pt_share; >>>>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >>>>>>>> index aea259db1ef2..936e83d333a0 100644 >>>>>>>> --- a/xen/include/xen/types.h >>>>>>>> +++ b/xen/include/xen/types.h >>>>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t; >>>>>>>> typedef __PTRDIFF_TYPE__ ptrdiff_t; >>>>>>>> +/* Users of this macro are expected to pass a positive >>>>>>>> value */ >>>>>>>> #define BITS_TO_LONGS(bits) \ >>>>>>>> (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) >>>>>>>> #define DECLARE_BITMAP(name,bits) \ >>>>>>> >>>>>>> Cheers, >>>>>> >>>>>> See [1] for the reason why I did so. I should have mentioned that >>>>>> in the >>>>>> commit notes, sorry. >>>>>> In short, making BITS_TO_LONGS essentially unsigned would cause a >>>>>> cascade of >>>>>> build errors and >>>>>> possibly other essential type violations. >>>>> Can you share some of the errors? >>>>> >>>>>> If this is to be fixed that way, the effort required >>>>>> is far greater. Either way, a comment on top of can be added, >>>>>> along the >>>>>> lines of: >>>>>> >>>>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1 >>>>> >>>>> I read this as you are simply trying to silence your tool. I think >>>>> this >>>>> the >>>>> wrong approach as you are now making the code confusing for the >>>>> reader >>>>> (you >>>>> pass a signed int to a function that is supposed to take a positive >>>>> number). >>>>> >>>>> I appreciate that this will result to more violations at the >>>>> beginning. >>>>> But >>>>> the whole point of MISRA is to make the code better. >>>>> >>>>> If this is too complex to solve now, then a possible option is to >>>>> deviate >>>>> for >>>>> the time being. >>>> >>>> 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? > Re-replying here, since in the other reply I didn't address your concern fully: yes, there are more than a few places where this comes up for Rule 10.1, especially in x86 code. In theory a cast is not the only option to bring the code into compliance, but the specific solution should be checked on a case-by-case basis. The main aim of the series on R10.1 is to deviate or fix the main offenders in terms of violations with as little effort as possible, to have a more manageable analysis result (in my branch, with some patches yet to be submitted I'm down to a few violations on ARM and ~100 on x86).
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 0e747b0bbc1c..34aa0b9b5b81 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -360,7 +360,7 @@ struct domain_iommu { #endif /* Features supported by the IOMMU */ - DECLARE_BITMAP(features, IOMMU_FEAT_count); + DECLARE_BITMAP(features, (int)IOMMU_FEAT_count); /* Does the guest share HAP mapping with the IOMMU? */ bool hap_pt_share; diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index aea259db1ef2..936e83d333a0 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -22,6 +22,7 @@ typedef signed long ssize_t; typedef __PTRDIFF_TYPE__ ptrdiff_t; +/* Users of this macro are expected to pass a positive value */ #define BITS_TO_LONGS(bits) \ (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) #define DECLARE_BITMAP(name,bits) \
Given its use in the declaration 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument 'bits' has essential type 'enum iommu_feature', which is not allowed by the Rule as an operand to the addition operator in macro 'BITS_TO_LONGS'. A comment in BITS_TO_LONGS is added to make it clear that values passed are meant to be positive. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/include/xen/iommu.h | 2 +- xen/include/xen/types.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)