Linux: '-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://bugs.llvm.org/show_bug.cgi?id=41035#c13

[nathanchance]

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

[tpimh]

@jyknight just closed https://bugs.llvm.org/show_bug.cgi?id=41035 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://bugs.llvm.org/show_bug.cgi?id=41035 reopened by Szabolcs Nagy just now.

[nickdesaulniers]

LGTM. Please CC me, @arnd, and [email protected]. to @masahiroy.

On Tue, Mar 12, 2019, 12:01 PM Nathan Chancellor [e-mail]
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
https://github.com/ClangBuiltLinux/linux/issues/416#issuecomment-472138416,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvUX8Oedpu7eeW3nL9Ds4o8niXAbsAxks5vV_mMgaJpZM4bmb1R
.

[nathanchance]

Patch sent, thanks Nick: https://lore.kernel.org/lkml/[email protected]/

[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://lkml.org/lkml/2019/3/13/559
• https://lkml.org/lkml/2019/3/13/562 (simplest)

Then we can worry about providing optimized implementations.

[nickdesaulniers]

V2 submitted: https://lkml.org/lkml/2019/3/13/603

[nickdesaulniers]

picked up by akpm: https://www.spinics.net/lists/stable/msg291538.html

[dileks]

This patch is not available in any mm series.
Not sure what people mean with "soon available" (the patch is from last Wednesday).

Is there a patchwork URL to easy pick that patch?
Not sure if [4] is what I look for.

Update: [5] looks good to me.
Update-2: On update a way to easy get it is from [6].

[1] https://www.spinics.net/lists/stable/msg291538.html
[1] https://lore.kernel.org/stable/20190321021358.8P7AM547b%[email protected]/
[2] https://ozlabs.org/~akpm/mmotm/series
[3] https://ozlabs.org/~akpm/mmots/series
[4] https://patches.linaro.org/project/linux-stable/list/
[5] https://lore.kernel.org/stable/20190321021358.8P7AM547b%[email protected]/raw
[6] http://git.cmpxchg.org/cgit.cgi/linux-mmotm.git/

[nickdesaulniers]

looks like there hasn't been a push to a public branch since March 8.

[dileks]

The patch lib/string.c: implement a basic bcmp is now in linux-mmotm.git#v5.1-rc2-mmotm-2019-03-28-15-50.

[1] http://git.cmpxchg.org/cgit.cgi/linux-mmotm.git/commit/?h=v5.1-rc2-mmotm-2019-03-28-15-50&id=e67937f39b039752811201287fba08611ca8687c

[dileks]

Cool, now in Linus tree :-).

[nathanchance]

Merged into mainline: https://git.kernel.org/linus/5f074f3e192f10c9fade898b9b3b8812e3d83342

It is cc'd to stable, Greg will probably pick it up within the next week.

[nickdesaulniers]

I suspect the LTO aspect of this may have been fixed by: https://reviews.llvm.org/rG0508c994f0b14144041f2cfd3ba9f9a80f03de08
@samitolvanen reported that -fno-builtin-bcmp now works with LTO and the definition removed.