Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-06-12 12:36, Jan Beulich wrote:
> On 12.06.2024 11:52, Nicola Vetrini wrote:
> > On 2024-06-12 11:19, Jan Beulich wrote:
> > > On 11.06.2024 17:53, Nicola Vetrini wrote:
> > > > MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> > > > of macro parameters shall be enclosed in parentheses". Therefore, 
> > > > some
> > > > macro definitions should gain additional parentheses to ensure that
> > > > all
> > > > current and future users will be safe with respect to expansions 
> > > > that
> > > > can possibly alter the semantics of the passed-in macro parameter.
> > > >
> > > > No functional change.
> > > >
> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > > ---
> > > > These local helpers could in principle be deviated, but the
> > > > readability
> > > > and functionality are essentially unchanged by complying with the
> > > > rule,
> > > > so I decided to modify the macro definition as the preferred option.
> > > Well, yes, but ...
> > >
> > > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > > > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > > > @@ -2255,7 +2255,7 @@ x86_emulate(
> > > >          switch ( modrm_reg & 7 )
> > > >          {
> > > >  #define GRP2(name, ext) \
> > > > -        case ext: \
> > > > +        case (ext): \
> > > >              if ( ops->rmw && dst.type == OP_MEM ) \
> > > >                  state->rmw = rmw_##name; \
> > > >              else \
> > > > @@ -8611,7 +8611,7 @@ int x86_emul_rmw(
> > > >              unsigned long dummy;
> > > >
> > > >  #define XADD(sz, cst, mod) \
> > > > -        case sz: \
> > > > +        case (sz): \
> > > >              asm ( "" \
> > > >                    COND_LOCK(xadd) " %"#mod"[reg], %[mem]; " \
> > > >                    _POST_EFLAGS("[efl]", "[msk]", "[tmp]") \
> > > ... this is really nitpicky of the rule / tool. What halfway 
> > > realistic
> > > ways do you see to actually misuse these macros? What follows the
> > > "case"
> > > keyword is just an expression; no precedence related issues are
> > > possible.
> > I do share the view: no real danger is possible in sensible uses. 
> > Often
> > MISRA rules are stricter than necessary to have a simple formulation, 
> > by
> > avoiding too many special cases.
> >
> > However, if a deviation is formulated, then it needs to be maintained,
> > for no real readability benefit in this case, in my opinion. I can be
> > convinced otherwise, of course.
> Well, aiui you're thinking of a per-macro deviation here. Whereas I'd 
> be
> thinking of deviating the pattern.
Might be reasonable. I'll think about it for v2

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