The compiler-rt tests build various components with varying compiler and link flags while also using custom compile and link mechanisms. When using a non-default toolchain for a "native compile", the tests do not appear to respect CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN. This could lead to link errors, etc. This patch attempts to address this by building upon the current "cross compile" support associated with COMPILER_RT_TEST_COMPILER_CFLAGS by adding a COMPILER_RT_TOOLCHAIN_CFLAGS, which is COMPILER_RT_TEST_COMPILER_CFLAGS should the latter be set and takes CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN into account otherwise.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCRT Compiler Runtime
- Build Status
Buildable 28785 Build 28784: arc lint + arc unit
Event Timeline
Comment Actions
Patch LGTM, but I think someone more familiar with compiler-rt cmake needs to be the one to accept it.
Comment Actions
Ping 3. We've been using this locally to support our build environments with non-default native toolchains. I believe this patch is generally applicable for such a use case.
| cmake/config-ix.cmake | ||
|---|---|---|
| 202 | Why don't we use COMPILER_RT_TEST_COMPILER_CFLAGS here instead of COMPILER_RT_TOOLCHAIN_CFLAGS? It seems to me that using COMPILER_RT_TEST_COMPILER_CFLAGS is a little more consistent with the usage in the if branch above. | |
| cmake/config-ix.cmake | ||
|---|---|---|
| 202 | The if branch above is a cross-compile only case. It is the exception rather than the rule here. | |
Comment Actions
If I've understood this correctly the patch introduces COMPILER_RT_TOOLCHAIN_CFLAGS which we don't expect users to set directly, but instead set -gcc-toolchain if the compiler is clang and CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is set?
My understanding is that CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is not respected when cross-compiling compiler-rt for Arm and AArch64. I've always had to pass in --gcc-toolchain manually in the C flags. Is there any reason why we should make a distinction between a native and a cross-compile here? I'd say we were more likely to want --gcc-toolchain when cross-compiling. If that is the case it would be better to find a way of using the existing COMPILER_RT_TEST_CFLAGS? This would also make it a bit less confusing as COMPILER_RT_TOOLCHAIN_CFLAGS is only used in the context of tests and COMPILER_RT_TEST_CFLAGS is where I would expect to see that being used.
An issue encountered when preparing this change was that COMPILER_RT_UNITTEST_LINK_FLAGS is dropped in many places, unlike COMPILER_RT_UNITTEST_CFLAGS. This patch also attempts to remove that inconsistency.
Personally I'd recommend splitting that part out into a separate patch, it will make it a bit easier to untangle which parts of the patch are concerned with the toolchain changes.
| cmake/config-ix.cmake | ||
|---|---|---|
| 192 | At a first glance this doesn't look right in the context of a native arm or aarch64 build of compiler-rt. I'll need to check to see what happens natively. | |
| 196 | At this stage are COMPILER_RT_TEST_COMPILER_CFLAGS the same as COMPILER_RT_TOOLCHAIN_FLAGS? As Line 222 cmake/base-config-ix.cmake looks to be doing if(COMPILER_RT_TEST_COMPILER_CFLAGS)
# COMPILER_RT_TEST_COMPILER_CFLAGS is used for cross-compiling.
set(COMPILER_RT_TOOLCHAIN_CFLAGS ${COMPILER_RT_TEST_COMPILER_CFLAGS})If that is the case then append to flags_out is the same on both branches. | |
Comment Actions
Yes.
My understanding is that CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is not respected when cross-compiling compiler-rt for Arm and AArch64. I've always had to pass in --gcc-toolchain manually in the C flags. Is there any reason why we should make a distinction between a native and a cross-compile here?
I am only able to speak to my own use case. There is existing logic for "cross-compile" cases that I cannot speak to (and therefore cannot change with confidence).
I'd say we were more likely to want --gcc-toolchain when cross-compiling. If that is the case it would be better to find a way of using the existing COMPILER_RT_TEST_CFLAGS? This would also make it a bit less confusing as COMPILER_RT_TOOLCHAIN_CFLAGS is only used in the context of tests and COMPILER_RT_TEST_CFLAGS is where I would expect to see that being used.
COMPILER_RT_TEST_CFLAGS is not showing up in my tree; perhaps you meant COMPILER_RT_UNITTEST_CFLAGS? As it is, the patch does feed the option into COMPILER_RT_UNITTEST_CFLAGS (through COMPILER_RT_TOOLCHAIN_CFLAGS). Note that COMPILER_RT_UNITTEST_CFLAGS does not factor into the various *_TEST_TARGET_CFLAGS used for the lit tests; so, adding the option directly into COMPILER_RT_UNITTEST_CFLAGS does not produce the desired effect.
An issue encountered when preparing this change was that COMPILER_RT_UNITTEST_LINK_FLAGS is dropped in many places, unlike COMPILER_RT_UNITTEST_CFLAGS. This patch also attempts to remove that inconsistency.
Personally I'd recommend splitting that part out into a separate patch, it will make it a bit easier to untangle which parts of the patch are concerned with the toolchain changes.
Sure, I'll post that as a separate patch that is a parent of this one and refresh this patch to contain only the toolchain changes.
| cmake/config-ix.cmake | ||
|---|---|---|
| 192 | Thanks; I am not familiar with the builds for those platforms. If there is something to be done that logically belongs with this patch, then we consider it. | |
| 196 | Yes, that's right; however, using COMPILER_RT_TEST_COMPILER_CFLAGS alongside the similarly named COMPILER_RT_TEST_COMPILER is perhaps beneficial in terms of not requiring extra analysis for the casual consumer of the code. Also, the block of code here was otherwise untouched. | |
Comment Actions
Thanks for splitting the patch, I'll take a look in detail as soon as I can, apologies I'm at a conference at the moment so it may be a few days.. I've got some experience and requirements from the cross-compilation side, although I'm certainly not the only one. At a high level I'd like to try and support --gcc-toolchain in both cross and native builds consistently if it is all possible,
Comment Actions
I've had a chance to look into this a bit more, although my understanding is far from complete. At the moment I think the patch is too specific to the use-case, I'm not an expert in this area so I welcome a second opinion. I don't want to block this, but I also not confident enough to approve either.
As I understand it compiler-rt is built by make/ninja from CMake derived files so CMake only features such as CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT will affect the make/ninja build files. The tests are run by llvm-lit so we wouldn't expect CMake only features to affect the output unless we convert them to something llvm-lit understands. We've also got a situation where cross-compiling and native tests have different options available to them:
- Cross compiled test runs can use COMPILER_RT_TEST_COMPILER_CFLAGS to pass in --gcc-toolchain=... --sysroot=... duplicating any use of CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT, but at least it is possible.
- Native test runs always pick up the default host compiler, and the default architecture C-flags and don't have a way (at least as far as I can work out in time available) to pass through --gcc-toolchain.
Possible ways to fix this are:
- Convert the CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT to a form that llvm-lit understands, either by converting them to c-flags or adding support to them in llvm-lit so that they have their own line in lit.common.configured.in
- Apply these to the cross compilation options as well.
- Allow COMPILER_RT_TEST_COMPILER_CFLAGS to be passed to native builds in addition to the exiting flags. This would mean duplication of CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT on the COMPILER_RT_TEST_COMPILER_CFLAGS but at least it would be consistent.
I would prefer if we could come up with a solution for both cross and native cases, as well as supporting CMAKE_SYSROOT as both this and CMAKE_C(XX)_EXTERNAL_TOOLCHAIN are used when cross-compiling the tests on linux platforms.
| cmake/base-config-ix.cmake | ||
|---|---|---|
| 226 | What would happen if CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN were set? Is this guaranteed to also set CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN? The compiler-rt builtins don't have any C++ code so I could expect someone just compiling the builtins to naturally use the C version of the flag. | |
| cmake/config-ix.cmake | ||
| 192 | I've tried building compiler-rt from a standalone directory on an AArch64 machine. It does indeed go down this path even though it is technically not cross compiling. It does happen to work because COMPILER_RT_TEST_COMPILER defaults to the compiler that built compiler-rt which is clang. However this is not right in general, for example I suspect RiscV32 might wish to be cross-compiled here. I suspect we'll need to test for CMAKE_CROSSCOMPILING here, which may cause some people's builds to fail at first as CMAKE_CROSSCOMPILING is only set when CMAKE_SYSTEM_NAME is set, but at least this is the documented CMake way to detect cross compilation. I don't think that this needs to be fixed here. It would be best addressed with a separate patch if it became important. One thing I think it does show is that there isn't any reason to not pass CMAKE_C(XX)_EXTERNAL_TOOLCHAIN through when cross-compiling and the compiler is clang as the expectation is that this is the same clang that built the builtins. | |
Comment Actions
That is also my understanding. I also believe that it is working sufficiently today for people using such configurations, or at least sufficiently so that improving the situation was not seen as being urgent.
- Native test runs always pick up the default host compiler, and the default architecture C-flags and don't have a way (at least as far as I can work out in time available) to pass through --gcc-toolchain.
This is the reason why I produced the patch under review here.
Possible ways to fix this are:
- Convert the CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT to a form that llvm-lit understands, either by converting them to c-flags or adding support to them in llvm-lit so that they have their own line in lit.common.configured.in
This patch implements the above approach for CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN. The various *_TEST_TARGET_CFLAGS variables are picked up by their respective lit.site.cfg.in files.
- Apply these to the cross compilation options as well.
- [ ... ]
I would prefer if we could come up with a solution for both cross and native cases, as well as supporting CMAKE_SYSROOT as both this and CMAKE_C(XX)_EXTERNAL_TOOLCHAIN are used when cross-compiling the tests on linux platforms.
Not having this patch is actively blocking the native build cases that use CMAKE_C(XX)_COMPILER_EXTERNAL_TOOLCHAIN. While improving the cross-compiling cases would be nice, I don't think those cases are currently broken.
| cmake/base-config-ix.cmake | ||
|---|---|---|
| 226 | I'll switch this to CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN since there is such a concern. | |
| cmake/config-ix.cmake | ||
| 192 |
I concerned with moving forward with such a change, because COMPILER_RT_TEST_COMPILER is available as a customization point. | |
Comment Actions
I think once you know what to you need to do it suffices, however finding out how to do it is more difficult. I think people have largely been reliant on the mailing list for guidance.
...
Possible ways to fix this are:
- Convert the CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT to a form that llvm-lit understands, either by converting them to c-flags or adding support to them in llvm-lit so that they have their own line in lit.common.configured.in
This patch implements the above approach for CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN. The various *_TEST_TARGET_CFLAGS variables are picked up by their respective lit.site.cfg.in files.
In the suggestion I was thinking more along the lines of a separate toolchain and sysroot rather than passing them as C Flags, something like
set_default("gcc_toolchain", "@GCC_TOOLCHAIN@")
set_default("sysroot", "@SYSROOT")However I'm not really sure that adds much value other than matching CMake a bit more closely. I guess it would hand off the decision over what to do with the flags to lit, which may have more of an idea of what to do with them.
- Apply these to the cross compilation options as well.
- [ ... ]
I would prefer if we could come up with a solution for both cross and native cases, as well as supporting CMAKE_SYSROOT as both this and CMAKE_C(XX)_EXTERNAL_TOOLCHAIN are used when cross-compiling the tests on linux platforms.
Not having this patch is actively blocking the native build cases that use CMAKE_C(XX)_COMPILER_EXTERNAL_TOOLCHAIN. While improving the cross-compiling cases would be nice, I don't think those cases are currently broken.
I agree with you that the native build is not supporting external_toolchain. It is also true that cross compiling can use an alternative method to work around it.
My main concerns are:
- We're adding another specific case to just the native part making the CMake files harder to understand, where there is an opportunity to support both native and cross. I may be alone here, I'd welcome alternative opinions from others to check?
- To me the name COMPILER_RT_TOOLCHAIN_CFLAGS matches the --gcctoolchain option well. However it doesn't intuitively say "merged flags from COMPILER_RT_TEST_COMPILER_FLAGS", the name is too specific. Having both native and cross use COMPILER_RT_TEST_COMPILER_FLAGS would be a way to fix this. Alternatives could be picking a better name that suites both, or perhaps adding both and not merging the flags.
I care about the merging of the COMPILER_RT_TEST_COMPILER_CFLAGS into a name like COMPILER_RT_TOOLCHAIN flags more than I care about both cross and native being the same. If I'm the only one that cares about the equivalence of cross and native I'd like to see if we can resolve the naming of COMPILER_RT_TOOLCHAIN.
Apologies for rambling on. I'm at Eurollvm at the moment, if you happen to be here, I'm happy to talk about it.
| compiler-rt/test/lit.common.configured.in | ||
|---|---|---|
| 10 ↗ | (On Diff #304015) | Please see change in this patch to compiler-rt/cmake/base-config-ix.cmake. COMPILER_RT_TEST_COMPILER_CFLAGS is respected. |
Comment Actions
This is a really old patch that I had in my usual build tree and needed to manually rebase recently.
It might be helpful for someone, but I think the original context where I developed it is not exactly the build configuration I wanted.
In particular, I had a "monorepo" build (as explained by Louis Dionne in https://lists.llvm.org/pipermail/llvm-dev/2020-October/145997.html) at the time (and such a build was not suitable for the combination of build+target toolchain).
What would happen if CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN were set? Is this guaranteed to also set CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN? The compiler-rt builtins don't have any C++ code so I could expect someone just compiling the builtins to naturally use the C version of the flag.