| Message ID | 20171103171203.107569-9-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I think this bug was fixed upstream in LLVM. Do we still want to take this workaround? On Fri, Nov 3, 2017 at 10:11 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > From: Greg Hackmann <ghackmann@google.com> > > LLVM bug 30792 causes clang's AArch64 backend to crash compiling > arch/arm64/crypto/aes-ce-cipher.c. Replacing -mgeneral-regs-only with > -mno-implicit-float is the suggested workaround. > > Signed-off-by: Greg Hackmann <ghackmann@google.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > --- > arch/arm64/Makefile | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > index 939b310913cf..eb6f3c9ec6cb 100644 > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -45,7 +45,13 @@ $(warning Detected assembler with broken .inst; disassembly will be unreliable) > endif > endif > > -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) > +ifeq ($(cc-name),clang) > +# This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792. > +KBUILD_CFLAGS += -mno-implicit-float > +else > +KBUILD_CFLAGS += -mgeneral-regs-only > +endif > +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) > KBUILD_CFLAGS += -fno-asynchronous-unwind-tables > KBUILD_CFLAGS += $(call cc-option, -mpc-relative-literal-loads) > KBUILD_AFLAGS += $(lseinstr) $(brokengasinst) > -- > 2.15.0.403.gc27cc4dac6-goog >
On Fri, Nov 03, 2017 at 10:11:52AM -0700, Sami Tolvanen wrote: > From: Greg Hackmann <ghackmann@google.com> > > LLVM bug 30792 causes clang's AArch64 backend to crash compiling > arch/arm64/crypto/aes-ce-cipher.c. Replacing -mgeneral-regs-only with > -mno-implicit-float is the suggested workaround. > > Signed-off-by: Greg Hackmann <ghackmann@google.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> Ah, so I guess this is what I was hitting when testing with clang 5.0.0. I was under the impression that this series was jsut enablnig LTO support, not clang support generally. It would be much nicer if we could depend on a fixed clang to start with... Thanks, Mark. > --- > arch/arm64/Makefile | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > index 939b310913cf..eb6f3c9ec6cb 100644 > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -45,7 +45,13 @@ $(warning Detected assembler with broken .inst; disassembly will be unreliable) > endif > endif > > -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) > +ifeq ($(cc-name),clang) > +# This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792. > +KBUILD_CFLAGS += -mno-implicit-float > +else > +KBUILD_CFLAGS += -mgeneral-regs-only > +endif > +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) > KBUILD_CFLAGS += -fno-asynchronous-unwind-tables > KBUILD_CFLAGS += $(call cc-option, -mpc-relative-literal-loads) > KBUILD_AFLAGS += $(lseinstr) $(brokengasinst) > -- > 2.15.0.403.gc27cc4dac6-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Nov 3, 2017 at 11:02 AM, Mark Rutland <mark.rutland@arm.com> wrote: > Ah, so I guess this is what I was hitting when testing with clang 5.0.0. Exactly. > I was under the impression that this series was jsut enablnig LTO > support, not clang support generally. Clang is supported at least for the arm64*/x86-64 configs that we've gotten around to testing. * minus this one last compiler bug (but maybe "last" is optimistic). > It would be much nicer if we could depend on a fixed clang to start > with... I agree, which is why I'm on the fence with this patch. With a newer version of Clang, it's not needed. There are some kbuild macros for certain versions of GCC, maybe something like that can be used to conditionally swap these flags if using an older version of Clang? >> +# This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792. >> +KBUILD_CFLAGS += -mno-implicit-float >> +else >> +KBUILD_CFLAGS += -mgeneral-regs-only >> +endif
On Fri, Nov 03, 2017 at 10:11:52AM -0700, Sami Tolvanen wrote: > From: Greg Hackmann <ghackmann@google.com> > > LLVM bug 30792 causes clang's AArch64 backend to crash compiling > arch/arm64/crypto/aes-ce-cipher.c. Replacing -mgeneral-regs-only with > -mno-implicit-float is the suggested workaround. > > Signed-off-by: Greg Hackmann <ghackmann@google.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> Just to check, what happens if you pass both to clang? If it works when you pass both... > -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) > +ifeq ($(cc-name),clang) > +# This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792. > +KBUILD_CFLAGS += -mno-implicit-float > +else > +KBUILD_CFLAGS += -mgeneral-regs-only > +endif ... then this can be reduced to: # This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792 KBUILD_CFLAGS += $(call cc-option, -mno-implicit-float) Thanks, Mark. > +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) > KBUILD_CFLAGS += -fno-asynchronous-unwind-tables > KBUILD_CFLAGS += $(call cc-option, -mpc-relative-literal-loads) > KBUILD_AFLAGS += $(lseinstr) $(brokengasinst) > -- > 2.15.0.403.gc27cc4dac6-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Nov 03, 2017 at 06:31:56PM +0000, Mark Rutland wrote: > On Fri, Nov 03, 2017 at 10:11:52AM -0700, Sami Tolvanen wrote: > > From: Greg Hackmann <ghackmann@google.com> > > > > LLVM bug 30792 causes clang's AArch64 backend to crash compiling > > arch/arm64/crypto/aes-ce-cipher.c. Replacing -mgeneral-regs-only with > > -mno-implicit-float is the suggested workaround. > > > > Signed-off-by: Greg Hackmann <ghackmann@google.com> > > Cc: Matthias Kaehlcke <mka@chromium.org> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Just to check, what happens if you pass both to clang? Apparently not. However, we can do: -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) +# Clang workaround for https://bugs.llvm.org/show_bug.cgi?id=30792 +KBUILD_CFLAGS += $(call cc-option, -mno-implicit-float, -mgeneral-regs-only) Thanks, Mark.
On Fri, Nov 3, 2017 at 11:52 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Nov 03, 2017 at 06:31:56PM +0000, Mark Rutland wrote: >> On Fri, Nov 03, 2017 at 10:11:52AM -0700, Sami Tolvanen wrote: >> > From: Greg Hackmann <ghackmann@google.com> >> > >> > LLVM bug 30792 causes clang's AArch64 backend to crash compiling >> > arch/arm64/crypto/aes-ce-cipher.c. Replacing -mgeneral-regs-only with >> > -mno-implicit-float is the suggested workaround. >> > >> > Signed-off-by: Greg Hackmann <ghackmann@google.com> >> > Cc: Matthias Kaehlcke <mka@chromium.org> >> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> >> >> Just to check, what happens if you pass both to clang? > > Apparently not. However, we can do: > > -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) > +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) > +# Clang workaround for https://bugs.llvm.org/show_bug.cgi?id=30792 > +KBUILD_CFLAGS += $(call cc-option, -mno-implicit-float, -mgeneral-regs-only) Should a clang version test be included, since we know at least 5.0 is need (with this fix)? There are distros with much earlier versions of clang, for example... -Kees
On Fri, Nov 03, 2017 at 12:06:15PM -0700, Kees Cook wrote: > Should a clang version test be included, since we know at least 5.0 is > need (with this fix)? There are distros with much earlier versions of > clang, for example... Greg knows better, but I remember him mentioning that upstream clang doesn't generate any worse code with -mno-implicit-float, so there should be no harm in using this workaround with all versions. This might change in future, of course, so I don't have objections to adding a check for clang version < 6.0 here. Sami
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 939b310913cf..eb6f3c9ec6cb 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -45,7 +45,13 @@ $(warning Detected assembler with broken .inst; disassembly will be unreliable) endif endif -KBUILD_CFLAGS += -mgeneral-regs-only $(lseinstr) $(brokengasinst) +ifeq ($(cc-name),clang) +# This is a workaround for https://bugs.llvm.org/show_bug.cgi?id=30792. +KBUILD_CFLAGS += -mno-implicit-float +else +KBUILD_CFLAGS += -mgeneral-regs-only +endif +KBUILD_CFLAGS += $(lseinstr) $(brokengasinst) KBUILD_CFLAGS += -fno-asynchronous-unwind-tables KBUILD_CFLAGS += $(call cc-option, -mpc-relative-literal-loads) KBUILD_AFLAGS += $(lseinstr) $(brokengasinst)