[PATCH] D80425: Fix LLVM/Clang builds with mingw toolchain

[Mateusz Mikuła via Phabricator via llvm-commits]

mati865 added inline comments.


================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:967
            CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
-  elseif(LINKER_IS_LLD_LINK)
+  elseif(LINKER_IS_LLD_LINK AND NOT MINGW)
     append("/lldltocache:${PROJECT_BINARY_DIR}/lto.cache"
----------------
mstorsjo wrote:
> mati865 wrote:
> > thieta wrote:
> > > mstorsjo wrote:
> > > > 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")`
> > > 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.
> > > 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.
> > 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)
> ```
> 
> 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))
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80425/new/

https://reviews.llvm.org/D80425