Re: [XEN PATCH] xen/types: address Rule 10.1 for macro BITS_TO_LONGS

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 05/09/2023 09:31, 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.
> Given that its value can be represented by a signed integer,
> the explicit cast resolves the violation.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
>  xen/include/xen/types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index aea259db1ef2..2ff8f243908d 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -23,7 +23,7 @@ typedef signed long ssize_t;
>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>
>  #define BITS_TO_LONGS(bits) \
> -    (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
> +    (((int)(bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>  #define DECLARE_BITMAP(name,bits) \
>      unsigned long name[BITS_TO_LONGS(bits)]
Revisiting this thread after a while: I did some digging and changing 
the essential type of
BITS_TO_LONGS to unsigned

#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)
#define DECLARE_BITMAP(name,bits) \
    unsigned long name[BITS_TO_LONGS(bits)]

leads to a whole lot of issues due to the extensive usage of these 
macros
(BITS_TO_LONGS, BITS_PER_LONG) in comparisons with e.g. the macros 
min/max.
The comments made in this series to the patch do have value (e.g. 
BITS_TO_LONGS should be
expected to have only a positive argument), but ultimately the changes 
required in order to
do this and respect all other constraints (either in the form of MISRA 
rules or gcc warnings)
is far too broad to be tackled by a single patch.

Notable examples of such consequences:

There is a build error due to -Werror because of a pointer comparison at 
line 469 of common/numa.c:
i = min(PADDR_BITS, BITS_PER_LONG - 1);
where
#define PADDR_BITS              52

if x86's PADDR_BITS becomes unsigned, then other issues arise, such as:

xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);

here the type of flsl is int, so either flsl should become unsigned too, 
or the second
argument should be suitably modified.

In the end, the modification that solves a lot of violations (due to 
this being inside an header, though it's a single place to be modified) 
is this:
DECLARE_BITMAP(features, (int)IOMMU_FEAT_count)

If, as it has been argued, BITS_TO_LONGS really needs to become 
unsigned, then a more general
rethinking of the types involved needs to happen.

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