[XEN PATCH 0/3] x86: address violations of MISRA C Rule 16.3

[XEN PATCH 0/3] x86: address violations of MISRA C Rule 16.3

[Federico Serafini]

Define pseudo keyword fallthrough for the x86 emulator,
use it and tag the rule as clean.

Federico Serafini (3):
  x86/emul: define pseudo keyword fallthrough
  x86/emul: use pseudo keyword fallthrough
  automation/eclair: tag Rule 16.3 as clean

 automation/eclair_analysis/ECLAIR/tagging.ecl |  3 ++-
 xen/arch/x86/x86_emulate/decode.c             |  6 ++++--
 xen/arch/x86/x86_emulate/x86_emulate.c        |  2 ++
 xen/arch/x86/x86_emulate/x86_emulate.h        | 10 ++++++++++
 4 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.43.0

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

[Federico Serafini]

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@bugseng.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index d75658eba0..f49b1e0dd8 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -23,6 +23,16 @@
 # error Unknown compilation width
 #endif
 
+/*
+ * 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
+
 struct x86_emulate_ctxt;
 
 /*
-- 
2.43.0

[XEN PATCH 2/3] x86/emul: use pseudo keyword fallthrough

[Federico Serafini]

Make explicit the fallthrough intetion by adding the pseudo keyword
where missing and refactor comments not following the agreed syntax.

This satisfies the requirements to deviate violations of
MISRA C:2012 Rule 16.3 "An unconditional break statement shall
terminate every switch-clause".

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/x86_emulate/decode.c      | 6 ++++--
 xen/arch/x86/x86_emulate/x86_emulate.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
index 32b9276dc5..0a0751f2ed 100644
--- a/xen/arch/x86/x86_emulate/decode.c
+++ b/xen/arch/x86/x86_emulate/decode.c
@@ -1356,7 +1356,8 @@ int x86emul_decode(struct x86_emulate_state *s,
                         --disp8scale;
                     break;
                 }
-                /* vcvt{,t}s{s,d}2usi need special casing: fall through */
+                /* vcvt{,t}s{s,d}2usi need special casing. */
+                fallthrough;
             case 0x2c: /* vcvtts{s,d}2si need special casing */
             case 0x2d: /* vcvts{s,d}2si need special casing */
                 if ( evex_encoded() )
@@ -1530,7 +1531,8 @@ int x86emul_decode(struct x86_emulate_state *s,
                         disp8scale -= 1 + (s->evex.pfx == vex_66);
                     break;
                 }
-                /* vcvt{,t}sh2usi needs special casing: fall through */
+                /* vcvt{,t}sh2usi needs special casing. */
+                fallthrough;
             case 0x2c: case 0x2d: /* vcvt{,t}sh2si need special casing */
                 disp8scale = 1;
                 break;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 30674ec301..c38984b201 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1460,6 +1460,7 @@ x86_emulate(
 
         d = (d & ~DstMask) | DstMem;
         /* Becomes a normal DstMem operation from here on. */
+        fallthrough;
     case DstMem:
         generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD);
         if ( state->simd_size != simd_none )
@@ -1942,6 +1943,7 @@ x86_emulate(
             break;
         }
         generate_exception_if((modrm_reg & 7) != 0, X86_EXC_UD);
+        fallthrough;
     case 0x88 ... 0x8b: /* mov */
     case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
     case 0xa2 ... 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */
-- 
2.43.0

[XEN PATCH 3/3] automation/eclair: tag Rule 16.3 as clean

[Federico Serafini]

Tag MISRA C:2012 Rule 16.3 as clean for both architectures:
new violations will cause a failure of the CI/CD pipeline.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
index e1d4ed012a..cb7d5743d2 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -64,6 +64,7 @@ MC3R1.R14.1||
 MC3R1.R14.3||
 MC3R1.R14.4||
 MC3R1.R16.2||
+MC3R1.R16.3||
 MC3R1.R16.7||
 MC3R1.R17.1||
 MC3R1.R17.3||
@@ -112,7 +113,7 @@ if(string_equal(target,"x86_64"),
 )
 
 if(string_equal(target,"arm64"),
-    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.3||MC3R1.R16.6||MC3R1.R20.7"})
+    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
 )
 
 -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
-- 
2.43.0

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

[Jan Beulich]

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@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.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -23,6 +23,16 @@
>  # error Unknown compilation width
>  #endif
>  
> +/*
> + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
> + * the end of a case statement.
> + */
> +#if (!defined(__clang__) && (__GNUC__ >= 7))

I realize xen/compiler.h has it like that, but may I ask that you omit
the meaningless outer pair of parentheses?

Jan

Re: [XEN PATCH 2/3] x86/emul: use pseudo keyword fallthrough

[Jan Beulich]

On 06.11.2024 10:04, Federico Serafini wrote:
> Make explicit the fallthrough intetion by adding the pseudo keyword
> where missing and refactor comments not following the agreed syntax.
> 
> This satisfies the requirements to deviate violations of
> MISRA C:2012 Rule 16.3 "An unconditional break statement shall
> terminate every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

We can certainly take this as is, as a tiny first step. Personally I'm
not overly happy though to see a mix of comment- and pseudo-keyword-
style annotations in individual files. After all going forward we'll
want to use the latter, now that this becomes possible. Yet for that I
view it a more than just helpful if bad precedents didn't exist
anymore. Being the one to touch the emulator files most, I can say
that here more than perhaps anywhere else new code is very often added
by copy-and-paste-then-edit.

Therefore question in particular to the other x86 maintainers: Won't
we be better off if we fully convert to pseudo-keyword-style right
away?

Jan

> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -1356,7 +1356,8 @@ int x86emul_decode(struct x86_emulate_state *s,
>                          --disp8scale;
>                      break;
>                  }
> -                /* vcvt{,t}s{s,d}2usi need special casing: fall through */
> +                /* vcvt{,t}s{s,d}2usi need special casing. */
> +                fallthrough;
>              case 0x2c: /* vcvtts{s,d}2si need special casing */
>              case 0x2d: /* vcvts{s,d}2si need special casing */
>                  if ( evex_encoded() )
> @@ -1530,7 +1531,8 @@ int x86emul_decode(struct x86_emulate_state *s,
>                          disp8scale -= 1 + (s->evex.pfx == vex_66);
>                      break;
>                  }
> -                /* vcvt{,t}sh2usi needs special casing: fall through */
> +                /* vcvt{,t}sh2usi needs special casing. */
> +                fallthrough;
>              case 0x2c: case 0x2d: /* vcvt{,t}sh2si need special casing */
>                  disp8scale = 1;
>                  break;
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1460,6 +1460,7 @@ x86_emulate(
>  
>          d = (d & ~DstMask) | DstMem;
>          /* Becomes a normal DstMem operation from here on. */
> +        fallthrough;
>      case DstMem:
>          generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD);
>          if ( state->simd_size != simd_none )
> @@ -1942,6 +1943,7 @@ x86_emulate(
>              break;
>          }
>          generate_exception_if((modrm_reg & 7) != 0, X86_EXC_UD);
> +        fallthrough;
>      case 0x88 ... 0x8b: /* mov */
>      case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
>      case 0xa2 ... 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */

Re: [XEN PATCH 2/3] x86/emul: use pseudo keyword fallthrough

[Stefano Stabellini]

On Wed, 6 Nov 2024, Federico Serafini wrote:
> Make explicit the fallthrough intetion by adding the pseudo keyword
> where missing and refactor comments not following the agreed syntax.
> 
> This satisfies the requirements to deviate violations of
> MISRA C:2012 Rule 16.3 "An unconditional break statement shall
> terminate every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/x86/x86_emulate/decode.c      | 6 ++++--
>  xen/arch/x86/x86_emulate/x86_emulate.c | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
> index 32b9276dc5..0a0751f2ed 100644
> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -1356,7 +1356,8 @@ int x86emul_decode(struct x86_emulate_state *s,
>                          --disp8scale;
>                      break;
>                  }
> -                /* vcvt{,t}s{s,d}2usi need special casing: fall through */
> +                /* vcvt{,t}s{s,d}2usi need special casing. */
> +                fallthrough;
>              case 0x2c: /* vcvtts{s,d}2si need special casing */
>              case 0x2d: /* vcvts{s,d}2si need special casing */
>                  if ( evex_encoded() )
> @@ -1530,7 +1531,8 @@ int x86emul_decode(struct x86_emulate_state *s,
>                          disp8scale -= 1 + (s->evex.pfx == vex_66);
>                      break;
>                  }
> -                /* vcvt{,t}sh2usi needs special casing: fall through */
> +                /* vcvt{,t}sh2usi needs special casing. */
> +                fallthrough;
>              case 0x2c: case 0x2d: /* vcvt{,t}sh2si need special casing */
>                  disp8scale = 1;
>                  break;
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 30674ec301..c38984b201 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1460,6 +1460,7 @@ x86_emulate(
>  
>          d = (d & ~DstMask) | DstMem;
>          /* Becomes a normal DstMem operation from here on. */
> +        fallthrough;
>      case DstMem:
>          generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD);
>          if ( state->simd_size != simd_none )
> @@ -1942,6 +1943,7 @@ x86_emulate(
>              break;
>          }
>          generate_exception_if((modrm_reg & 7) != 0, X86_EXC_UD);
> +        fallthrough;
>      case 0x88 ... 0x8b: /* mov */
>      case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
>      case 0xa2 ... 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */
> -- 
> 2.43.0
> 

Re: [XEN PATCH 3/3] automation/eclair: tag Rule 16.3 as clean

[Stefano Stabellini]

On Wed, 6 Nov 2024, Federico Serafini wrote:
> Tag MISRA C:2012 Rule 16.3 as clean for both architectures:
> new violations will cause a failure of the CI/CD pipeline.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Pending the first 2 patches being acked:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index e1d4ed012a..cb7d5743d2 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -64,6 +64,7 @@ MC3R1.R14.1||
>  MC3R1.R14.3||
>  MC3R1.R14.4||
>  MC3R1.R16.2||
> +MC3R1.R16.3||
>  MC3R1.R16.7||
>  MC3R1.R17.1||
>  MC3R1.R17.3||
> @@ -112,7 +113,7 @@ if(string_equal(target,"x86_64"),
>  )
>  
>  if(string_equal(target,"arm64"),
> -    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.3||MC3R1.R16.6||MC3R1.R20.7"})
> +    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
>  )
>  
>  -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
> -- 
> 2.43.0
> 

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

[Stefano Stabellini]

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@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?

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?

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


> > --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> > @@ -23,6 +23,16 @@
> >  # error Unknown compilation width
> >  #endif
> >  
> > +/*
> > + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
> > + * the end of a case statement.
> > + */
> > +#if (!defined(__clang__) && (__GNUC__ >= 7))
> 
> I realize xen/compiler.h has it like that, but may I ask that you omit
> the meaningless outer pair of parentheses?
> 
> Jan
> 

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

[Jan Beulich]

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@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.

> 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

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

[Stefano Stabellini]

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@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?


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 */

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

[Jan Beulich]

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@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 */

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

[Federico Serafini]

On 06/11/24 12:22, 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@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.

Sorry Jan, I misinterpreted your message.
I tested the definition in the x86-emulate.h and there are no problems.
I will send a V2.


> In fact the patch subject should also already clarify that the auxiliary
> definition is only needed for the test and fuzzing harnesses.

Ok.


>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -23,6 +23,16 @@
>>   # error Unknown compilation width
>>   #endif
>>   
>> +/*
>> + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
>> + * the end of a case statement.
>> + */
>> +#if (!defined(__clang__) && (__GNUC__ >= 7))
> 
> I realize xen/compiler.h has it like that, but may I ask that you omit
> the meaningless outer pair of parentheses?

Ok.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG
Ph.D. Student, Ca' Foscari University of Venice