Re: [tip:x86/urgent] x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work around Clang incompatibility

[Matthias Kaehlcke]

Hi Peter,

El Fri, May 05, 2017 at 12:37:23PM -0700 hpa@xxxxxxxxx ha dit:

> On May 5, 2017 11:44:05 AM PDT, Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
> >El Fri, May 05, 2017 at 07:50:39PM +0200 Ingo Molnar ha dit:
> >
> >> 
> >> * Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >> 
> >> > On Fri, May 05, 2017 at 01:11:47AM -0700, tip-bot for Matthias
> >Kaehlcke wrote:
> >> > > Commit-ID:  121843eb02a6e2fa30aefab64bfe183c97230c75
> >> > > Gitweb:    
> >http://git.kernel.org/tip/121843eb02a6e2fa30aefab64bfe183c97230c75
> >> > > Author:     Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> >> > > AuthorDate: Mon, 1 May 2017 15:47:41 -0700
> >> > > Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
> >> > > CommitDate: Fri, 5 May 2017 08:31:05 +0200
> >> > > 
> >> > > x86/mm/kaslr: Use the _ASM_MUL macro for multiplication to work
> >around Clang incompatibility
> >> > > 
> >> > > The constraint "rm" allows the compiler to put mix_const into
> >memory.
> >> > > When the input operand is a memory location then MUL needs an
> >operand
> >> > > size suffix, since Clang can't infer the multiplication width
> >from the
> >> > > operand.
> >> > 
> >> > *sigh*, this is another shining example of how LLVM is a better,
> >faster
> >> > moving compiler?
> >> 
> >> Well, I don't like it - but we already have similar patterns to cover
> >some asm 
> >> complications so I didn't mind. Apparently Clang is very close to
> >being able to 
> >> build a working Linux kernel, right?
> >
> >Indeed, I expect 4.12 (with this patch ...) to build with Clang for a
> >x86 defconfig (with tons of warnings). ARM64 is very close.
> >
> >> In that sense it would be unfair to expect it to not have various
> >legacies, 
> >> missing features and quirks - just like the kernel has dozens of GCC
> >related 
> >> workarounds.
> >
> >Also my understanding is that this isn't really a clang issue. In the
> >context of this code gcc apparently chooses to use a register for
> >'mix_const', for memory locations it also needs a suffix.
> >
> >Actually I just tried to build this code from a single C file:
> >
> >void test() {
> >  unsigned long raw, random;
> >  const unsigned long mix_const = 0x3f39e593UL;
> >
> >  asm("MUL %3"
> >    : "=a" (random), "=d" (raw)
> >    : "a" (random), "rm" (mix_const));
> >}
> >
> >gcc -c /tmp/test.c
> >/tmp/test.c: Assembler messages:
> >/tmp/test.c:6: Error: no instruction mnemonic suffix given and no
> >  register operands; can't size instruction
> >
> >gcc version 4.9.x 20150123
> >
> >Cheers
> >
> >Matthias
> 
> Yes, this its a bug regardless of clang or not. It just happens to be hittin by the particular optimization choice is the current versions of gcc makes.
> 
> I asked that that be clear in the commit message, but by the time it was merged it has gotten muddled again.
> 
> However, the clang team also need to accept that they can't do
> arbitrary hacks in the kernel when their compiler is inadequate.

AFAIK there isn't really such a thing as "the clang team" (anymore?),
or at least they don't invite me to their parties :)

Overall I totally agree that we should avoid hacks to support clang.
Short-comings in clang that require ugly or wide-spread hacks should
be fixed in the compiler and out-of-tree patches can be used as
workaround.

However there may be cases where minor innocuous changes are needed
to enable clang (this one could be an example if it was really a
clang issue) and I hope we can be pragmatic in these situations,
especially since these are/seem the last missing bits to support the
other big open source compiler out there.

Cheers

Matthias