Re: [PATCH] x86/nospec: Fix evaluate_nospec() code generation under Clang

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2022 19:56, Andrew Cooper wrote:
> It turns out that evaluate_nospec() code generation is not safe under Clang.
> Given:
> 
>   void eval_nospec_test(int x)
>   {
>       if ( evaluate_nospec(x) )
>           asm volatile ("nop #true" ::: "memory");
>       else
>           asm volatile ("nop #false" ::: "memory");
>   }
> 
> Clang emits:
> 
>   <eval_nospec_test>:
>          0f ae e8                lfence
>          85 ff                   test   %edi,%edi
>          74 02                   je     <eval_nospec_test+0x9>
>          90                      nop
>          c3                      ret
>          90                      nop
>          c3                      ret
> 
> which is not safe because the lfence has been hoisted above the conditional
> jump.  Clang concludes that both barrier_nospec_true()'s have identical side
> effects and can safely be merged.
> 
> Clang can be persuaded that the side effects are different if there are
> different comments in the asm blocks.  This is fragile, but no more fragile
> that other aspects of this construct.
> 
> Introduce barrier_nospec_false() with a separate internal comment to prevent
> Clang merging it with barrier_nospec_true() despite the otherwise-identical
> content.  The generated code now becomes:
> 
>   <eval_nospec_test>:
>          85 ff                   test   %edi,%edi
>          74 05                   je     <eval_nospec_test+0x9>
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
> 
> which has the correct number of lfence's, and in the correct place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I can live with us going this route, so
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, I'd like alternatives to be considered: Would two asm()s
perhaps not be candidates for merging when they have different
(perhaps fake) arguments or clobbers? If so, would this be less
fragile than relying on comments, which clearly any layer could be
viewed as free to strip off (when the same isn't true for arguments
and clobbers)?

Also you did say you'd open an issue with Clang to try to get their
view on relying on comments here. Could you please add a reference
to that issue in the description here?

Jan