Debian LLVM patches causing failures for LLVM_IAS=1 in android tree builds

[nickdesaulniers]

cc @terceiro @ivoire

Since upgrading our builds of android kernels to use clang's integrated assembler for 32b ARM targets, our CI has been red with errors I cannot reproduce locally. example

All of the Android kernel tiles on https://clangbuiltlinux.github.io/ are demonstrating this issue.

It looks like from the log that the invocation of the build was:

$ make -j72 LLVM=1 LLVM_IAS=1 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- HOSTCC=clang CC=clang allmodconfig
$ make -j72 LLVM=1 LLVM_IAS=1 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- HOSTCC=clang CC=clang

Even if I checkout clang-12 (release/12.x branch of llvm-project) I cannot reproduce. Same for clang-11.

We do have a one off comment from @arndb about the error, but I think it might have been that CONFIG_THUMB2_KERNEL=y was enabled, which was not in our invocation of tuxmake.

Without being able to reproduce locally, I don't know whether there's an issue with ccache perhaps, or tuxmake not using the configs requested, or the wrong version of clang, or what.

[nickdesaulniers]

@arndb 's comment even mentions

Attaching a log from an allmodconfig build with CONFIG_ARCH_MULTI_V6=n and CONFIG_THUMB2_KERNEL=y

Neither of those were set that way in the reported build config.

[terceiro]

@nickdesaulniers can you share the tuxbuild link instead (i.e. the folder with all artifacts)? the links you posted don't work for me

[vishalbhoj]

@terceiro https://builds.tuxbuild.com/1qjDdiIxOARpNm2jCxOg363Rd84/

[nathanchance]

This is reproducible with tuxmake -a arm -k allmodconfig -r podman -t clang-nightly -w ccache LLVM=1 LLVM_IAS=1 on the latest android-mainline branch at https://android.googlesource.com/kernel/common.

However, if I build upstream LLVM at the hash that the current version of clang-nightly is built from (5f57793c4fe4), I cannot reproduce it:

$ git -C "${CBL}"/github/tc-build/llvm-project reset --hard 5f57793c4fe4
HEAD is now at 5f57793c4fe4 * NFC. Refactored DIPrinter for better support of new print styles.

$ "${CBL}"/github/tc-build/build-llvm.py \
--assertions \
--build-stage1-only \
--no-update \
--projects "clang;compiler-rt;lld" \
--targets "ARM;X86"
...

$ PATH=${CBL}/github/tc-build/build/llvm/stage1/bin:${PATH} tuxmake -a arm -k allmodconfig -t clang -w ccache LLVM=1 LLVM_IAS=1
...

$ echo $?
0

[nathanchance]

One of these patches might be relevant?

https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/clang-arm-default-vfp3-on-armv7a.patch

https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/26-set-correct-float-abi.diff

https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/930008-arm.diff

[nathanchance]

Yup, it's the first one: https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/clang-arm-default-vfp3-on-armv7a.patch

which was added because of these two bugs:

https://bugs.debian.org/841474

https://bugs.debian.org/842142

In response to this patch in LLVM, which defaulted armv7-a to NEON.

llvm/llvm-project@4adcb73

I do not really understand though, allmodconfig adds -march=armv6k, instead of -march=armv7-a so I fail to see why that patch would change anything in this case but it does.

Assuming Debian does not want to remove this patch, we will probably need to work around this in the kernel.

[nickdesaulniers]

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=841474#22 says:

LLVM 3.8/3.9 default to emitting NEON instructions on
armhf but it is optional in ARMv7 so LLVM is producing binaries that do
not work on ARMv7 CPUs that lack NEON.

llvm/llvm-project@4adcb73#diff-239adc65a4d98655f5b95d1f6f72605518c540115d81e2198d7a360714ca5439R79-R86 shows that NEON is indeed not required for ARMv7, but can be enabled if -march=armv7-a (A class, but not R or M class).

This is causing build failures
in packages that use LLVM on Debian buildds that lack NEON.

So it sounds to me like the fix would be to ensure that builds of those projects set -march=armv7-m then and not -march=armv7-a? I'm pretty sure the default ARM revision for --target=arm-linux-gnueabihf is not even ARMv7, it's ARMv6. See #define __ARM_ARCH 6 via: https://godbolt.org/z/rc7n1bKhf

@sylvestre can AFL now be built with that patch dropped? I assume either:

• LLVM was defaulting to ARMv7 for --target=arm-linux-gnueabihf and has since changed to ARMv6 to match GCC, or
• AFL was setting -march=armv7-a when they should be setting -march=armv7-m (or -march=armv6 which is implicit, so not specifying -march= at all should be equivalent).
• AFL was setting -Wa,-march=armv7-m or -Wa,-march=armv6 or older, which LLVM wasn't respecting until relatively recently by @DavidSpickett 's https://reviews.llvm.org/rG1d51c699b9e2ebc5bcfdbe85c74cc871426333d4).

https://reviews.llvm.org/rGe17c58003498b06572cf0e56cb6eac65fe7e1014 looks interesting but landed before Debian's issue was filed.

cc @smithp35 @rengolin

[jrtc27]

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=841474#22 says:

LLVM 3.8/3.9 default to emitting NEON instructions on
armhf but it is optional in ARMv7 so LLVM is producing binaries that do
not work on ARMv7 CPUs that lack NEON.

llvm/llvm-project@4adcb73#diff-239adc65a4d98655f5b95d1f6f72605518c540115d81e2198d7a360714ca5439R79-R86 shows that NEON is indeed not required for ARMv7, but can be enabled if -march=armv7-a (A class, but not R or M class).

This is causing build failures
in packages that use LLVM on Debian buildds that lack NEON.

So it sounds to me like the fix would be to ensure that builds of those projects set -march=armv7-m then and not -march=armv7-a?

A-class and M-class are entirely different. Debian's armel is armv5te and armhf is armv7 with VFPv3-D16 but optional NEON (https://wiki.debian.org/ArchitectureSpecificsMemo#armel).

I'm pretty sure the default ARM revision for --target=arm-linux-gnueabihf is not even ARMv7, it's ARMv6. See #define __ARM_ARCH 6 via: https://godbolt.org/z/rc7n1bKhf

@sylvestre can AFL now be built with that patch dropped? I assume either:

• LLVM was defaulting to ARMv7 for --target=arm-linux-gnueabihf and has since changed to ARMv6 to match GCC, or
• AFL was setting -march=armv7-a when they should be setting -march=armv7-m (or -march=armv6 which is implicit, so not specifying -march= at all should be equivalent).
• AFL was setting -Wa,-march=<foo>, which LLVM wasn't respecting until relatively recently by @DavidSpickett 's https://reviews.llvm.org/rG1d51c699b9e2ebc5bcfdbe85c74cc871426333d4).

https://reviews.llvm.org/rGe17c58003498b06572cf0e56cb6eac65fe7e1014 looks interesting but landed before Debian's issue was filed.

cc @smithp35 @rengolin

[rengolin]

@jrtc27 is correct, do not pick armv7-m to mean either armv7-a or armv6. These are completely different things.

I believe this is a buildbot that still uses the NVidia boards that don't have NEON, and the option is -mfpu=vfpv3. This will disable NEON on the build, but you have to keep the arch as armv7-a or the instructions, optimisations and extensions used will be different and very inefficient.

The default being NEON (unlike GCC) is an old discussion that I lost a long time ago. My rationale was to avoid precisely this problem, but the counter argument was that most people don't really care and the number of ARMv7A devices without NEON are very very limited.

[nickdesaulniers]

Test case:

# neon.s
.fpu neon
.arch armv7-a
vmov.i32  q0, ClangBuiltLinux/continuous-integration2#1

builds just fine with

$ clang --target=arm-linux-gnueabi neon.s -c

but with Debian's diff

diff --git a/llvm/include/llvm/Support/ARMTargetParser.def b/llvm/include/llvm/Support/ARMTargetParser.def
index 37cf0a93bb04..b3e4e2351273 100644
--- a/llvm/include/llvm/Support/ARMTargetParser.def
+++ b/llvm/include/llvm/Support/ARMTargetParser.def
@@ -76,7 +76,7 @@ ARM_ARCH("armv6kz", ARMV6KZ, "6KZ", "v6kz", ARMBuildAttrs::CPUArch::v6KZ,
 ARM_ARCH("armv6-m", ARMV6M, "6-M", "v6m", ARMBuildAttrs::CPUArch::v6_M,
           FK_NONE, ARM::AEK_NONE)
 ARM_ARCH("armv7-a", ARMV7A, "7-A", "v7", ARMBuildAttrs::CPUArch::v7,
-          FK_NEON, ARM::AEK_DSP)
+          FK_VFPV3_D16, ARM::AEK_DSP)
 ARM_ARCH("armv7ve", ARMV7VE, "7VE", "v7ve", ARMBuildAttrs::CPUArch::v7,
           FK_NEON, (ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT |
           ARM::AEK_HWDIVARM | ARM::AEK_HWDIVTHUMB | ARM::AEK_DSP))
diff --git a/llvm/lib/Target/ARM/ARM.td b/llvm/lib/Target/ARM/ARM.td
index 9da2bdbbb103..8ce70cf5d8c9 100644
--- a/llvm/lib/Target/ARM/ARM.td
+++ b/llvm/lib/Target/ARM/ARM.td
@@ -725,7 +725,8 @@ def ARMv6sm   : Architecture<"armv6s-m",  "ARMv6sm",  [HasV6MOps,
                                                        FeatureStrictAlign]>;
 
 def ARMv7a    : Architecture<"armv7-a",   "ARMv7a",   [HasV7Ops,
-                                                       FeatureNEON,
+                                                       FeatureVFP3,
+                                                       FeatureVFP3_D16,
                                                        FeatureDB,
                                                        FeatureDSP,
                                                        FeatureAClass]>;

produces the error observed:

neon.s:3:1: error: instruction requires: NEON
vmov.i32  q0, ClangBuiltLinux/continuous-integration2#1
^

I'm not sure how we could build that (with additional compiler flags, or assembler source changes).

[jrtc27]

Test case:

# neon.s
.fpu neon
.arch armv7-a
vmov.i32  q0, ClangBuiltLinux/continuous-integration2#1

Swap the order of the lines; .arch overrides whatever you just set with .fpu.

[nathanchance]

diff --git a/arch/arm/crypto/curve25519-core.S b/arch/arm/crypto/curve25519-core.S
index be18af52e7dc..b697fa5d059a 100644
--- a/arch/arm/crypto/curve25519-core.S
+++ b/arch/arm/crypto/curve25519-core.S
@@ -10,8 +10,8 @@
 #include <linux/linkage.h>

 .text
-.fpu neon
 .arch armv7-a
+.fpu neon
 .align 4

 ENTRY(curve25519_neon)

fixes the issue for me.

[nickdesaulniers]

Cool. @nathanchance you have the diff handy and did the debugging. Do you mind sending that as a kernel patch with @arndb 's reported by and @jrtc27 's suggested by?

[nathanchance]

Sounds good, I will write up the patch now.

@jrtc27 is it okay that I include the Suggested-by: tag? If so, do you have a particular email you would like me to use?

[jrtc27]

Sounds good, I will write up the patch now.

@jrtc27 is it okay that I include the Suggested-by: tag? If so, do you have a particular email you would like me to use?

Sure; Jessica Clarke <jrtc27@jrtc27.com> is me.

[nathanchance]

Patch submitted: https://lore.kernel.org/r/20210409221155.1113205-1-nathan@kernel.org/

[nickdesaulniers]

accepted: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=44200f2d9b8b52389c70e6c7bbe51e0dc6eaf938

[nathanchance]

Merged into mainline: https://git.kernel.org/torvalds/c/44200f2d9b8b

This will find its way to the stable trees automatically.