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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?


diff --git a/tools/tests/x86_emulator/x86-emulate.h 
b/tools/tests/x86_emulator/x86-emulate.h
index 00abc829b0..380eb8abff 100644
--- 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 */