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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 20 Jun 2023, Jan Beulich wrote:
> 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).

I am not sure about this one because it is nice to have simple
mechanical changes to review where as a reviewer you are basically only
checking for one thing only.

I think it would be far less effort for reviewers to review additional
MISRA fixes separately. I could understand code style fixes, those could
go either way in my opinion (although I would probably prefer even those
to be done in separate patches if they are non-trivial like the ones
here).

To help Simone, given that our code style is not super-well-documented,
this would be the code style:

#define __RD4(x)  (((x) & 0x0000000cU) ? __RD2((x) >> 2) << 2    : __RD2(x))