Fix LLVM/Clang builds with mingw toolchain

[thieta]

Summary

These are a collection of small fixes to make LLVM/Clang build with a clang+mingw toolchain to target Windows.

The three commits address the following problems:

• When using LTO we pass --lto-cache-directory to lld - but this option is not supported by the lld MingW driver so it fails with unknown argument.
• Don't symlink the tools - a MingW build version of clang should be assumed to be used on Windows - which doesn't support symlinks correctly - so instead use the copy path of the code for MingW as well.
• The logic for linking libclang with libdl was a bit flawed - use the similar logic as to other places in the CMake build system.

[mstorsjo]

Super-nitpick: If you want to capitalize mingw, it's MinGW (minimalist gnu for windows) 😛 But as that's rather annoying to type, the all-lowercase version is quite fine as well.

clang/tools/libclang/CMakeLists.txt

71

If you say this is the same way it's done elsewhere, then sure - although I have no idea about what the issue is, why I haven't run into it, etc. Normally you wouldn't have a libdl on mingw right? What's the concrete issue you're running into, and in which conditions would one run into it?

llvm/cmake/modules/AddLLVM.cmake

1872

Not entirely sure what this one does - is it just a condition that needs to match the one for create_symlink below?

1898

What's the practical issue you're trying to fix with this one here? If CMAKE_HOST_UNIX, i.e. when cross compiling, it does work fine to create symlinks (saving a bit of disk space and bandwidth). Then when transferring the built products to an actual windows system, they're converted into copies at some point (e.g. when zipping up the results).

llvm/cmake/modules/HandleLLVMOptions.cmake

957

We could certainly add support for it in the mingw driver - ideally with the same name as for ELF, which then would be remapped to the corresponding lld-link option. This takes just a couple lines in lld/MinGW/Options.td, lld/MinGW/Driver.cpp and lld/test/MinGW/driver.test.

967

Do you happen to know why LINKER_IS_LLD_LINK gets set in this case? ld.lld (the ELF linker interface, which then the MinGW driver remaps onto the COFF backend with the lld-link interface) certainly doesn't take lld-link style options. I believe (without diving further into it) that we shouldn't be setting this flag in this combination, but with the option implemented, we should fit it into the case further above, elseif((UNIX OR MINGW) AND LLVM_USE_LINKER STREQUAL "lld")

[thieta]

clang/tools/libclang/CMakeLists.txt

71

The problem here is that on my system find_library() picks up libdl in /usr/lib and then tries to link to it and gives me an error about it. HAVE_LIBDL comes from config-ix.cmake where it's checked if we are on windows or not: https://github.com/llvm/llvm-project/blob/216833b32befd14079130a3b857906f4e301179c/llvm/cmake/config-ix.cmake#L101

So this is how other places uses CMAKE_DL_LIBS like here: https://github.com/llvm/llvm-project/blob/7aaff8fd2da2812a2b3cbc8a41af29774b10a7d6/llvm/lib/Support/CMakeLists.txt#L13

llvm/cmake/modules/AddLLVM.cmake

1872

As the comment above:

# If you're not overriding the OUTPUT_DIR, we can make the link relative in
# the same directory.

so that means that it creates the symlink like clang-10.exe -> clang.exe

So this breaks when using the copy path below.

1898

The problem I tried to fix here was to avoid creating symlinks since the binaries under mingw is always going to be executed on windows - so yeah I could remove the symlinks and redo them as copies later - but I thought it might be better to just do it directly from the build system so that you can build on wsl and use it from the same dir from the windows host.

But yeah I am not married to this change - I think it's not to vital.

llvm/cmake/modules/HandleLLVMOptions.cmake

957

TBH I was trying to avoid that bit of extra work :)

But yeah it would be nice to have it supported there as well - I could maintain this change as a patch locally if you don't think I should have this in there while waiting for doing the change in the lld driver.

967

Yeah I bet that variable is set because I pass LLVM_USE_LINKER=lld but I haven't digged to deeply. I can rework the if statement here when we have the lld option in there.

[mati865]

clang/tools/libclang/CMakeLists.txt

71

I also had this issue in MSYS2 but used -DCMAKE_SYSTEM_IGNORE_PATH=/usr/lib to get around it.
MSYS2 has Cygwin like (so UNIX like) environment in /usr/ and /mingw{32,64} for Mingw-w64.

llvm/cmake/modules/HandleLLVMOptions.cmake

967

Yeah I bet that variable is set because I pass LLVM_USE_LINKER=lld but I haven't digged to deeply.

It does use lld-link when you use LLVM_USE_LINKER=lld.

The problem lies in this line:

append("/lldltocache:${PROJECT_BINARY_DIR}/lto.cache"

For MinGW that should read:

append("-Wl,/lldltocache:${PROJECT_BINARY_DIR}/lto.cache"

That's because you are passing this flag to GCC/Clang.

[mstorsjo]

clang/tools/libclang/CMakeLists.txt

71

Ah, I see.

I build with -DCMAKE_FIND_ROOT_PATH=$CROSS_ROOT -DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY - but yeah, this fix probably is nice to have in any case.

llvm/cmake/modules/AddLLVM.cmake

1898

Oh, right, WSL - although - I just tested that, symlinking an exe to another name in WSL, and executing it in cmd.exe, and it seemed to work as well.

In any case, not totally opposed, but shouldn't it be if(CMAKE_HOST_UNIX AND NOT WIN32) in that case, i.e. building on a unix host, and not building for a win32 target? And in that case, it should be split out to a separate review with a much wider audience with more of the windows stakeholders involved.

llvm/cmake/modules/HandleLLVMOptions.cmake

967

For MinGW that should read:

append("-Wl,/lldltocache:${PROJECT_BINARY_DIR}/lto.cache"

We're adding this option properly in the mingw frontend, so we shouldn't do the hacky way of passing lld-link options via the mingw frontend by passing them as /option.

And the reason LINKER_IS_LLD_LINK is set seems to be this:

if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld") OR LLVM_ENABLE_LLD)

Perhaps that should be changed into

if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld" AND NOT MINGW) OR LLVM_ENABLE_LLD)

[mati865]

llvm/cmake/modules/HandleLLVMOptions.cmake

967

We're adding this option properly in the mingw frontend, so we shouldn't do the hacky way of passing lld-link options via the mingw frontend by passing them as /option.

I was about to add that and one more LTO related option but thought "what if LLVM should link with lld-link on MinGW?" and eventually forgot about it.

And the reason LINKER_IS_LLD_LINK is set seems to be this:

if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld") OR LLVM_ENABLE_LLD)

Perhaps that should be changed into

if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld" AND NOT MINGW) OR LLVM_ENABLE_LLD)

It won't work if somebody uses LLVM_ENABLE_LLD=ON instead of LLVM_USE_LINKER=lld which is supposed to be equivalent.
That raises question how LLVM_ENABLE_LLD=ON does even work on UNIX platforms.

IMO this would be better:

if(CMAKE_LINKER MATCHES "lld-link" OR MSVC AND (LLVM_USE_LINKER STREQUAL "lld" OR LLVM_ENABLE_LLD))

[mstorsjo]

llvm/cmake/modules/HandleLLVMOptions.cmake

967

I was about to add that and one more LTO related option but thought "what if LLVM should link with lld-link on MinGW?" and eventually forgot about it.

I don't think that really is a supposedly supported setup - lld-link wouldn't find any libs to link against etc, as the compiler driver is supposed to pass those as -L options in mingw/unix style tools.

It won't work if somebody uses LLVM_ENABLE_LLD=ON instead of LLVM_USE_LINKER=lld which is supposed to be equivalent.
That raises question how LLVM_ENABLE_LLD=ON does even work on UNIX platforms.

Either it doesn't work and nobody actually use it that way, or the effects of LINKER_IS_LLD_LINK are mostly harmless.

IMO this would be better:

if(CMAKE_LINKER MATCHES "lld-link" OR MSVC AND (LLVM_USE_LINKER STREQUAL "lld" OR LLVM_ENABLE_LLD))

Yeah, that looks sensible.

[thieta]

This comment was removed by thieta.

[mati865]

llvm/cmake/modules/HandleLLVMOptions.cmake

967

I don't think that really is a supposedly supported setup - lld-link wouldn't find any libs to link against etc, as the compiler driver is supposed to pass those as -L options in mingw/unix style tools.

Well, it worked both ways (lld-link and ld.lld) with slight changes to CMake files when using your toolchain ;)

[thieta]

This comment was removed by thieta.

[thieta]

I am planning to revise this one now that we have thinlto-cache-dir option landed here are my plans:

• Keep the libdl patch as is (seems like there are no more comments on this).
• Remove the symlink patch for now and potentially move that to another patch
• Rework the cache-dir option so that it passes the same option as we pass to ELF lld.

Is that what everyone would expect?

[mstorsjo]

In D80425#2052607, @thieta wrote:

I am planning to revise this one now that we have thinlto-cache-dir option landed here are my plans:

• Keep the libdl patch as is (seems like there are no more comments on this).
• Remove the symlink patch for now and potentially move that to another patch
• Rework the cache-dir option so that it passes the same option as we pass to ELF lld.

Is that what everyone would expect?

Sounds good to me, but ideally I'd like to split all three issues to separate commits/reviews, as it's three quite independent issues.

[thieta]

Sounds good - I'll close this one and open three new ones.