| Message ID | 0fa68b9aee5a7a3f1b696bfc6b18ecc826663212.1725958417.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address violations of MISRA C Rule 16.3 | expand |
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). 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."
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.
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; }
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(+)