mstorsjo 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"
----------------
mati865 wrote:
> 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))
> ```
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80425/new/
https://reviews.llvm.org/D80425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
