'-fno-builtin-bcmp' is needed after r355672

[nathanchance]

r355672 causes the following issue with basically all configs:

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'
ld: arch/x86/kernel/setup.o:setup.c:(.init.text+0x244): more undefined references to `bcmp' follow

According to the commit message, this is the diff we need to fix it (I don't think cc-option is necessary?):

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

[nickdesaulniers]

Probably should be behind a cc-option macro, though -Qunused-arguments will hide that.

  // memcmp(x, y, Len) == 0 -> bcmp(x, y, Len) == 0
  // `bcmp` can be more efficient than memcmp because it only has to know that
  // there is a difference, not where is is.

That's a nice little optimization...

  // Posix removed support from bcmp() in 2001, but the glibc and several
  // implementations of the libc still have it.

whoops!

Probably could provide glibc's implementation to the kernel. We had an internal report this week from CrOS about __lshrdi3 that I need to dig into more.

[nathanchance]

Probably should be behind a cc-option macro...

Yeah I'd be fine with that.

Probably could provide glibc's implementation to the kernel

That'd be nice if it is a good optimization. I'm just more worried about how long that would take to make it through the pipeline since it will need to be supplied for all architectures we are using. As it stands, CI is going to go completely red when we get an update (we can work around it in driver.sh with the KCFLAGS variable but still, doesn't help everyone).

[nathanchance]

Hmmm looking into it more, glibc just aliases bcmp to memcmp nowadays... (and has since 1995?): https://www.gnu.org/software/libc/manual/html_mono/libc.html#String_002fArray-Comparison

I personally think this should be done (plus adjusting the comment and tests I guess), this would save us a lot of headaches:

diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index 50a05a897e9..3312093672a 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -53,7 +53,7 @@ static bool hasBcmp(const Triple &TT) {
   // Posix removed support from bcmp() in 2001, but the glibc and several
   // implementations of the libc still have it.
   if (TT.isOSLinux())
-    return TT.isGNUEnvironment() || TT.isMusl();
+    return TT.isMusl();
   // Both NetBSD and OpenBSD are planning to remove the function. Windows does
   // not have it.
   return TT.isOSFreeBSD() || TT.isOSSolaris() || TT.isOSDarwin();

[nathanchance]

This hit CI and well... 💀

https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/104027249

[nickdesaulniers]

I agree with James' assessment: https://llvm.org/pr41035

[nathanchance]

Fair enough, should I send the -fno-builtin-bcmp patch then?

[tpimh]

@jyknight just closed https://llvm.org/pr41035 as WONTFIX, so let's send -fno-builtin-bcmp patch.

[nathanchance]

Until it gets fixed, we should be able to work around it in CI with this diff:

diff --git a/driver.sh b/driver.sh
index 1f0e196..7cd16fc 100755
--- a/driver.sh
+++ b/driver.sh
@@ -213,7 +213,7 @@ mako_reactor() {
   KBUILD_BUILD_TIMESTAMP="Thu Jan  1 00:00:00 UTC 1970" \
   KBUILD_BUILD_USER=driver \
   KBUILD_BUILD_HOST=clangbuiltlinux \
-  make -j"${jobs:-$(nproc)}" CC="${CC}" HOSTCC="${CC}" LD="${LD}" HOSTLD="${HOSTLD:-ld}" AR="${AR}" "${@}"
+  make -j"${jobs:-$(nproc)}" CC="${CC}" HOSTCC="${CC}" LD="${LD}" HOSTLD="${HOSTLD:-ld}" AR="${AR}" KCFLAGS="-fno-builtin-bcmp" "${@}"
 }
 
 build_linux() {

[nathanchance]

Tentative patch but I won't send it until we come to a consensus of what to do: https://gist.github.com/e68ae38e40fff93649b1f8c3def07a19

[tpimh]

Btw, https://llvm.org/pr41035 reopened by Szabolcs Nagy just now.

[nickdesaulniers]

LGTM. Please CC me, @arnd, and jyknight@google.com. to @masahiroy.

…

On Tue, Mar 12, 2019, 12:01 PM Nathan Chancellor ***@***.***> wrote: Tentative patch but I won't send it until we come to a consensus of what to do: https://gist.github.com/e68ae38e40fff93649b1f8c3def07a19 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvUX8Oedpu7eeW3nL9Ds4o8niXAbsAxks5vV_mMgaJpZM4bmb1R> .

[nathanchance]

Patch sent, thanks Nick: https://lore.kernel.org/lkml/20190312215203.27643-1-natechancellor@gmail.com/

[nathanchance]

Hmm, it seems like -fno-builtin-bcmp isn't good enough for LTO and CFI in kernel/common though.

With them enabled: https://gist.github.com/2970612cc207f00f1864fc2490fdfe21

With them disabled: https://gist.github.com/4dd746c92283bea124db81f068fee9a3

I can very clearly see that that flag was passed to those translation units (.printk.o.cmd and .core.o.cmd). I do not see this issue when using my LLVM patch above, suggesting that -fno-builtin-bcmp isn't getting respected somewhere in the backend.

I can open another issue here or on LLVM's bug tracker if that's preferred.

[nickdesaulniers]

-fno-builtin-bcmp isn't good enough for LTO and CFI in kernel/common though.

argh (cc @samitolvanen @jyknight ). I think then we'll have to provide implementations of bcmp (or at least alias it to memcmp) in the kernel then. I'm testing out such a patch now.

EDIT: just saw now that @nathanchance came up with a similar patch, and hit similar issues.

[nathanchance]

I messed around with it this morning before work. It's easy for the generic memcmp implementation (in lib/string.c, see my reply to Arnd) but I had no idea how to deal with the assembly implementations like arm64 has.

[samitolvanen]

Sounds like -fno-builtin-bcmp is not stored in the IR then. Have you checked if it's possible to pass it to the linker as -plugin-opt like we're doing with code model?

[nickdesaulniers]

Sounds like -fno-builtin-bcmp is not stored in the IR then.

That's what I was thinking.

Have you checked if it's possible to pass it to the linker as -plugin-opt like we're doing with code model?

Not yet. I posted 2 simple patches that just define bcmp, which I think is the simplest solution:

• https://lore.kernel.org/r/CAKwvOd=UYEtQC9T2K=O0rxrDKug4XzsA6dHjSKmogFa7whE7Zw@mail.gmail.com/
• https://lore.kernel.org/r/CAKwvOd=8rAZi3ZT8qEupmS6xY2U10cKT7kPtB=gYfWsJEJ0GSQ@mail.gmail.com/ (simplest)

Then we can worry about providing optimized implementations.

[nickdesaulniers]

V2 submitted: https://lore.kernel.org/r/20190313181719.87859-1-ndesaulniers@google.com/