1. Patchew
  2. Xen
  3. [XEN PATCH 0/6] address several violations of MISRA Rule 20.7
  4. View patch

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

Nicola Vetrini posted 6 patches 5 months, 2 weeks ago
There is a newer version of this series
[XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7
Posted by Nicola Vetrini 5 months, 2 weeks ago 
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(-)

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]") \
-- 
2.34.1
Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7
Posted by Jan Beulich 5 months, 2 weeks ago 
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
Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7
Posted by Nicola Vetrini 5 months, 2 weeks ago 
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.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7
Posted by Jan Beulich 5 months, 2 weeks ago 
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
Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7
Posted by Nicola Vetrini 5 months, 2 weeks ago 
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

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.