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

[Jan Beulich <jbeulich@xxxxxxxx>]

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