HomePhabricator

[compiler-rt] Normalize i?86 to i386 and armv* to arm for…

Authored by mstorsjo on Mar 8 2021, 3:38 AM.

Description

[compiler-rt] Normalize i?86 to i386 and armv* to arm for COMPILER_RT_DEFAULT_TARGET_ARCH

This corresponds to getArchNameForCompilerRTLib in clang; any
32 bit x86 architecture triple (except on android, but those
exceptions are already handled in compiler-rt on a different level)
get the compiler rt library names with i386; arm targets get either
"arm" or "armhf". (Mapping to "armhf" is handled in the toplevel
CMakeLists.txt.)

Differential Revision: https://reviews.llvm.org/D98173

Event Timeline

This commit has broken our downstream toolchain builds which utilizes the llvm runtimes system to compile multiple sets of libraries with a just-built cross compiler.

Prior to the commit, our runtimes builds looked like this:

...
[54/270] Performing build step for 'builtins-armv7em-ti-none-eabihf'
[1/183] Building ASM object CMakeFiles/clang_rt.builtins-armv7em.dir/arm/sync_fetch_and_nand_4.S.obj
[2/183] Building ASM object CMakeFiles/clang_rt.builtins-armv7em.dir/arm/clzdi2.S.obj
[3/183] Building ASM object CMakeFiles/clang_rt.builtins-armv7em.dir/arm/modsi3.S.obj
[4/183] Building ASM object CMakeFiles/clang_rt.builtins-armv7em.dir/arm/sync_fetch_and_and_4.S.obj
[5/183] Building ASM object CMakeFiles/clang_rt.builtins-armv7em.dir/arm/udivmodsi4.S.obj
...

Note that the directory being populated is named with the tag 'armv7em'

After this commit, we see:

...
[57/270] Performing build step for 'builtins-armv6m-ti-none-eabi'
[1/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/comparesf2.S.obj
[2/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/divsi3.S.obj
[3/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/sync_fetch_and_and_8.S.obj
[4/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/sync_fetch_and_max_8.S.obj
[5/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/sync_fetch_and_min_8.S.obj
[6/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/sync_fetch_and_sub_8.S.obj
[7/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/bswapdi2.S.obj
[8/143] Building ASM object CMakeFiles/clang_rt.builtins-arm.dir/arm/clzdi2.S.obj
FAILED: CMakeFiles/clang_rt.builtins-arm.dir/arm/clzdi2.S.obj
<Workspace>/path/to/build_dir/llvm/./bin/clang -DVISIBILITY_HIDDEN -B<Workspace>/path/to/build_dir/ti/bin -isystem<Workspace>/path/to/build_dir/libc/lib/generic/include/c -Oz -ffunction-sections -fdata-sections -fomit-frame-pointer -fvisibility=hidden -Oz -D_LIBUNWIND_IS_BAREMETAL -march=armv6m -mlittle-endian -mfpu=none -mfloat-abi=soft -DCOMPILER_RT_HAS_FLOAT16 -std=c11 -fno-builtin -fvisibility=hidden -fomit-frame-pointer -o CMakeFiles/clang_rt.builtins-arm.dir/arm/clzdi2.S.obj -c <Workspace>/llvm-project/compiler-rt/lib/builtins/arm/clzdi2.S
<Workspace>/llvm-project/compiler-rt/lib/builtins/arm/clzdi2.S:48:2: error: conditional execution not supported in Thumb1
...

Note that now, all of our separate target runtime builds are attempting to populate the same directory, tagged with 'arm'. Furthermore, it looks like this change also affects how the compiler options are selected, since we're now seeing compilation errors.
Is our project doing something unexpected? I'll tag @phosek who probably knows more about the runtimes build system than I do to see if he can weigh in.

Edit: Have noted that there's still an -march, but this change still causes unexpected compilation failures perhaps due to the name being changed to a generic 'arm'. My next steps are to inspect how we populate the runtimes cmake variables to see if that could be the cause on our end.

This commit has broken our downstream toolchain builds which utilizes the llvm runtimes system to compile multiple sets of libraries with a just-built cross compiler.

I'm sorry about that. Do I guess correctly that you're building with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR enabled, as otherwise, clang would just look for all files in the same directory, and expect all of them to be named libclang_rt.builtins-arm.a or something like that? In such a setup, I guess that one doesn't want the normalization - but I hadn't really expect it to break anything either.

Note that now, all of our separate target runtime builds are attempting to populate the same directory, tagged with 'arm'.

I'm not very familiar with the runtimes build myself (I build separately by invoking cmake on the compiler-rt/lib/builtins directory), but do they end up running in the exact same directory, where they might clobber each others files?

Furthermore, it looks like this change also affects how the compiler options are selected, since we're now seeing compilation errors.
Is our project doing something unexpected? I'll tag @phosek who probably knows more about the runtimes build system than I do to see if he can weigh in.

Edit: Have noted that there's still an -march, but this change still causes unexpected compilation failures perhaps due to the name being changed to a generic 'arm'. My next steps are to inspect how we populate the runtimes cmake variables to see if that could be the cause on our end.

Can you pinpoint an example of the actual compile command for one individual file, before and after? I was afraid that this could do something like running with -march=arm, but that doesn't seem to be happening. What is happening though?

I can revert this if you prefer, while figuring out what's going on?

I'm sorry about that. Do I guess correctly that you're building with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR enabled, as otherwise, clang would just look for all files in the same directory, and expect all of them to be named libclang_rt.builtins-arm.a or something like that? In such a setup, I guess that one doesn't want the normalization - but I hadn't really expect it to break anything either.

LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is forced to ON for seemingly everyone except for 'aix'. See https://github.com/llvm/llvm-project/blob/main/llvm/runtimes/CMakeLists.txt#L75

I'm not very familiar with the runtimes build myself (I build separately by invoking cmake on the compiler-rt/lib/builtins directory), but do they end up running in the exact same directory, where they might clobber each others files?

This is correct. Because all compiler-rt builds target the same directory, these object files will all clobber one-another. However, it's my understanding that the runtimes sub-projects are each cmake ExternalProjects of the LLVM cmake system, meaning that each build step happens in-order. If the libraries are actually created into their own separate directories, it may still run successfully. However, this is still very dangerous and not robust.

Can you pinpoint an example of the actual compile command for one individual file, before and after? I was afraid that this could do something like running with -march=arm, but that doesn't seem to be happening. What is happening though?

I can revert this if you prefer, while figuring out what's going on?

I've confirmed that the CMakeCache is identical between the failing and passing versions.
The problem is apparently not related to options, but rather the file being compiled. This does skirt around some downstream changes we've made, but I'll attempt to ignore them. Note that this actually shouldn't have anything to do with the 'runtimes' build, and is instead just a general failure that should occur if anyone should attempt to use an armv6m compiler to build compiler-rt builtins.

The key variable here seems to be BUILTIN_SUPPORTED_ARCH, which is thankfully printed for us during builds.

Before: -- Builtin supported architectures: armv6m
After: -- Builtin supported architectures: arm

At https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/CMakeLists.txt#L552, the cmake system sets up a number of "${arch}_SOURCES" variables which contain the source files to build as part of builtins. Note that there are 'arm_SOURCES', 'armhf_SOURCES', and then, important to this example, 'armv6m_SOURCES'.

These variables are accessed as part of the loop in https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/CMakeLists.txt#L695 where, among other things, filter_builtin_sources is called on ${arch}_SOURCES. This call is intended to take default natural C builtin implementations (C) and replace them with optimized versions for a particular architecture (usually asm). Before the change, our builds printed the below, but do not afterwards:

For armv6m builtins preferring arm/divsi3.S to divsi3.c
For armv6m builtins preferring arm/udivsi3.S to udivsi3.c
For armv6m builtins preferring arm/comparesf2.S to comparesf2.c
For armv6m builtins preferring arm/addsf3.S to addsf3.c

You may also note that above the call to filter_builtin_sources, there are checks for ${arch} being various flavors of ARM, which will never trigger after your changes and could cause other errors.

Looking at the definitions, ${armv6m_SOURCES} is equal to ${thumb1_SOURCES}, which is a few arm/*.S files plus ${GENERIC_SOURCES}, which is a list of all default C implementations.
${arm_SOURCES}, however, contains a much larger number of assembly files, one of which is arm/divmodsi4.S. However, armv6m is a thumb-only architecture (link), and can't compile these assembly files for various reasons related to not being a full ARM instruction set.

path/to/build/dir/bin/clang ... -c path/to/llvm/compiler-rt/lib/builtins/arm/divmodsi4.S

path/to/llvm/compiler-rt/lib/builtins/arm/divmodsi4.S:48:5: error: invalid instruction, any one of the following would fix this:
    eor r4, r0, r1
    ^
path/to/llvm/compiler-rt/lib/builtins/arm/divmodsi4.S:48:5: note: instruction requires: thumb2
    eor r4, r0, r1
    ^
path/to/llvm/compiler-rt/lib/builtins/arm/divmodsi4.S:48:17: note: too many operands for instruction
    eor r4, r0, r1
                ^
path/to/llvm/compiler-rt/lib/builtins/arm/divmodsi4.S:52:5: error: instruction requires: thumb2
    eor ip, r0, r0, asr #31

This is unfortunately where any semblance of 'expertise' in the CMake system ends. I'd think the easiest solution would be to have BUILTIN_SUPPORTED_ARCH not be based off of this COMPILER_RT_DEFAULT_TARGET_ARCH. The latter is set in compiler-rt's CMakeLists as well as in the builtins CMakeLists when building standalone, while BUILTIN_SUPPORTED_ARCH is set by including builtin-config-ix in the builtins CMakeLists. That could be harder said than done, or the wrong way to go about it. I don't need the change reverted, but if no one is depending on it, we'd appreciate being able to continue our downstream automation :)

I'm sorry about that. Do I guess correctly that you're building with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR enabled, as otherwise, clang would just look for all files in the same directory, and expect all of them to be named libclang_rt.builtins-arm.a or something like that? In such a setup, I guess that one doesn't want the normalization - but I hadn't really expect it to break anything either.

LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is forced to ON for seemingly everyone except for 'aix'. See https://github.com/llvm/llvm-project/blob/main/llvm/runtimes/CMakeLists.txt#L75

Ah, thanks for pointing that out!

I'm not very familiar with the runtimes build myself (I build separately by invoking cmake on the compiler-rt/lib/builtins directory), but do they end up running in the exact same directory, where they might clobber each others files?

This is correct. Because all compiler-rt builds target the same directory, these object files will all clobber one-another. However, it's my understanding that the runtimes sub-projects are each cmake ExternalProjects of the LLVM cmake system, meaning that each build step happens in-order. If the libraries are actually created into their own separate directories, it may still run successfully. However, this is still very dangerous and not robust.

Ok, that's clearly not good.

You may also note that above the call to filter_builtin_sources, there are checks for ${arch} being various flavors of ARM, which will never trigger after your changes and could cause other errors.

Looking at the definitions, ${armv6m_SOURCES} is equal to ${thumb1_SOURCES}, which is a few arm/*.S files plus ${GENERIC_SOURCES}, which is a list of all default C implementations.
${arm_SOURCES}, however, contains a much larger number of assembly files, one of which is arm/divmodsi4.S. However, armv6m is a thumb-only architecture (link), and can't compile these assembly files for various reasons related to not being a full ARM instruction set.

Oh crap, yes. I'll go ahead and revert the arm part of this commit. If the change I tried to do is to be done, it has to be done at the final stages (maybe something like this, https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L126).

So clearly, normalizing arm* to arm (or armhf) isn't right in general. The toplevel CMakeLists also does happen to have such a case though, see https://github.com/llvm/llvm-project/blob/main/compiler-rt/CMakeLists.txt#L111-L114. IMO that also would lose all the details between various arm architecture variants. But maybe that just doesn't trigger for ones that are sensitive enough for the distinction to matter?

The toplevel CMakeLists also does happen to have such a case though, see https://github.com/llvm/llvm-project/blob/main/compiler-rt/CMakeLists.txt#L111-L114. IMO that also would lose all the details between various arm architecture variants. But maybe that just doesn't trigger for ones that are sensitive enough for the distinction to matter?

As far as I know, that check is very laser-focused. The only use of COMPILER_RT_ARM_THUMB is in the address sanitizer's lit.site.cfg, so it's not surprising that it wouldn't cause immediate issues.

The toplevel CMakeLists also does happen to have such a case though, see https://github.com/llvm/llvm-project/blob/main/compiler-rt/CMakeLists.txt#L111-L114. IMO that also would lose all the details between various arm architecture variants. But maybe that just doesn't trigger for ones that are sensitive enough for the distinction to matter?

As far as I know, that check is very laser-focused. The only use of COMPILER_RT_ARM_THUMB is in the address sanitizer's lit.site.cfg, so it's not surprising that it wouldn't cause immediate issues.

It's not the COMPILER_RT_ARM_THUMB case I'm worried about, it's the fact that it does set(COMPILER_RT_DEFAULT_TARGET_ARCH "armhf"), which essentially is the same as I did; if you'd have a fictional armv6m-linux-gnueabihf triple, it would overwrite the architecture to plain armhf instead of the proper armv6m and you'd have the same issue.

Oh! Sorry, I glazed over that block and missed the set. I suspect that an architecture called '^arm.*hf$' doesn't cause issues in builtins specifically because it seems there's no special casing for anything besides 'armhf'

https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/builtin-config-ix.cmake#L40

These target lists are defined to eventually populate ALL_BUILTIN_SUPPORTED_ARCH, which is used as one of two sets which are intersected to get the full list of architectures being built in that particular project. Because there's no 'arm.*hf' in that list besides armhf, then the only architecture supported will be 'armhf'.

Oh! Sorry, I glazed over that block and missed the set. I suspect that an architecture called '^arm.*hf$' doesn't cause issues in builtins specifically because it seems there's no special casing for anything besides 'armhf'

https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/builtin-config-ix.cmake#L40

These target lists are defined to eventually populate ALL_BUILTIN_SUPPORTED_ARCH, which is used as one of two sets which are intersected to get the full list of architectures being built in that particular project. Because there's no 'arm.*hf' in that list besides armhf, then the only architecture supported will be 'armhf'.

You wouldn't need to have an architecture name arm.*hf - the first check (that checks for the traiilng hf) checks the triple, not the architecture, so it'd hit for e.g. armv7-linux-gnueabihf.

JamesNagurne added a comment.EditedMar 11 2021, 1:36 PM

I want to apologize for missing such obvious things. Perhaps I used all of my observational power on my initial investigation. Of course it checks the triple for *hf and then the leading arch for arm, since that's how the property is added to the triple (*-eabihf)

In our last successful build, armhf is nowhere to be seen, so I'm not sure precisely how that check-and-set of armhf affects the builtins project. Stepping through the code visually would seem to suggest that the condition is true, since the triple (armv7em-ti-none-eabihf) ends with 'hf' and the arch (armv7em) starts with 'arm'. Indeed it does trigger, but only in the compiler-rt build.

[52/161] Performing configure step for 'runtimes-armv7reb-ti-none-eabihf'
...
-- Compiler-RT supported architectures: armhf
...
[215/270] Performing build step for 'runtimes-armv7em-ti-none-eabihf'
...
[11/256] Linking C static library path/to/build/dir/lib/clang/13.0.0/lib/armv7em-ti-none-eabihf/libclang_rt.profile.a

The builtins library is built separate from compiler-rt when done as part of a runtimes build. Thus, the code in compiler-rt's CMakeLists is irrelevant for my particular case (and any case using LLVM_ENABLE_RUNTIMES). It does, however, expose a dangerous interaction where the target can change between a standalone and included build.

[52/270] Performing configure step for 'builtins-armv7em-ti-none-eabihf'
...
-- Builtin supported architectures: armv7em
...
-- Looking for __ARM_FP
-- Looking for __ARM_FP - found
-- Performing Test COMPILER_RT_HAS_armv7em_VFP_DP
-- Performing Test COMPILER_RT_HAS_armv7em_VFP_DP - Failed
-- For armv7em builtins preferring arm/fp_mode.c to fp_mode.c
-- For armv7em builtins preferring arm/bswapdi2.S to bswapdi2.c
-- For armv7em builtins preferring arm/bswapsi2.S to bswapsi2.c
...

I want to apologize for missing such obvious things. Perhaps I used all of my observational power on my initial investigation. Of course it checks the triple for *hf and then the leading arch for arm, since that's how the property is added to the triple (*-eabihf)

In our last successful build, armhf is nowhere to be seen, so I'm not sure precisely how that check-and-set of armhf affects the builtins project. Stepping through the code visually would seem to suggest that the condition is true, since the triple (armv7em-ti-none-eabihf) ends with 'hf' and the arch (armv7em) starts with 'arm'. Indeed it does trigger, but only in the compiler-rt build.

Yeah presumably this only happens for cases when building the full compiler-rt. Still a situation that might produce code different from what's expected, but apparently not something that has been an issue for someone yet....