lld has LTO support, if requested we should add a dependency on lld rather than LLVMgold when doing LTO bootstrap build.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Make sense to me, but I don't think it is enough: LLVM_ENABLE_LLD should be passed to stage-2, and it also should be set to the absolute path of the stage-1 lld build.
| CMakeLists.txt | ||
|---|---|---|
| 516 ↗ | (On Diff #77916) | This dep does not make sense when using either lld or gold by the way. |
Comment Actions
What is search directories order in Clang? It's reasonable it it tries to look into same directory first. In this case CMAKE_C_COMPILER/CMAKE_CXX_COMPILER should be enough for later stages.
Comment Actions
It's sufficient, I just tested it. I'm not actually sure if LLVM_ENABLE_LLD here is correct, LLVM_ENABLE_LLD forces the use of lld, but lld might not be available during first stage. We need LLVM_ENABLE_LLD to be set, but only for the second stage (which is something that can be done in the cache file), in the first stage we should probably just check whether lld source is available and we're building it, but I'm not sure what's the easiest/cleanest way to do that?
Comment Actions
-DBOOTSTRAP_ LLVM_ENABLE_LLD is the right way to check for stage-2 options.
If you look just a few lines above your check you'll see an example of check for BOOTSTRAP_LLVM_ENABLE_LTO
Comment Actions
I think this should be handled in higher level script (utils/release/test-release.sh or similar), not in CMake. CMake compiler tests just need to fail when LLVM_ENABLE_LLD is used without actually having them.
Comment Actions
I meant that multi-stage build is processed by higher level script. So it should take care about consistency of source code srt versus later stages build options.
Comment Actions
We have a 2-stage build directly with a single CMake invocation, this is what this code path is about.
Are you referring to this situation or are you referring to 2-stage builds that are not "visible" from CMake itself and requires to first build stage-1 before configuring stage-2 with a second cmake invocation? (If I understand correctly I suspect you meant the latter)
Comment Actions
How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to stage-2?
| CMakeLists.txt | ||
|---|---|---|
| 534 ↗ | (On Diff #77916) | What if not by the way? Should we error out here? What is the expected behavior? |
Comment Actions
Sufficient as in Clang looks for lld in the same directory where clang/clang++ binary is first so we don't need to explicitly pass the path to lld to later stages.
| CMakeLists.txt | ||
|---|---|---|
| 534 ↗ | (On Diff #77916) | I assume that's platform dependent; if some platforms (other than Darwin) use linker other than gold or lld for LTO build, erroring out here might break them. I don't think there are any such platforms though because they'd be broken already as we currently always try to use LLVMgold which causes CMake error in case we're not building with Binutils (that's how I discovered this issue, since we're using lld rather than gold on our platform). |
Comment Actions
OK, but still, LLVM_ENABLE_LLD needs to be passed to stage-2, so it needs to be actually BOOTSTRAP_LLVM_ENABLE_LLD.
I looked at all the CMake _PASSTHROUGH and didn't find it mentioned anywhere. We could make it auto-forwarded in this case maybe, @beanz is best to answer this.
Have you looked into turning if(LLVM_ENABLE_LLD) into if(BOOTSTRAP_LLVM_ENABLE_LLD)?
Technically we may want to have the stage-2 linked with lld even if lld is not on the system and not available during stage1.
Comment Actions
Yes, I just finished a test build and BOOTSTRAP_LLVM_ENABLE_LLD seems to work, see the updated diff.
Comment Actions
What about the following logic?
if(BOOTSTRAP_LLVM_ENABLE_LLD)
add_dependencies(clang-bootstrap-deps lld)
endif()
if(APPLE)
# on Darwin we need to set DARWIN_LTO_LIBRARY so that -flto will work
# using the just-built compiler, and we need to override DYLD_LIBRARY_PATH
# so that the host object file tools will use the just-built libLTO.
# However if System Integrity Protection is enabled the DYLD variables
# will be scrubbed from the environment of any base system commands. This
# includes /bin/sh, which ninja uses when executing build commands. To
# work around the envar being filtered away we pass it in as a CMake
# variable, and have LLVM's CMake append the envar to the archiver calls.
set(LTO_LIBRARY -DDARWIN_LTO_LIBRARY=${LLVM_SHLIB_OUTPUT_INTDIR}/libLTO.dylib
-DDYLD_LIBRARY_PATH=${LLVM_LIBRARY_OUTPUT_INTDIR})
elseif(NOT WIN32)
add_dependencies(clang-bootstrap-deps llvm-ar llvm-ranlib)
if(NOT LLVM_ENABLE_LLD AND LLVM_BINUTILS_INCDIR)
add_dependencies(clang-bootstrap-deps LLVMgold)
endif()
set(LTO_AR -DCMAKE_AR=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-ar)
set(LTO_RANLIB -DCMAKE_RANLIB=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-ranlib)
endif()
endif()I'm not even sure why we don't use llvm-ar and llvm-ranlib on OSX by the way, this makes the logic more complicated here, but there might be a good reason for that.
Comment Actions
AFAIK lld on Darwin doesn't support LTO (last I heard it cannot even self-host at the moment) so I'm not sure if it makes sense to enable it if(APPLE).
Comment Actions
OK, we could error instead of ignoring then.
But you still need to restore the dependency on libLTO, unless I'm missing something.
Comment Actions
On Darwin you don't want to set DARWIN_LTO_LIBRARY on the next stage if you're intending the next stage to use lld because that sets flags that aren't supported by lld.
Comment Actions
Right, but that's a separate issue than what is addressed here right?
The issue already exists: setting both BOOTSTRAP_LLVM_ENABLE_LTO and LLVM_ENABLE_LLD can't work on Darwin right now.
Comment Actions
Yep. Can we maybe add an error to that effect? I really don't want the build system ignoring invalid configurations.
Otherwise LGTM!
Comment Actions
@beanz : don't you think that it should be handled at the top level and not in the bootstrap logic?
Right now we don't error when invoking cmake with -DLLVM_ENABLE_LLD=ON and -DLLVM_ENABLE_LTO=ON on Darwin.
Comment Actions
That might a better approach, I tried invoking -DLLVM_ENABLE_LLD=ON on Darwin and it fails, even without LLVM_ENABLE_LTO; the CXX_SUPPORTS_LLD check which checks whether lld is supported by the driver succeeds, but all subsequent ones fail because lld fails.
| cfe/trunk/CMakeLists.txt | ||
|---|---|---|
| 531 | I come back to this a bit late, sorry, but I'm not sure I understand why this dependency on the bootstrap happens only when LTO is used? | |
| cfe/trunk/CMakeLists.txt | ||
|---|---|---|
| 531 | We should probably separate LLVM_ENABLE_LTO and LLVM_ENABLE_LLD. You can use LLD without LTO on a wider variety of platforms (including OS X), and you might want to use LLD without LTO. | |
I come back to this a bit late, sorry, but I'm not sure I understand why this dependency on the bootstrap happens only when LTO is used?