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/8e16d73346f8091461319a7dfc4ddd18... [2]: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/10402724...
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(+)
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
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.
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.
ArndSo 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.
ArndSo 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.
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed: * A weak alias was discussed, but breaks for architectures that define their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly. * -ffreestanding also is used sporadically throughout the kernel. * -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com --- lib/string.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..5b2377d7143f 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,21 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/** + * bcmp - Like memcmp but the return code simply indicates a non-match. + * @cs: One area of memory. + * @ct: Another area of memory. + * @count: The size of the areas. + */ +#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 11:02 AM Nick Desaulniers ndesaulniers@google.com wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com
lib/string.c | 15 +++++++++++++++
ah, looks like I should provide a declaration in include/linux/string.h, too, in case others within the kernel would like to use bcmp. WIll send a v2.
1 file changed, 15 insertions(+)
diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..5b2377d7143f 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,21 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - Like memcmp but the return code simply indicates a non-match.
- @cs: One area of memory.
- @ct: Another area of memory.
- @count: The size of the areas.
- */
+#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.
-- 2.21.0.360.g471c308f928-goog
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed: * A weak alias was discussed, but breaks for architectures that define their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly. * -ffreestanding also is used sporadically throughout the kernel. * -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com --- Changes V1 -> V2: * Add declaration to include/linux/string.h. * Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..b882692bac04 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,21 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/** + * bcmp - Like memcmp but a non-zero return code simply indicates a non-match. + * @cs: One area of memory. + * @ct: Another area of memory. + * @count: The size of the areas. + */ +#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, 13 Mar 2019 11:17:15 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - Like memcmp but a non-zero return code simply indicates a non-match.
- @cs: One area of memory.
- @ct: Another area of memory.
- @count: The size of the areas.
- */
+#undef bcmp +int bcmp(const void *cs, const void *ct, size_t count) +{
- return memcmp(cs, ct, count);
This is confusing where the comment says "like memcmp but .." and then just returns memcmp() unmodified. If anything, I would expect to see
return !!memcmp(cs, ct, conut);
or have a better comment explaining why its the same.
-- Steve
+} +EXPORT_SYMBOL(bcmp); +#endif
On Wed, Mar 13, 2019 at 11:40 AM Steven Rostedt rostedt@goodmis.org wrote:
On Wed, 13 Mar 2019 11:17:15 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - Like memcmp but a non-zero return code simply indicates a non-match.
- @cs: One area of memory.
- @ct: Another area of memory.
- @count: The size of the areas.
- */
+#undef bcmp +int bcmp(const void *cs, const void *ct, size_t count) +{
return memcmp(cs, ct, count);This is confusing where the comment says "like memcmp but .." and then just returns memcmp() unmodified. If anything, I would expect to see
return !!memcmp(cs, ct, conut);
That's more work than strictly needed. memcmp already provides the semantics of bcmp. memcmp just provides more meaning to the signedness of the return code, whereas bcmp does not.
or have a better comment explaining why its the same.
I could add something about "the signedness of the return code not providing any meaning." What would you like to see in such a comment?
On Wed, 13 Mar 2019 11:51:09 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
This is confusing where the comment says "like memcmp but .." and then just returns memcmp() unmodified. If anything, I would expect to see
return !!memcmp(cs, ct, conut);That's more work than strictly needed. memcmp already provides the semantics of bcmp. memcmp just provides more meaning to the signedness of the return code, whereas bcmp does not.
I figured you would say as much ;-)
or have a better comment explaining why its the same.
I could add something about "the signedness of the return code not providing any meaning." What would you like to see in such a comment?
I think it's the wording that bothers me:
+ * bcmp - Like memcmp but a non-zero return code simply indicates a non-match.
What about:
* bcmp - Like memcmp but non-zero only means a non-match
Then in the description say that bcmp() callers must not expect anything more than zero and non-zero, as different implementations only need to return non-zero for non matches. The non-zero has no other meaning like it does in memcmp(). You could add that memcmp() itself is one implementation of bcmp() but not vice versa.
-- Steve
On 13/03/2019 20.01, Steven Rostedt wrote:
On Wed, 13 Mar 2019 11:51:09 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
or have a better comment explaining why its the same.
I could add something about "the signedness of the return code not providing any meaning." What would you like to see in such a comment?
I think it's the wording that bothers me:
- bcmp - Like memcmp but a non-zero return code simply indicates a non-match.
What about:
- bcmp - Like memcmp but non-zero only means a non-match
Then in the description say that bcmp() callers must not expect anything more than zero and non-zero,
Yes, but let's completely avoid mentioning memcmp in the summary.
bcmp - return 0 if and only if the buffers have identical contents @a: pointer to first buffer @b: pointer to second buffer @len: size of buffers
The sign or magnitude of a non-zero return value has no particular meaning, and architectures may implement their own more efficient bcmp(). So while this particular implementation is a simple (tail) call to memcmp, do not rely on anything but whether the return value is zero or non-zero.
Rasmus
On Wed, 13 Mar 2019 20:34:11 +0100 Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
Yes, but let's completely avoid mentioning memcmp in the summary.
bcmp - return 0 if and only if the buffers have identical contents @a: pointer to first buffer @b: pointer to second buffer @len: size of buffers
The sign or magnitude of a non-zero return value has no particular meaning, and architectures may implement their own more efficient bcmp(). So while this particular implementation is a simple (tail) call to memcmp, do not rely on anything but whether the return value is zero or non-zero.
Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
Thanks!
-- Steve
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed: * A weak alias was discussed, but breaks for architectures that define their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly. * -ffreestanding also is used sporadically throughout the kernel. * -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org --- Changes V2 -> V3: * Adjust comment as per Steven to Rasmus' sugguestion. * Pick up Steven's Ack. Changes V1 -> V2: * Add declaration to include/linux/string.h. * Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..48a43a42d3f2 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,21 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/** + * bcmp - returns 0 if and only if the buffers have identical contents. + * @cs: One area of memory. + * @ct: Another area of memory. + * @count: The size of the areas. + */ +#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 March 13, 2019 4:37:37 PM EDT, Nick Desaulniers ndesaulniers@google.com wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
Changes V2 -> V3:
- Adjust comment as per Steven to Rasmus' sugguestion.
- Pick up Steven's Ack.
Changes V1 -> V2:
- Add declaration to include/linux/string.h.
- Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..48a43a42d3f2 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,21 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical
contents.
- @cs: One area of memory.
- @ct: Another area of memory.
- @count: The size of the areas.
Where's the rest of the comment that I gave my ack to?
-- Steve
- */
+#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.
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed: * A weak alias was discussed, but breaks for architectures that define their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly. * -ffreestanding also is used sporadically throughout the kernel. * -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org --- Changes V3 -> V4: * Include the entirety of Rasmus' sugguestion, as per Steven. * Change formal parameter identifiers to match the comment. Changes V2 -> V3: * Adjust comment as per Steven to Rasmus' sugguestion. * Pick up Steven's Ack. Changes V1 -> V2: * Add declaration to include/linux/string.h. * Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..3ab861c1a857 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/** + * bcmp - returns 0 if and only if the buffers have identical contents. + * @a: pointer to first buffer. + * @b: pointer to second buffer. + * @len: size of buffers. + * + * The sign or magnitude of a non-zero return value has no particular + * meaning, and architectures may implement their own more efficient bcmp(). So + * while this particular implementation is a simple (tail) call to memcmp, do + * not rely on anything but whether the return value is zero or non-zero. + */ +#undef bcmp +int bcmp(const void *a, const void *b, size_t len) +{ + return memcmp(a, b, len); +} +EXPORT_SYMBOL(bcmp); +#endif + #ifndef __HAVE_ARCH_MEMSCAN /** * memscan - Find a character in an area of memory.
On Wed, Mar 13, 2019 at 02:13:31PM -0700, Nick Desaulniers wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
Reviewed-by: Nathan Chancellor natechancellor@gmail.com Tested-by: Nathan Chancellor natechancellor@gmail.com
Thanks for fixing this better than I did (or tried to)!
Changes V3 -> V4:
- Include the entirety of Rasmus' sugguestion, as per Steven.
- Change formal parameter identifiers to match the comment.
Changes V2 -> V3:
- Adjust comment as per Steven to Rasmus' sugguestion.
- Pick up Steven's Ack.
Changes V1 -> V2:
- Add declaration to include/linux/string.h.
- Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..3ab861c1a857 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif +#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical contents.
- @a: pointer to first buffer.
- @b: pointer to second buffer.
- @len: size of buffers.
- The sign or magnitude of a non-zero return value has no particular
- meaning, and architectures may implement their own more efficient bcmp(). So
- while this particular implementation is a simple (tail) call to memcmp, do
- not rely on anything but whether the return value is zero or non-zero.
- */
+#undef bcmp +int bcmp(const void *a, const void *b, size_t len) +{
- return memcmp(a, b, len);
+} +EXPORT_SYMBOL(bcmp); +#endif
#ifndef __HAVE_ARCH_MEMSCAN /**
- memscan - Find a character in an area of memory.
-- 2.21.0.360.g471c308f928-goog
On Thu, Mar 14, 2019 at 6:13 AM Nick Desaulniers ndesaulniers@google.com wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
Reviewed-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes V3 -> V4:
- Include the entirety of Rasmus' sugguestion, as per Steven.
- Change formal parameter identifiers to match the comment.
Changes V2 -> V3:
- Adjust comment as per Steven to Rasmus' sugguestion.
- Pick up Steven's Ack.
Changes V1 -> V2:
- Add declaration to include/linux/string.h.
- Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..3ab861c1a857 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical contents.
- @a: pointer to first buffer.
- @b: pointer to second buffer.
- @len: size of buffers.
- The sign or magnitude of a non-zero return value has no particular
- meaning, and architectures may implement their own more efficient bcmp(). So
- while this particular implementation is a simple (tail) call to memcmp, do
- not rely on anything but whether the return value is zero or non-zero.
- */
+#undef bcmp +int bcmp(const void *a, const void *b, size_t len) +{
return memcmp(a, b, len);+} +EXPORT_SYMBOL(bcmp); +#endif
#ifndef __HAVE_ARCH_MEMSCAN /**
- memscan - Find a character in an area of memory.
-- 2.21.0.360.g471c308f928-goog
On Wed, Mar 13, 2019 at 02:13:31PM -0700, Nick Desaulniers wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
Thanks! Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Link: https://bugs.llvm.org/show_bug.cgi?id=41035 Link: https://code.woboq.org/userspace/glibc/string/memcmp.c.html#bcmp Link: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18... Link: https://github.com/ClangBuiltLinux/linux/issues/416 Cc: stable@vger.kernel.org Reported-by: Nathan Chancellor natechancellor@gmail.com Reported-by: Adhemerval Zanella adhemerval.zanella@linaro.org Suggested-by: Arnd Bergmann arnd@arndb.de Suggested-by: James Y Knight jyknight@google.com Suggested-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Nathan Chancellor natechancellor@gmail.com Suggested-by: Rasmus Villemoes linux@rasmusvillemoes.dk Signed-off-by: Nick Desaulniers ndesaulniers@google.com Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
Changes V3 -> V4:
- Include the entirety of Rasmus' sugguestion, as per Steven.
- Change formal parameter identifiers to match the comment.
Changes V2 -> V3:
- Adjust comment as per Steven to Rasmus' sugguestion.
- Pick up Steven's Ack.
Changes V1 -> V2:
- Add declaration to include/linux/string.h.
- Reword comment above bcmp.
include/linux/string.h | 3 +++ lib/string.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..3ab861c1a857 100644 --- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif +#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical contents.
- @a: pointer to first buffer.
- @b: pointer to second buffer.
- @len: size of buffers.
- The sign or magnitude of a non-zero return value has no particular
- meaning, and architectures may implement their own more efficient bcmp(). So
- while this particular implementation is a simple (tail) call to memcmp, do
- not rely on anything but whether the return value is zero or non-zero.
- */
+#undef bcmp +int bcmp(const void *a, const void *b, size_t len) +{
- return memcmp(a, b, len);
+} +EXPORT_SYMBOL(bcmp); +#endif
#ifndef __HAVE_ARCH_MEMSCAN /**
- memscan - Find a character in an area of memory.
-- 2.21.0.360.g471c308f928-goog
From: Nick Desaulniers
Sent: 13 March 2019 21:14
...
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif
Shouldn't that prototype always be present? Any architecture specific implementation better have the same definition. This is particularly true here since the compiler is going to assume the default calling convention.
The only time you wouldn't want it is when the architecture specific implementation is inline asm. If that were an option the default implementation would need to be excluded using a different #define.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 14/03/2019 10.57, David Laight wrote:
From: Nick Desaulniers
Sent: 13 March 2019 21:14
...
diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..6ab0a6fa512e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -150,6 +150,9 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif +#ifndef __HAVE_ARCH_BCMP +extern int bcmp(const void *,const void *,__kernel_size_t); +#endif
Shouldn't that prototype always be present?
Yes and no. The problem is that asm/string.h may decide to implement bcmp (or memcpy, memset, strcpy, ...) as a macro, which would break rather badly when used to declare the function. And we can't undef the macro temporarily. So I think the convention is that asm/string.h provides the prototype before it #defines the macro, see e.g. arch/x86/include/asm/string_32.h which has a #define of memcpy.
There is a fix for the "may be defined as a function-like macro", which is
extern int (bcmp)(const void *,const void *,__kernel_size_t);
I'd like to see that used rather than the ad hoc convention, but it is of course somewhat unconventional C.
Rasmus
On Wed, 13 Mar 2019 14:13:31 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
I guess we should backport this into -stable so that older kernels can be built with newer Clang.
...
--- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif +#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical contents.
- @a: pointer to first buffer.
- @b: pointer to second buffer.
- @len: size of buffers.
- The sign or magnitude of a non-zero return value has no particular
- meaning, and architectures may implement their own more efficient bcmp(). So
- while this particular implementation is a simple (tail) call to memcmp, do
- not rely on anything but whether the return value is zero or non-zero.
- */
+#undef bcmp
What is the undef for?
+int bcmp(const void *a, const void *b, size_t len) +{
- return memcmp(a, b, len);
+} +EXPORT_SYMBOL(bcmp); +#endif
#ifndef __HAVE_ARCH_MEMSCAN /**
- memscan - Find a character in an area of memory.
-- 2.21.0.360.g471c308f928-goog
On Wed, Mar 20, 2019 at 7:11 PM Andrew Morton akpm@linux-foundation.org wrote:
On Wed, 13 Mar 2019 14:13:31 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
A recent optimization in Clang (r355672) lowers comparisons of the return value of memcmp against zero to comparisons of the return value of bcmp against zero. This helps some platforms that implement bcmp more efficiently than memcmp. glibc simply aliases bcmp to memcmp, but an optimized implementation is in the works.
This results in linkage failures for all targets with Clang due to the undefined symbol. For now, just implement bcmp as a tailcail to memcmp to unbreak the build. This routine can be further optimized in the future.
Other ideas discussed:
- A weak alias was discussed, but breaks for architectures that define
their own implementations of memcmp since aliases to declarations are not permitted (only definitions). Arch-specific memcmp implementations typically declare memcmp in C headers, but implement them in assembly.
- -ffreestanding also is used sporadically throughout the kernel.
- -fno-builtin-bcmp doesn't work when doing LTO.
I guess we should backport this into -stable so that older kernels can be built with newer Clang.
Ah, you're right. I always forget. Is it too late to add
Cc: stable@vger.kernel.org
to the patch in your tree?
...
--- a/lib/string.c +++ b/lib/string.c @@ -866,6 +866,26 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif
+#ifndef __HAVE_ARCH_BCMP +/**
- bcmp - returns 0 if and only if the buffers have identical contents.
- @a: pointer to first buffer.
- @b: pointer to second buffer.
- @len: size of buffers.
- The sign or magnitude of a non-zero return value has no particular
- meaning, and architectures may implement their own more efficient bcmp(). So
- while this particular implementation is a simple (tail) call to memcmp, do
- not rely on anything but whether the return value is zero or non-zero.
- */
+#undef bcmp
What is the undef for?
Used the same convention as memcpy. Looking at the rest of the translation unit, I see that there's a mix of functions that do or do not undef their symbol. Rasmus pointed out that memcpy is implemented as a macro for x86 (arch/x86/include/asm/string_32.h). But looking closer now, I suspect it's not needed (anyone declaring memcmp as a macro should be setting __HAVE_ARCH_MEMCMP).
Shall I send you a cleanup removing the undefs for bcmp, memcmp, strcat, strcpy, and strcmp? Of those, I only see memcmp being `#defined` in arch/m68k/include/asm/string.h, arch/x86/boot/string.h, and arch/x86/include/asm/string_32.h.
Further, I can drop some of the __GNUC__ < 4 code in arch/x86/include/asm/string_32.h. (grepping for __GNUC__, looks like there's a fair amount of code that can be cleaned up). We should probably check it since Clang lies about being GCC 4.2 compatible, which will surely break in horrific ways at some point.
On Thu, Mar 21, 2019 at 10:02 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Wed, Mar 20, 2019 at 7:11 PM Andrew Morton akpm@linux-foundation.org wrote:
I guess we should backport this into -stable so that older kernels can be built with newer Clang.
Ah, you're right. I always forget. Is it too late to add
Cc: stable@vger.kernel.org
to the patch in your tree?
Just noticed now. Thank you for adding it!
On Thu, 21 Mar 2019 10:02:37 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
Shall I send you a cleanup removing the undefs for bcmp, memcmp, strcat, strcpy, and strcmp? Of those, I only see memcmp being `#defined` in arch/m68k/include/asm/string.h, arch/x86/boot/string.h, and arch/x86/include/asm/string_32.h.
Further, I can drop some of the __GNUC__ < 4 code in arch/x86/include/asm/string_32.h. (grepping for __GNUC__, looks like there's a fair amount of code that can be cleaned up). We should probably check it since Clang lies about being GCC 4.2 compatible, which will surely break in horrific ways at some point.\
All sounds good. Some time, when convenient, thanks.
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
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Andy Shevchenko -
Arnd Bergmann -
David Laight -
Masahiro Yamada -
Nathan Chancellor -
Nick Desaulniers -
Rasmus Villemoes -
Steven Rostedt