Re: [XEN PATCH 1/3] x86/emul: define pseudo keyword fallthrough

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2024 03:16, Stefano Stabellini wrote:
> On Mon, 11 Nov 2024, Jan Beulich wrote:
>> On 11.11.2024 03:24, Stefano Stabellini wrote:
>>> On Wed, 6 Nov 2024, Jan Beulich wrote:
>>>> On 06.11.2024 10:04, Federico Serafini wrote:
>>>>> The pseudo keyword fallthrough shall be used to make explicit the
>>>>> fallthrough intention at the end of a case statement (doing this
>>>>> through comments is deprecated).
>>>>>
>>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>>> ---
>>>>>  xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> When you had asked my privately on Matrix, I specifically said: "Adding
>>>> the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best,
>>>> unless problems with that approach turn up." Even if identical re-
>>>> definitions are deemed fine, I for one consider such bad practice. Yet
>>>> by playing with this file (and outside of any relevant #ifdef) means
>>>> there will be such a re-definition when building Xen itself.
>>>>
>>>> In fact the patch subject should also already clarify that the auxiliary
>>>> definition is only needed for the test and fuzzing harnesses.
>>>
>>> Hi Jan, I don't understand this comment.
>>>
>>> You say "playing with this file (and outside of any relevant #ifdef)"
>>> but actually the changes are within the #ifndef
>>> __X86_EMULATE_H__/#endif. What do you mean?
>>
>> "relevant" was specifically to exclude the guard #ifdef. And the remark
>> was to avoid the #define to merely be moved into or framed by an
>> "#ifndef __XEN__".
>>
>>> You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h)
>>> is probably best". I am not very familiar with x86-isms but the only
>>> x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h
>>> which is not a header that would help define anything for the Xen build?
>>
>> But that's the whole point: We _have_ "fallthrough" as a pseudo-keyword
>> already for the Xen build. For it to be usable in the emulator files, it
>> particularly needs to be made available for the test and fuzzing
>> harnesses. And that without interfering with what the Xen build has.
>> Hence why it wants to go into precisely that file, where all other build
>> compatibility definitions also live.
> 
> OK. So if I get this right, we need the below instead of patch #1 in
> this series?

Yes, just with the addition not at the bottom of the file, but where the
other compatibility definitions are. Also (nit) perhaps "statement block",
matching terminology in xen/compiler.h.

Jan

> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -233,4 +233,14 @@ void emul_test_put_fpu(
>      enum x86_emulate_fpu_type backout,
>      const struct x86_emul_fpu_aux *aux);
>  
> +/*
> + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
> + * the end of a case statement.
> + */
> +#if (!defined(__clang__) && (__GNUC__ >= 7))
> +# define fallthrough        __attribute__((__fallthrough__))
> +#else
> +# define fallthrough        do {} while (0)  /* fallthrough */
> +#endif
> +
>  #endif /* X86_EMULATE_H */