This is an archive of the discontinued LLVM Phabricator instance.

[clang][cmake] Force CMAKE_LINKER for multistage build in case of BOOTSTRAP_LLVM_ENABLE_LLD and MSVC
ClosedPublic

Authored by krisb on May 30 2020, 11:15 AM.

Details

Summary

The issue with LLVM_ENABLE_LLD is that it just passes -fuse-ld=lld
to compiler/linker options which makes sense only for those platforms
where cmake invokes a compiler driver for linking. On Windows (MSVC) cmake
invokes a linker directly and requires CMAKE_LINKER to be specified otherwise
it defaults CMAKE_LINKER to be link.exe.

This patch allows BOOTSTRAP_LLVM_ENABLE_LLD to set CMAKE_LINKER in the case
of building for host Windows. It also skips adding '-fuse-ld=lld' to make
lld-link not warning about 'unknown argument'.

Diff Detail

Event Timeline

krisb created this revision.May 30 2020, 11:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 30 2020, 11:15 AM
phosek added inline comments.Jul 8 2020, 1:29 AM
clang/CMakeLists.txt
759

I don't understand the second part of this condition, can you elaborate? Why not set CMAKE_LINKER to lld-link.exe even if BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"?

krisb updated this revision to Diff 277806.Jul 14 2020, 6:13 AM
krisb edited the summary of this revision. (Show Details)

Changed MSVC -> WIN32 check and simplified the warning fix.

krisb marked an inline comment as done.Jul 14 2020, 6:48 AM
krisb added inline comments.
clang/CMakeLists.txt
759

The only reason that stopped me from adding BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows" case here is that there is no way to keep it working in the future (or, at least, I don't know one). Moreover, the build which sets BOOTSTRAP_CMAKE_SYSTEM_NAME equal to "Windows" is currently broken.
Since the same issue is exposed if to build LLVM with clang/cmake/caches/DistributionExample.cmake on Windows, I included only the changes that contribute to making this configuration buildable. I wanted to avoid making the impression that some other configurations are supported (because they are not) and also avoid adding any dead code that nobody would use and that would easily be broken.

phosek accepted this revision.Jul 21 2020, 10:52 AM

LGTM

clang/CMakeLists.txt
759

I'd prefer to use WIN32 OR BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows" here even if we don't support cross-compilation to Windows now as I hope it's something we're going to support in the future and this will be one less thing we need to address. Alternatively, please at least leave a TODO so it's easier to find later.

This revision is now accepted and ready to land.Jul 21 2020, 10:52 AM
krisb updated this revision to Diff 280835.Jul 27 2020, 3:10 AM

Addressed the review comment.

krisb marked an inline comment as done.Jul 27 2020, 3:32 AM

@phosek thank you for reviewing this!

clang/CMakeLists.txt
759

Okay, I made the condition to be (WIN32 AND NOT BOOTSTRAP_CMAKE_SYSTEM_NAME) OR BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows" to avoid setting CMAKE_LINKER in case of cross-compiling with Windows host and non-Windows target.

This revision was landed with ongoing or failed builds.Jul 28 2020, 1:13 AM
This revision was automatically updated to reflect the committed changes.