Address violations of MISRA C:2012 Rule 16.3:
"An unconditional `break' statement shall terminate every
switch-clause".
No functional change.
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
xen/arch/x86/x86_emulate/fpu.c | 4 ++++
xen/arch/x86/x86_emulate/x86_emulate.c | 1 +
2 files changed, 5 insertions(+)
diff --git a/xen/arch/x86/x86_emulate/fpu.c b/xen/arch/x86/x86_emulate/fpu.c
index 480d879657..3351f549e7 100644
--- a/xen/arch/x86/x86_emulate/fpu.c
+++ b/xen/arch/x86/x86_emulate/fpu.c
@@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
*/
if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
dst->type = OP_NONE;
+ break;
}
break;
@@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
default:
generate_exception(X86_EXC_UD);
}
+ break;
}
break;
@@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
*/
if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
dst->type = OP_NONE;
+ break;
}
break;
@@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
case 7: /* fistp m64i */
goto fpu_memdst64;
}
+ break;
}
break;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 2b03d65fce..9fb316e0d7 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8297,6 +8297,7 @@ x86_emulate(
}
if ( rc != 0 )
goto done;
+ break;
default:
break;
}
--
2.34.1On 10.09.2024 12:09, Federico Serafini wrote: > --- a/xen/arch/x86/x86_emulate/fpu.c > +++ b/xen/arch/x86/x86_emulate/fpu.c > @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s, > */ > if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() ) > dst->type = OP_NONE; > + break; > } > break; > > @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s, > default: > generate_exception(X86_EXC_UD); > } > + break; > } > break; > > @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s, > */ > if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() ) > dst->type = OP_NONE; > + break; > } > break; > > @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s, > case 7: /* fistp m64i */ > goto fpu_memdst64; > } > + break; Aren't you swapping one violation for another here? Unlike in the earlier three cases, this new break is unreachable, because of the nature of the preceding switch() statement (cases being exhaustive and every case ending in "goto"; this is something even a static analyzer can [in principle] spot). Jan
On 11/09/24 14:42, Jan Beulich wrote:
> On 10.09.2024 12:09, Federico Serafini wrote:
>> --- a/xen/arch/x86/x86_emulate/fpu.c
>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>> */
>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>> dst->type = OP_NONE;
>> + break;
>> }
>> break;
>>
>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>> default:
>> generate_exception(X86_EXC_UD);
>> }
>> + break;
>> }
>> break;
>>
>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>> */
>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>> dst->type = OP_NONE;
>> + break;
>> }
>> break;
>>
>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>> case 7: /* fistp m64i */
>> goto fpu_memdst64;
>> }
>> + break;
>
> Aren't you swapping one violation for another here? Unlike in the earlier
> three cases, this new break is unreachable, because of the nature of the
> preceding switch() statement (cases being exhaustive and every case ending
> in "goto"; this is something even a static analyzer can [in principle]
> spot).
You are right, but the resulting violation of Rule 2.1
("A project shall not contain unreachable code") is deviated with the
following justification:
"The compiler implementation guarantees that the unreachable code is
removed. Constant expressions and unreachable branches of if and switch
statements are expected."
--
Federico Serafini,
Ph.D. Student, Ca' Foscari University of Venice
Software Engineer, BUGSENG
On 12.09.2024 11:17, Federico Serafini wrote:
> On 11/09/24 14:42, Jan Beulich wrote:
>> On 10.09.2024 12:09, Federico Serafini wrote:
>>> --- a/xen/arch/x86/x86_emulate/fpu.c
>>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>> */
>>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>> dst->type = OP_NONE;
>>> + break;
>>> }
>>> break;
>>>
>>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>> default:
>>> generate_exception(X86_EXC_UD);
>>> }
>>> + break;
>>> }
>>> break;
>>>
>>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>> */
>>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>> dst->type = OP_NONE;
>>> + break;
>>> }
>>> break;
>>>
>>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>> case 7: /* fistp m64i */
>>> goto fpu_memdst64;
>>> }
>>> + break;
>>
>> Aren't you swapping one violation for another here? Unlike in the earlier
>> three cases, this new break is unreachable, because of the nature of the
>> preceding switch() statement (cases being exhaustive and every case ending
>> in "goto"; this is something even a static analyzer can [in principle]
>> spot).
>
> You are right, but the resulting violation of Rule 2.1
> ("A project shall not contain unreachable code") is deviated with the
> following justification:
> "The compiler implementation guarantees that the unreachable code is
> removed.
I'm not convinced this is the case here in practice.
Instead of "break", wouldn't "unreachable()" be the better construct
to use in situations like this one?
> Constant expressions and unreachable branches of if and switch
> statements are expected."
This I don't think applies in this particular case?
Jan
On 12/09/24 12:01, Jan Beulich wrote:
> On 12.09.2024 11:17, Federico Serafini wrote:
>> On 11/09/24 14:42, Jan Beulich wrote:
>>> On 10.09.2024 12:09, Federico Serafini wrote:
>>>> --- a/xen/arch/x86/x86_emulate/fpu.c
>>>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>>>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>> */
>>>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>> dst->type = OP_NONE;
>>>> + break;
>>>> }
>>>> break;
>>>>
>>>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>> default:
>>>> generate_exception(X86_EXC_UD);
>>>> }
>>>> + break;
>>>> }
>>>> break;
>>>>
>>>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>> */
>>>> if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>> dst->type = OP_NONE;
>>>> + break;
>>>> }
>>>> break;
>>>>
>>>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>> case 7: /* fistp m64i */
>>>> goto fpu_memdst64;
>>>> }
>>>> + break;
>>>
>>> Aren't you swapping one violation for another here? Unlike in the earlier
>>> three cases, this new break is unreachable, because of the nature of the
>>> preceding switch() statement (cases being exhaustive and every case ending
>>> in "goto"; this is something even a static analyzer can [in principle]
>>> spot).
>>
>> You are right, but the resulting violation of Rule 2.1
>> ("A project shall not contain unreachable code") is deviated with the
>> following justification:
>> "The compiler implementation guarantees that the unreachable code is
>> removed.
>
> I'm not convinced this is the case here in practice.
>
> Instead of "break", wouldn't "unreachable()" be the better construct
> to use in situations like this one?
>
>> Constant expressions and unreachable branches of if and switch
>> statements are expected."
>
> This I don't think applies in this particular case?
I agree,
the ECLAIR configuration for the deviation covers
more cases than expected. I'll fix it.
--
Federico Serafini,
Ph.D. Student, Ca' Foscari University of Venice
Software Engineer, BUGSENG
© 2016 - 2026 Red Hat, Inc.