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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> I am not understanding your suggestions. From what I can see this patch
> looks OK?

No, it is - first and foremost - the wrong file that is being touched.

Jan