Re: [XEN PATCH 09/13] xen/public: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.06.2023 12:35, Simone Ballarin wrote:
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -36,11 +36,11 @@
>  typedef unsigned int RING_IDX;
>  
>  /* Round a 32-bit unsigned constant down to the nearest power of two. */
> -#define __RD2(_x)  (((_x) & 0x00000002) ? 0x2                  : ((_x) & 
> 0x1))
> -#define __RD4(_x)  (((_x) & 0x0000000c) ? __RD2((_x)>>2)<<2    : __RD2(_x))
> -#define __RD8(_x)  (((_x) & 0x000000f0) ? __RD4((_x)>>4)<<4    : __RD4(_x))
> -#define __RD16(_x) (((_x) & 0x0000ff00) ? __RD8((_x)>>8)<<8    : __RD8(_x))
> -#define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x))
> +#define __RD2(_x)  (((_x) & 0x00000002U) ? 0x2                  : ((_x) & 
> 0x1))
> +#define __RD4(_x)  (((_x) & 0x0000000cU) ? __RD2((_x)>>2)<<2    : __RD2(_x))
> +#define __RD8(_x)  (((_x) & 0x000000f0U) ? __RD4((_x)>>4)<<4    : __RD4(_x))
> +#define __RD16(_x) (((_x) & 0x0000ff00U) ? __RD8((_x)>>8)<<8    : __RD8(_x))
> +#define __RD32(_x) (((_x) & 0xffff0000U) ? __RD16((_x)>>16)<<16 : __RD16(_x))

While I don't mind the suffixes being added, I'm wondering how
the tool would have spotted the single violation here. Iirc we
don't use this header anywhere in the hypervisor.

If this block is to be touched, I think it wants correcting for
style at the same time: There are numerous blanks missing, and
especially in a public header we shouldn't use underscore-
prefixed names outside of their spec permitted purpose. (This is
about _x; I'm not convinced we can change the various __RD<n>.)

Furthermore, if I recall correctly Misra also mandates single
evaluation of macro arguments. While I don't immediately see how
to address that without resorting to compiler extensions, I don't
think it makes sense to address one violation here but not he
other (the more when the code in question doesn't affect the
hypervisor build).

Jan