| Message ID | 20190212194645.32504-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: crypto: Disable xor-neon build when using clang | expand |
On Tue, 12 Feb 2019 at 20:46, Mark Brown <broonie@kernel.org> wrote: > > Currently clang is able to build an arm64 defconfig or allmodconfig > using the release branch for clang 8 apart from a series of errors in > xor-neon.c in the form: > > arch/arm64/lib/xor-neon.c:27:28: error: incompatible pointer types assigning to 'const unsigned long *' from 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types] > > This issue has been extensively discussed between various interested > parties but it is still unclear what if any the best solution is in the > compiler and there are concerns about just disabling the warning with a > pragma in the header due to the potential for masking other serious > issues. Details of the discussion can be followed here: > > https://github.com/ClangBuiltLinux/linux/issues/283 > > In order to avoid the issue and facilitate further clang work and > testing for the time being just disable the use of xor-neon.c if we are > building with clang. Obviously this is not actually resolving the > problem and is not something that should be with us for the long term > but since it only affects clang and enables people working on that to > work directly with upstream it seems like a useful tradeoff. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > > It would also be possible to achieve the same effect by using either a > Makefile change or a pragma to disable the warning for this specific > file, I can send a patch for those approaches if that's OK and > preferable. Disabling the acceleration entirely seemed safer and less > likely to be forgotten to me. > > We've got some KernelCI support for clang in the middle of getting > deployed: > > https://staging.kernelci.org/job/next-clang/branch/master/ > > so being able to build and run upstream kernels directly would be > enormously helpful. > > arch/arm64/include/asm/xor.h | 2 +- > arch/arm64/lib/Makefile | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/xor.h b/arch/arm64/include/asm/xor.h > index 856386ad076c..971b8336d2d2 100644 > --- a/arch/arm64/include/asm/xor.h > +++ b/arch/arm64/include/asm/xor.h > @@ -14,7 +14,7 @@ > #include <asm/hwcap.h> > #include <asm/neon.h> > > -#ifdef CONFIG_KERNEL_MODE_NEON > +#if defined(CONFIG_KERNEL_MODE_NEON) && !defined(CONFIG_CC_IS_CLANG) > > extern struct xor_block_template const xor_block_inner_neon; > > diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile > index 5540a1638baf..b2a5b8d56354 100644 > --- a/arch/arm64/lib/Makefile > +++ b/arch/arm64/lib/Makefile > @@ -6,10 +6,12 @@ lib-y := clear_user.o delay.o copy_from_user.o \ > strchr.o strrchr.o tishift.o > > ifeq ($(CONFIG_KERNEL_MODE_NEON), y) > +ifneq ($(CONFIG_CC_IS_CLANG), y) > obj-$(CONFIG_XOR_BLOCKS) += xor-neon.o > CFLAGS_REMOVE_xor-neon.o += -mgeneral-regs-only > CFLAGS_xor-neon.o += -ffreestanding > endif > +endif > > # Tell the compiler to treat all general purpose registers (with the > # exception of the IP registers, which are already handled by the caller > -- > 2.20.1 > Can we just add the pragma to neon-instrinsics.h and be done with it? As long as Clang is not the dominant compiler, I don't buy the 'hiding real bugs' argument, especially since neon-intrinsics.h should never be included transitively, so it only affects source files that actually call NEON intrinsics.
On Tue, Feb 12, 2019 at 08:33:57PM +0000, Ard Biesheuvel wrote: > Can we just add the pragma to neon-instrinsics.h and be done with it? > As long as Clang is not the dominant compiler, I don't buy the 'hiding > real bugs' argument, especially since neon-intrinsics.h should never > be included transitively, so it only affects source files that > actually call NEON intrinsics. That's fine from my point of view, when posting this I was basically just going for the most conservative workaround possible.
On Wed, Feb 13, 2019 at 3:40 AM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 12, 2019 at 08:33:57PM +0000, Ard Biesheuvel wrote: > > > Can we just add the pragma to neon-instrinsics.h and be done with it? > > As long as Clang is not the dominant compiler, I don't buy the 'hiding > > real bugs' argument, especially since neon-intrinsics.h should never > > be included transitively, so it only affects source files that > > actually call NEON intrinsics. > > That's fine from my point of view, when posting this I was basically > just going for the most conservative workaround possible. Idealistically, I'm not really happy with either. But pragmatically, if this is the last thing preventing us from turning on KernelCI support for arm64+clang; I'll pick the lesser of two evils, which would be the pragma (vs disabling the whole translation unit). We can always revisit why all of this complexity is going into this particular part of the code later when we have more time. I'm focused right now on finding bugs in llvm asm goto support so that it works well when it lands, and upstreaming lld support, but hopefully we can revisit this more later, when I have more time and energy. Nathan's patch [0] is probably ready to go; with Ard's suggested by tag [1]. Again, I'm not happy about it; but KernelCI coverage is ultimately more important to the project. [0] https://github.com/ClangBuiltLinux/linux/issues/283#issuecomment-457093369 [1] https://github.com/ClangBuiltLinux/linux/issues/283#issuecomment-457087288
On Wed, Feb 13, 2019 at 01:45:08PM -0800, Nick Desaulniers wrote: > Nathan's patch [0] is probably ready to go; with Ard's suggested by > tag [1]. Again, I'm not happy about it; but KernelCI coverage is > ultimately more important to the project. OK, Nathan will you post that?
On Thu, Feb 14, 2019 at 04:30:48PM +0000, Mark Brown wrote: > On Wed, Feb 13, 2019 at 01:45:08PM -0800, Nick Desaulniers wrote: > > > Nathan's patch [0] is probably ready to go; with Ard's suggested by > > tag [1]. Again, I'm not happy about it; but KernelCI coverage is > > ultimately more important to the project. > > OK, Nathan will you post that? Yes, I can do it later today.
diff --git a/arch/arm64/include/asm/xor.h b/arch/arm64/include/asm/xor.h index 856386ad076c..971b8336d2d2 100644 --- a/arch/arm64/include/asm/xor.h +++ b/arch/arm64/include/asm/xor.h @@ -14,7 +14,7 @@ #include <asm/hwcap.h> #include <asm/neon.h> -#ifdef CONFIG_KERNEL_MODE_NEON +#if defined(CONFIG_KERNEL_MODE_NEON) && !defined(CONFIG_CC_IS_CLANG) extern struct xor_block_template const xor_block_inner_neon; diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile index 5540a1638baf..b2a5b8d56354 100644 --- a/arch/arm64/lib/Makefile +++ b/arch/arm64/lib/Makefile @@ -6,10 +6,12 @@ lib-y := clear_user.o delay.o copy_from_user.o \ strchr.o strrchr.o tishift.o ifeq ($(CONFIG_KERNEL_MODE_NEON), y) +ifneq ($(CONFIG_CC_IS_CLANG), y) obj-$(CONFIG_XOR_BLOCKS) += xor-neon.o CFLAGS_REMOVE_xor-neon.o += -mgeneral-regs-only CFLAGS_xor-neon.o += -ffreestanding endif +endif # Tell the compiler to treat all general purpose registers (with the # exception of the IP registers, which are already handled by the caller
Currently clang is able to build an arm64 defconfig or allmodconfig using the release branch for clang 8 apart from a series of errors in xor-neon.c in the form: arch/arm64/lib/xor-neon.c:27:28: error: incompatible pointer types assigning to 'const unsigned long *' from 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types] This issue has been extensively discussed between various interested parties but it is still unclear what if any the best solution is in the compiler and there are concerns about just disabling the warning with a pragma in the header due to the potential for masking other serious issues. Details of the discussion can be followed here: https://github.com/ClangBuiltLinux/linux/issues/283 In order to avoid the issue and facilitate further clang work and testing for the time being just disable the use of xor-neon.c if we are building with clang. Obviously this is not actually resolving the problem and is not something that should be with us for the long term but since it only affects clang and enables people working on that to work directly with upstream it seems like a useful tradeoff. Signed-off-by: Mark Brown <broonie@kernel.org> --- It would also be possible to achieve the same effect by using either a Makefile change or a pragma to disable the warning for this specific file, I can send a patch for those approaches if that's OK and preferable. Disabling the acceleration entirely seemed safer and less likely to be forgotten to me. We've got some KernelCI support for clang in the middle of getting deployed: https://staging.kernelci.org/job/next-clang/branch/master/ so being able to build and run upstream kernels directly would be enormously helpful. arch/arm64/include/asm/xor.h | 2 +- arch/arm64/lib/Makefile | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)