The string literal inside the expansion of BUILD_BUG_ON is considered
unreachable code; however, such statement can be moved earlier
with no functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The motivation for this code movement is that keeping it inside the switch
statement matches MISRA's definition of unreachable code, but does not fall into
the category of declarations without initialization, which is already a deviated
aspect. An alternative approach would be to deviate BUILD_BUG_ON as well.
---
 xen/arch/x86/xstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index cf94761d0542..99f0526c8988 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask)
      */
     for ( prev_faults = faults = 0; ; prev_faults = faults )
     {
+        BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
+
         switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
         {
-            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
 #define _xrstor(insn) \
         asm volatile ( "1: .byte " insn "\n" \
                        "3:\n" \
-- 
2.34.1

On 11.12.2023 11:30, Nicola Vetrini wrote:
> The string literal inside the expansion of BUILD_BUG_ON is considered
> unreachable code; however, such statement can be moved earlier
> with no functional change.

First: Why is this deemed dead code in its present position, but okay when
moved? Second: While moving is indeed no functional change (really
BUILD_BUG_ON() can be moved about anywhere, for not producing any code in
the final binary), it removes the connection between it and the respective
asm() (where %z would have been nice to use).

Jan

On 12.12.2023 11:04, Jan Beulich wrote:
> On 11.12.2023 11:30, Nicola Vetrini wrote:
>> The string literal inside the expansion of BUILD_BUG_ON is considered
>> unreachable code; however, such statement can be moved earlier
>> with no functional change.
> 
> First: Why is this deemed dead code in its present position, but okay when
> moved? Second: While moving is indeed no functional change (really
> BUILD_BUG_ON() can be moved about anywhere, for not producing any code in
> the final binary), it removes the connection between it and the respective
> asm() (where %z would have been nice to use).

Oh, and third: Which string literal? I expect you're not building with
an ancient compiler, so it got to be

#define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })

which you see in use. Yet that string literal isn't "code" or "data", but
an argument to _Static_assert(). Is Eclair perhaps not properly aware of
_Static_assert()?

Jan

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> The string literal inside the expansion of BUILD_BUG_ON is considered
> unreachable code; however, such statement can be moved earlier
> with no functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> The motivation for this code movement is that keeping it inside the switch
> statement matches MISRA's definition of unreachable code, but does not fall into
> the category of declarations without initialization, which is already a deviated
> aspect. An alternative approach would be to deviate BUILD_BUG_ON as well.

I think that deviating BUILD_BUG_ON would be totally fine. But given
that this patch is obviously correct:

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


> ---
>  xen/arch/x86/xstate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index cf94761d0542..99f0526c8988 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask)
>       */
>      for ( prev_faults = faults = 0; ; prev_faults = faults )
>      {
> +        BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
> +
>          switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>          {
> -            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
>  #define _xrstor(insn) \
>          asm volatile ( "1: .byte " insn "\n" \
>                         "3:\n" \
> -- 
> 2.34.1
>