| Message ID | 20190312215203.27643-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: Add '-fno-builtin-bcmp' to CLANG_FLAGS | expand |
On Tue, Mar 12, 2019 at 2:53 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > After LLVM revision r355672 [1], all known working kernel configurations > fail to link [2]: > > ld: init/do_mounts.o: in function `prepare_namespace': > do_mounts.c:(.init.text+0x5ca): undefined reference to `bcmp' > ld: do_mounts.c:(.init.text+0x5e6): undefined reference to `bcmp' > ld: init/initramfs.o: in function `do_header': > initramfs.c:(.init.text+0x6e0): undefined reference to `bcmp' > ld: initramfs.c:(.init.text+0x6f8): undefined reference to `bcmp' > ld: arch/x86/kernel/setup.o: in function `setup_arch': > setup.c:(.init.text+0x21d): undefined reference to `bcmp' > > Commit 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to > builtin") removed '-ffreestanding' globally and the kernel doesn't > provide a bcmp definition so the linker cannot find a reference to it. > > Fix this by explicitly telling LLVM through Clang not to emit bcmp > references. This flag does not need to be behind 'cc-option' because all > working versions of Clang support this flag. > > [1]: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18eedcff13 > [2]: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/104027249 > > Link: https://github.com/ClangBuiltLinux/linux/issues/416 > Link: https://bugs.llvm.org/show_bug.cgi?id=41035 > Cc: stable@vger.kernel.org > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Thanks for this patch. Can the maintainers please consider this an emergency patch; without it, the recent change to LLVM has caused ALL of our CI targets to go red. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile b/Makefile > index 9ef547fc7ffe..6645a274b6e3 100644 > --- a/Makefile > +++ b/Makefile > @@ -501,6 +501,7 @@ ifneq ($(GCC_TOOLCHAIN),) > CLANG_FLAGS += --gcc-toolchain=$(GCC_TOOLCHAIN) > endif > CLANG_FLAGS += -no-integrated-as > +CLANG_FLAGS += -fno-builtin-bcmp > KBUILD_CFLAGS += $(CLANG_FLAGS) > KBUILD_AFLAGS += $(CLANG_FLAGS) > export CLANG_FLAGS > -- > 2.21.0 > -- Thanks, ~Nick Desaulniers
On 12/03/2019 22.52, Nathan Chancellor wrote: > After LLVM revision r355672 [1], all known working kernel configurations > fail to link [2]: > > ld: init/do_mounts.o: in function `prepare_namespace': > do_mounts.c:(.init.text+0x5ca): undefined reference to `bcmp' > ld: do_mounts.c:(.init.text+0x5e6): undefined reference to `bcmp' > ld: init/initramfs.o: in function `do_header': > initramfs.c:(.init.text+0x6e0): undefined reference to `bcmp' > ld: initramfs.c:(.init.text+0x6f8): undefined reference to `bcmp' > ld: arch/x86/kernel/setup.o: in function `setup_arch': > setup.c:(.init.text+0x21d): undefined reference to `bcmp' > > Commit 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to > builtin") removed '-ffreestanding' globally and the kernel doesn't > provide a bcmp definition so the linker cannot find a reference to it. > > Fix this by explicitly telling LLVM through Clang not to emit bcmp > references. This flag does not need to be behind 'cc-option' because all > working versions of Clang support this flag. Wouldn't it be better to just define bcmp as an alias for memcmp? They seem to have compatible prototypes, and then somebody might someday sit down and implement some word-at-a-time version of bcmp making use of the weaker guarantees about the return value to gain some performance. But I suppose that can also be done later. Rasmus
On Wed, Mar 13, 2019 at 5:13 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 12/03/2019 22.52, Nathan Chancellor wrote: > > After LLVM revision r355672 [1], all known working kernel configurations > > fail to link [2]: > > > > ld: init/do_mounts.o: in function `prepare_namespace': > > do_mounts.c:(.init.text+0x5ca): undefined reference to `bcmp' > > ld: do_mounts.c:(.init.text+0x5e6): undefined reference to `bcmp' > > ld: init/initramfs.o: in function `do_header': > > initramfs.c:(.init.text+0x6e0): undefined reference to `bcmp' > > ld: initramfs.c:(.init.text+0x6f8): undefined reference to `bcmp' > > ld: arch/x86/kernel/setup.o: in function `setup_arch': > > setup.c:(.init.text+0x21d): undefined reference to `bcmp' > > > > Commit 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to > > builtin") removed '-ffreestanding' globally and the kernel doesn't > > provide a bcmp definition so the linker cannot find a reference to it. > > > > > Fix this by explicitly telling LLVM through Clang not to emit bcmp > > references. This flag does not need to be behind 'cc-option' because all > > working versions of Clang support this flag. > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > seem to have compatible prototypes, and then somebody might someday sit > down and implement some word-at-a-time version of bcmp making use of the > weaker guarantees about the return value to gain some performance. But I > suppose that can also be done later. I also thought of this.
On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote: > On 12/03/2019 22.52, Nathan Chancellor wrote: > > After LLVM revision r355672 [1], all known working kernel configurations > > fail to link [2]: > > > > ld: init/do_mounts.o: in function `prepare_namespace': > > do_mounts.c:(.init.text+0x5ca): undefined reference to `bcmp' > > ld: do_mounts.c:(.init.text+0x5e6): undefined reference to `bcmp' > > ld: init/initramfs.o: in function `do_header': > > initramfs.c:(.init.text+0x6e0): undefined reference to `bcmp' > > ld: initramfs.c:(.init.text+0x6f8): undefined reference to `bcmp' > > ld: arch/x86/kernel/setup.o: in function `setup_arch': > > setup.c:(.init.text+0x21d): undefined reference to `bcmp' > > > > Commit 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to > > builtin") removed '-ffreestanding' globally and the kernel doesn't > > provide a bcmp definition so the linker cannot find a reference to it. > > > > > Fix this by explicitly telling LLVM through Clang not to emit bcmp > > references. This flag does not need to be behind 'cc-option' because all > > working versions of Clang support this flag. > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > seem to have compatible prototypes, and then somebody might someday sit > down and implement some word-at-a-time version of bcmp making use of the > weaker guarantees about the return value to gain some performance. But I > suppose that can also be done later. > > Rasmus Hi Rasmus, Thank you much for the review, I didn't even realize this was possible :) I'd certainly like to explore it as that is what glibc does. How would you suggest going about it here? Thanks, Nathan
On Wed, Mar 13, 2019 at 2:44 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote: > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > > seem to have compatible prototypes, and then somebody might someday sit > > down and implement some word-at-a-time version of bcmp making use of the > > weaker guarantees about the return value to gain some performance. But I > > suppose that can also be done later. > > Thank you much for the review, I didn't even realize this was possible :) > > I'd certainly like to explore it as that is what glibc does. How would > you suggest going about it here? I suggested a possible implementation (likely contains bugs) and an alias for architectures that require strict alignment, see https://bugs.llvm.org/show_bug.cgi?id=41035#c11 We could start out with just the alias. Arnd
On Wed, Mar 13, 2019 at 02:48:49PM +0100, Arnd Bergmann wrote: > On Wed, Mar 13, 2019 at 2:44 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote: > > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > > > seem to have compatible prototypes, and then somebody might someday sit > > > down and implement some word-at-a-time version of bcmp making use of the > > > weaker guarantees about the return value to gain some performance. But I > > > suppose that can also be done later. > > > > Thank you much for the review, I didn't even realize this was possible :) > > > > I'd certainly like to explore it as that is what glibc does. How would > > you suggest going about it here? > > I suggested a possible implementation (likely contains bugs) and > an alias for architectures that require strict alignment, see > https://bugs.llvm.org/show_bug.cgi?id=41035#c11 > > We could start out with just the alias. > > Arnd So I've been messing around with this for a bit (forgive me, I'm still learning all of the intricacies around here) and this is what I came up with for when __ARCH_HAVE_MEMCMP is unset (not particularly difficult obviously). I can compile, link, and boot an x86 in QEMU. However, I am not sure how to handle memcmp definitions that are written in assembly like arm64, as the alias attribute only works when the alias is defined in the same translation unit. Thoughts? Cheers, Nathan ======================================== diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..69a9daca9179 100644 --- a/lib/string.c +++ b/lib/string.c @@ -864,6 +864,9 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) return res; } EXPORT_SYMBOL(memcmp); + +int bcmp(const void *cs, const void *ct, size_t count) __weak __alias(memcmp); +EXPORT_SYMBOL(bcmp); #endif #ifndef __HAVE_ARCH_MEMSCAN
On Wed, Mar 13, 2019 at 8:32 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Wed, Mar 13, 2019 at 02:48:49PM +0100, Arnd Bergmann wrote: > > On Wed, Mar 13, 2019 at 2:44 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote: > > > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > > > > seem to have compatible prototypes, and then somebody might someday sit > > > > down and implement some word-at-a-time version of bcmp making use of the > > > > weaker guarantees about the return value to gain some performance. But I > > > > suppose that can also be done later. > > > > > > Thank you much for the review, I didn't even realize this was possible :) > > > > > > I'd certainly like to explore it as that is what glibc does. How would > > > you suggest going about it here? > > > > I suggested a possible implementation (likely contains bugs) and > > an alias for architectures that require strict alignment, see > > https://bugs.llvm.org/show_bug.cgi?id=41035#c11 > > > > We could start out with just the alias. > > > > Arnd > > So I've been messing around with this for a bit (forgive me, I'm still > learning all of the intricacies around here) and this is what I came up > with for when __ARCH_HAVE_MEMCMP is unset (not particularly difficult > obviously). I can compile, link, and boot an x86 in QEMU. > > However, I am not sure how to handle memcmp definitions that are written > in assembly like arm64, as the alias attribute only works when the alias > is defined in the same translation unit. Thoughts? I hit this, too: ./arch/arm64/include/asm/string.h:40:15: error: alias must point to a defined variable or function since memcmp is only declared (not defined) in that header, clang is not happy to alias to memcmp. If __HAVE_ARCH_MEMCMP is defined, then we can just return a call to memcmp. Thoughts (I need to add comments above bcmp, anything else)? Do we like the typeof trick (stolen from glibc) or no? diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..e6c1954f2716 100644 --- a/lib/string.c +++ b/lib/string.c @@ -845,7 +845,13 @@ void *memmove(void *dest, const void *src, size_t count) EXPORT_SYMBOL(memmove); #endif -#ifndef __HAVE_ARCH_MEMCMP +#ifdef __HAVE_ARCH_MEMCMP +int bcmp(const void *cs, const void *ct, size_t n) +{ + return memcmp(cs, ct, n); +} +EXPORT_SYMBOL(bcmp); +#else /** * memcmp - Compare two areas of memory * @cs: One area of memory @@ -864,6 +870,8 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) return res; } EXPORT_SYMBOL(memcmp); +__weak __alias(memcmp) typeof(memcmp) bcmp; +EXPORT_SYMBOL(bcmp); #endif #ifndef __HAVE_ARCH_MEMSCAN
On Wed, Mar 13, 2019 at 10:21 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Mar 13, 2019 at 8:32 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Wed, Mar 13, 2019 at 02:48:49PM +0100, Arnd Bergmann wrote: > > > On Wed, Mar 13, 2019 at 2:44 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote: > > > > > Wouldn't it be better to just define bcmp as an alias for memcmp? They > > > > > seem to have compatible prototypes, and then somebody might someday sit > > > > > down and implement some word-at-a-time version of bcmp making use of the > > > > > weaker guarantees about the return value to gain some performance. But I > > > > > suppose that can also be done later. > > > > > > > > Thank you much for the review, I didn't even realize this was possible :) > > > > > > > > I'd certainly like to explore it as that is what glibc does. How would > > > > you suggest going about it here? > > > > > > I suggested a possible implementation (likely contains bugs) and > > > an alias for architectures that require strict alignment, see > > > https://bugs.llvm.org/show_bug.cgi?id=41035#c11 > > > > > > We could start out with just the alias. > > > > > > Arnd > > > > So I've been messing around with this for a bit (forgive me, I'm still > > learning all of the intricacies around here) and this is what I came up > > with for when __ARCH_HAVE_MEMCMP is unset (not particularly difficult > > obviously). I can compile, link, and boot an x86 in QEMU. > > > > However, I am not sure how to handle memcmp definitions that are written > > in assembly like arm64, as the alias attribute only works when the alias > > is defined in the same translation unit. Thoughts? > > I hit this, too: > ./arch/arm64/include/asm/string.h:40:15: error: alias must point to a > defined variable > or function > > since memcmp is only declared (not defined) in that header, clang is > not happy to alias to memcmp. If __HAVE_ARCH_MEMCMP is defined, then > we can just return a call to memcmp. Thoughts (I need to add comments > above bcmp, anything else)? Do we like the typeof trick (stolen from > glibc) or no? > > diff --git a/lib/string.c b/lib/string.c > index 38e4ca08e757..e6c1954f2716 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -845,7 +845,13 @@ void *memmove(void *dest, const void *src, size_t count) > EXPORT_SYMBOL(memmove); > #endif > > -#ifndef __HAVE_ARCH_MEMCMP > +#ifdef __HAVE_ARCH_MEMCMP > +int bcmp(const void *cs, const void *ct, size_t n) > +{ > + return memcmp(cs, ct, n); > +} > +EXPORT_SYMBOL(bcmp); > +#else > /** > * memcmp - Compare two areas of memory > * @cs: One area of memory > @@ -864,6 +870,8 @@ __visible int memcmp(const void *cs, const void > *ct, size_t count) > return res; > } > EXPORT_SYMBOL(memcmp); > +__weak __alias(memcmp) typeof(memcmp) bcmp; > +EXPORT_SYMBOL(bcmp); > #endif > > #ifndef __HAVE_ARCH_MEMSCAN Alternatively, just not worrying about __alias makes this simpler and seems to work (need to add comments, thoughts?): diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..2112108ecc35 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,15 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif +#ifndef __HAVE_ARCH_BCMP +#undef bcmp +int bcmp(const void *cs, const void *ct, size_t count) +{ + return memcmp(cs, ct, count); +} +EXPORT_SYMBOL(bcmp); +#endif + #ifndef __HAVE_ARCH_MEMSCAN /** * memscan - Find a character in an area of memory.
On Wed, Mar 13, 2019 at 6:27 PM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > diff --git a/lib/string.c b/lib/string.c > > index 38e4ca08e757..e6c1954f2716 100644 > > --- a/lib/string.c > > +++ b/lib/string.c > > @@ -845,7 +845,13 @@ void *memmove(void *dest, const void *src, size_t count) > > EXPORT_SYMBOL(memmove); > > #endif > > > > -#ifndef __HAVE_ARCH_MEMCMP > > +#ifdef __HAVE_ARCH_MEMCMP > > +int bcmp(const void *cs, const void *ct, size_t n) > > +{ > > + return memcmp(cs, ct, n); > > +} > > +EXPORT_SYMBOL(bcmp); > > +#else > > /** > > * memcmp - Compare two areas of memory > > * @cs: One area of memory > > @@ -864,6 +870,8 @@ __visible int memcmp(const void *cs, const void > > *ct, size_t count) > > return res; > > } > > EXPORT_SYMBOL(memcmp); > > +__weak __alias(memcmp) typeof(memcmp) bcmp; > > +EXPORT_SYMBOL(bcmp); > > #endif > > > > #ifndef __HAVE_ARCH_MEMSCAN > > Alternatively, just not worrying about __alias makes this simpler and > seems to work (need to add comments, thoughts?): Either way seems fine to me. If we don't plan to provide an optimized version, I'd go with the simpler definition rather than the alias. Arnd
diff --git a/Makefile b/Makefile index 9ef547fc7ffe..6645a274b6e3 100644 --- a/Makefile +++ b/Makefile @@ -501,6 +501,7 @@ ifneq ($(GCC_TOOLCHAIN),) CLANG_FLAGS += --gcc-toolchain=$(GCC_TOOLCHAIN) endif CLANG_FLAGS += -no-integrated-as +CLANG_FLAGS += -fno-builtin-bcmp KBUILD_CFLAGS += $(CLANG_FLAGS) KBUILD_AFLAGS += $(CLANG_FLAGS) export CLANG_FLAGS
After LLVM revision r355672 [1], all known working kernel configurations fail to link [2]: ld: init/do_mounts.o: in function `prepare_namespace': do_mounts.c:(.init.text+0x5ca): undefined reference to `bcmp' ld: do_mounts.c:(.init.text+0x5e6): undefined reference to `bcmp' ld: init/initramfs.o: in function `do_header': initramfs.c:(.init.text+0x6e0): undefined reference to `bcmp' ld: initramfs.c:(.init.text+0x6f8): undefined reference to `bcmp' ld: arch/x86/kernel/setup.o: in function `setup_arch': setup.c:(.init.text+0x21d): undefined reference to `bcmp' Commit 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin") removed '-ffreestanding' globally and the kernel doesn't provide a bcmp definition so the linker cannot find a reference to it. Fix this by explicitly telling LLVM through Clang not to emit bcmp references. This flag does not need to be behind 'cc-option' because all working versions of Clang support this flag. [1]: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18eedcff13 [2]: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/104027249 Link: https://github.com/ClangBuiltLinux/linux/issues/416 Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Cc: stable@vger.kernel.org Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- Makefile | 1 + 1 file changed, 1 insertion(+)