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

[Alexander Potapenko <glider@google.com>]

On Fri, Apr 5, 2019 at 11:39 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Alexander Potapenko <glider@google.com> 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@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Paul E. McKenney <paulmck@linux.ibm.com>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: James Y Knight <jyknight@google.com>
> > ---
> >  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?
Depends on what we name an actual miscompilation.
I've built a defconfig kernel and looked through some of the functions
generated by GCC 7.3.0 with and without this clobber, and didn't spot
any miscompilations.
However there is a (trivial) theoretical case where this code leads to
miscompilation: https://lkml.org/lkml/2019/3/28/393 using just GCC
8.3.0 with -O2.
It isn't hard to imagine someone writes such a function in the kernel someday.

So the primary motivation is to fix an existing misuse of the asm
directive, which happens to work in certain configurations now, but
isn't guaranteed to work under different circumstances.

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



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg