Re: [XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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