| Message ID | 0a502d2a9c5ce13be13281d9de49d263313b7852.1718117557.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address several violations of MISRA Rule 20.7 | expand |
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@bugseng.com> > --- > 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. Jan
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@bugseng.com> >> --- >> 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.
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@bugseng.com> >>> --- >>> 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. Jan
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@bugseng.com> >>>> --- >>>> 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
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 2d5c1de8ecc2..9352d341346a 100644 --- 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]") \
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@bugseng.com> --- 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. --- xen/arch/x86/x86_emulate/x86_emulate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)