Differential D24156
[CMake] Explicitly add --target option to compiler flags Authored by beanz on Sep 1 2016, 2:20 PM.
Details Much of the non-Darwin build system assumes that COMPILER_RT_DEFAULT_TARGET_TRIPLE is the default target triple for the compiler being used. With clang as your compiler this isn't necessarily true. To ensure that the rest of the build system behaves as expected this patch adds "--target=${COMPILER_RT_DEFAULT_TARGET_TRIPLE}" to the compiler flags for C, CXX and ASM sources.
Diff Detail
Event TimelineComment Actions I think this is a good patch (and simplifies cmake invocations for sure). However, our use of COMPILER_RT_DEFAULT_TARGET_TRIPLE isn't entirely consistent with the --target expected by clang. Two particular cases that come to mind for me are linux arm related. For linux arm with soft float, we expect armv7-linux-gnueabi as --target, but arm-linux-gnueabi when parsing COMPILER_RT_DEFAULT_TARGET_TRIPLE. androideabi has an analagous issue, and I've also seen issues on Linux with i386 vs i686. I'm not sure how much re-working of the build is required to make thing consistent in all cases, but I do think that the consistency would be a good thing to have. Comment Actions Hi Chris, Let me get my bearings on RT's CMake system... With regards to COMPILER_RT_DEFAULT_TARGET_TRIPLE:
I don't think it's case 1, as it wouldn't make sense... Case 2 would need a "list of default triples" in CMake, which won't get the ABI issue right. Case 3 would be mostly correct because the GNU toolchain has names like "arm-linux-gnueabihf-gcc" or "arm-linux-androideabi-gcc", which also describes itself to "Target: arm-none-eabi" (via arm-none-eabi-gcc -v), which is close to what you want. If we had some detection like case 3, we could use case 4 to fix the remaining issues. Now, the important issue to consider is how run time libraries are normally used. In the case of "gnueabhf", we already have most floating point instructions implemented in hardware, but not all calls in RT are FP, so you still want RT to provide you with the remaining cases. But some toolchains build files assuming that run time libraries are always "soft-float", even though the target is "hard-float". For eample, Compiler-RT's implementations for floating point (libs/builtins/arm) use R0 instead of D0. But this is not always the case, and depends on the toolchain, optimisation levels or available libraries. So, defaulting to "whatever the toolchain you're using identifies itself as" is a good strategy, and can take care of most cases. But being able to override, preferably using only one flag which gets passed along all the similar decisions, would be necessary to get the remaining cases correctly. The case of "arm-none-eabi" is even more complicated (but in a way, simpler - bear with me). That triple is a generic triple that defaults to "ARMv4 soft-float". But embedded targets can be anything, so it's almost *always* the case (unless you're using StrongARM) that you'll change the meaning of the triple via flags, which the toolchain will not tell you directly. This may seem impossible to get it right, but users end up having to pass the triple and a lot of architecture flags to define what they want. And, just like magic, if you pass the triple and the flags, the library will hopefully, be compiled as expected. All in all, library developers/users know what they want and how to get it. We just need to give them a way to do so with the minimal amount of flags. If they want an embedded target with a generic library but a heavily optimised ARMv7A+NEON source that calls libraries as soft-float, they'll know how to get that. We just need to allow them to build the run time libraries in a way they expect. On that topic, I think @compnerd can help you outline all the weird way people build run time libraries, but probably a bit too much for this patch. cheers, Comment Actions COMPILER_RT_DEFAULT_TARGET_TRIPLE defaults today to whatever the TARGET_TRIPLE is from LLVM, which should be the default target triple of clang. It can be overridden in a dozen or so ways, but that is the default expected behavior. The complication in the build system is that Compiler-RT's build assumes that COMPILER_RT_DEFAULT_TARGET is the compiler's default target, even if it isn't. This comes into play in the target tests where it differentiates Arm32 from AArch64 by passing the -m64 compiler flag, and it passes -mcpu to the ArmV7 builds without passing -target. In general these issues cause the configuration checks to fail or silently incorrectly succeed, when you are building multi-arch libraries with a multi-targeting compiler. I'm trying to come up with a solution where I can support building multi-targeting cross-compilers simply from a single configuration. My experimental patch is P7504, this supports configuring LLVM & Clang something like: cmake -G ... -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" -DLLVM_BUILTIN_TARGETS="i686-unknown-freebsd;armv7-unknown-freebsd;aarch64-unknown-freebsd" Which will generate builtins-<triple> targets as part of all. Allowing you to simply build a multi-target compiler from a single build configuration. If the COMPILER_RT_DEFAULT_TARGET is arm-linux-gnueabi, and that gets passed to -target is that a problem? Anecdotally it seems to me that it isn't, but @fjricci implied that it is. Comment Actions Ok.
The -mcpu case is real, but you should not use -m64 to pick between 32 and 64 bit ARM. This works on x86 because they're the same back-end, but should not work between AArch32 and AArch64.
Yes, this is the *real* problem.
This would be awesome! But how do you expect to pass the other flags (-mfloat-abi, -mtune, etc) to each individual target?
I don't know if @fjricci was referring to the same problem I was, but in my case it's not a problem, per se, to pass that to -target, unless this is not what you want. So, if this comes from -DLLVM_BUILTIN_TARGETS, than I assume you know what you're doing and it works fine. If it comes from the default LLVM_TARGET, then it's likely not what you want, and silent errors will be the enemy. All in all, I think we should move to a more annoying driver that understand the problems across architectures (for example, error out instead of using the system linker for cross architectures), but that's orthogonal to your patch. The only problem I see with your patch as is is that you can't still pass the correct architectural options to each target, which may be restrictive, but not *broken*. Otherwise, looks good. cheers, Comment Actions Ok. If the intended behavior of COMPILER_RT_DEFAULT_TARGET_TRIPLE is to match the LLVM triple passed in TARGET_TRIPLE, I can work on a patch to fix a few of the inconsistencies I found when using this patch for my builds. armv7 hard-float is the trickiest, but fixing it should just require some small changes to how we parse things. Comment Actions It looks like this change won't break any of the targets I use. However, I still need to manually set CMAKE_C_COMPILER_TARGET or add --target to CMAKE_C_FLAGS for some targets, which I think somewhat defeats the purpose of this patch. The problem is that the initial cmake compiler test will fail when cross-compiling, because it will attempt to compile and link for the default architecture instead of the architecture specified by COMPILER_RT_DEFAULT_TARGET_TRIPLE. This will fail, for example, when the sysroot and system libs are built for the target architecture, which is different from the default architecture that the compiler is targeting. It will also fail if there are architecture-specific flags in CMAKE_C_FLAGS. So, I don't mind this change being merged, as it won't break anything, as far as I can tell, but I don't think it will fully solve the problems you're trying to solve by using it. Comment Actions Yes, this is a known issue (tm) with the current CMake approach. :(
I agree. Chris, I'm also ok with this going in if you know where you're going with it. I know of the problems, but I don't have any solution, sorry. So I'll trust you know better than me (even if it's just a gut feeling), we can easily change later to a better solution. I'm accepting the revision in that respect. cheers, Comment Actions Hmm, looks like I can't remove my "request changes", except by accepting. So I'll accept as well. |