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.seraf...@bugseng.com>
>>>>> ---
>>>>> 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 */
