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

[XEN PATCH v2 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: auxiliary definition of pseudo keyword fallthrough
  x86/emul: use pseudo keyword fallthrough
  automation/eclair: tag Rule 16.3 as clean

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

-- 
2.43.0

[XEN PATCH v2 1/3] x86/emul: auxiliary definition of 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
using comments is deprecated).

A definition of such pseudo keyword is already present in the
Xen build. This auxiliary definition makes it available also for
for test and fuzzing harness without iterfearing with the one
that the Xen build has.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes from v1:
- moved definition in the right file;
- remove useless parenthesis;
- description improved.
---
 tools/tests/x86_emulator/x86-emulate.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index 00abc829b0..b01bb0cdce 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -70,6 +70,16 @@
 extern uint32_t mxcsr_mask;
 extern struct cpu_policy cpu_policy;
 
+/*
+ * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
+ * the end of a case statement block.
+ */
+#if !defined(__clang__) && (__GNUC__ >= 7)
+# define fallthrough        __attribute__((__fallthrough__))
+#else
+# define fallthrough        do {} while (0)  /* fallthrough */
+#endif
+
 #define MMAP_SZ 16384
 bool emul_test_init(void);
 
-- 
2.43.0

Re: [XEN PATCH v2 1/3] x86/emul: auxiliary definition of pseudo keyword fallthrough

[Jan Beulich]

On 13.11.2024 09:17, Federico Serafini wrote:
> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -70,6 +70,16 @@
>  extern uint32_t mxcsr_mask;
>  extern struct cpu_policy cpu_policy;
>  
> +/*
> + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at
> + * the end of a case statement block.
> + */
> +#if !defined(__clang__) && (__GNUC__ >= 7)
> +# define fallthrough        __attribute__((__fallthrough__))
> +#else
> +# define fallthrough        do {} while (0)  /* fallthrough */
> +#endif
> +
>  #define MMAP_SZ 16384
>  bool emul_test_init(void);
>  

I'm sure you saw my reply to Stefano where I said "Yes, just with the
addition not at the bottom of the file, but where the other compatibility
definitions are." Yes, you fulfilled the first half, but then you still
put it in the middle of testing-specific definitions / declarations. In
the interest of saving yet another round trip and to avoid yet further
misunderstanding, I'll take care of the further movement while committing.
Then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Federico Serafini]

Make explicit the fallthrough intention by adding the pseudo keyword
where missing and replace fallthrough 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>
---
No changes from v1.
---
 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 v2 2/3] x86/emul: use pseudo keyword fallthrough

[Jan Beulich]

On 13.11.2024 09:17, Federico Serafini wrote:
> Make explicit the fallthrough intention by adding the pseudo keyword
> where missing and replace fallthrough 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>

Repeating my v1 comment:

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

I'm afraid I don't like this "don't care" behavior, yet once again in
the interest of making progress:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[XEN PATCH v2 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>
---
No changes from v1.
---
 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 v2 3/3] automation/eclair: tag Rule 16.3 as clean

[Jan Beulich]

On 13.11.2024 09:17, 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>
> ---
> No changes from v1.
> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Where did Stefano's (conditional) ack go?

Jan

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

[Federico Serafini]

On 13/11/24 10:57, Jan Beulich wrote:
> On 13.11.2024 09:17, 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>
>> ---
>> No changes from v1.
>> ---
>>   automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Where did Stefano's (conditional) ack go?

I missed it, sorry.

-- 
Federico Serafini, M.Sc.

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