Re: [PATCH v2] x86/asm: fix assembly constraints in bitops

[Ingo Molnar]

* Alexander Potapenko <glider@xxxxxxxxxx> wrote:

> 1. Use memory clobber in bitops that touch arbitrary memory
> 
> Certain bit operations that read/write bits take a base pointer and an
> arbitrarily large offset to address the bit relative to that base.
> Inline assembly constraints aren't expressive enough to tell the
> compiler that the assembly directive is going to touch a specific memory
> location of unknown size, therefore we have to use the "memory" clobber
> to indicate that the assembly is going to access memory locations other
> than those listed in the inputs/outputs.
> To indicate that BTR/BTS instructions don't necessarily touch the first
> sizeof(long) bytes of the argument, we also move the address to assembly
> inputs.
> 
> This particular change leads to size increase of 124 kernel functions in
> a defconfig build. For some of them the diff is in NOP operations, other
> end up re-reading values from memory and may potentially slow down the
> execution. But without these clobbers the compiler is free to cache
> the contents of the bitmaps and use them as if they weren't changed by
> the inline assembly.
> 
> 2. Use byte-sized arguments for operations touching single bytes.
> 
> Passing a long value to ANDB/ORB/XORB instructions makes the compiler
> treat sizeof(long) bytes as being clobbered, which isn't the case. This
> may theoretically lead to worse code in the case of heavy optimization.
> 
> Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: James Y Knight <jyknight@xxxxxxxxxx>
> ---
>  v2:
>   -- renamed the patch
>   -- addressed comment by Peter Zijlstra: don't use "+m" for functions
>   returning void
>   -- fixed input types for operations touching single bytes
> ---
>  arch/x86/include/asm/bitops.h | 41 +++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)

I'm wondering what the primary motivation for the patch is:

 - Does it fix an actual miscompilation, or only a theoretical miscompilation?

 - If it fixes an existing miscompilation:

   - Does it fix a miscompilation triggered by current/future versions of GCC?
   - Does it fix a miscompilation triggered by current/future versions of Clang?

 - Also, is the miscompilation triggered by 'usual' kernel configs, or 
   does it require exotics such as weird debug options or GCC plugins, 
   etc?

I.e. a bit more context would be useful.

Thanks,

	Ingo