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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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