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
