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

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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


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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.


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

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

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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?


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

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

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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

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

[Mateusz Mikuła via Phabricator via cfe-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:
> > 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.
> 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 ;)


Repository:
  rG LLVM Github Monorepo

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

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

[Martin Storsjö via Phabricator via cfe-commits]

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

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

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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

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

[Mateusz Mikuła via Phabricator via cfe-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: clang/tools/libclang/CMakeLists.txt:71
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 

mati865 wrote:
> thieta wrote:
> > mstorsjo wrote:
> > > 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?
> > 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
> 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.
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.



Comment at: llvm/cmake/modules/AddLLVM.cmake:1898
+  if(CMAKE_HOST_UNIX AND NOT MINGW)
 set(LLVM_LINK_OR_COPY create_symlink)
   else()

thieta wrote:
> mstorsjo wrote:
> > 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).
> 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.
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.



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:
> 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)

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

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

mati865 added inline comments.



Comment at: clang/tools/libclang/CMakeLists.txt:71
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 

thieta wrote:
> mstorsjo wrote:
> > 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?
> 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
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.



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"

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.


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

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

[Tobias Hieta via Phabricator via cfe-commits]

thieta marked 5 inline comments as done.
thieta added inline comments.



Comment at: clang/tools/libclang/CMakeLists.txt:71
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 

mstorsjo wrote:
> 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?
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



Comment at: llvm/cmake/modules/AddLLVM.cmake:1872
+if(CMAKE_HOST_UNIX AND NOT MINGW)
   set(dest_binary "$")
 endif()

mstorsjo wrote:
> Not entirely sure what this one does - is it just a condition that needs to 
> match the one for `create_symlink` below?
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.



Comment at: llvm/cmake/modules/AddLLVM.cmake:1898
+  if(CMAKE_HOST_UNIX AND NOT MINGW)
 set(LLVM_LINK_OR_COPY create_symlink)
   else()

mstorsjo wrote:
> 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).
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.



Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:957
+  #
+  # FIXME; mingw lld driver doesn't support any of the options below
   if(APPLE)

mstorsjo wrote:
> 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.
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.



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:
> 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.


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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.




Comment at: clang/tools/libclang/CMakeLists.txt:71
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 

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?



Comment at: llvm/cmake/modules/AddLLVM.cmake:1872
+if(CMAKE_HOST_UNIX AND NOT MINGW)
   set(dest_binary "$")
 endif()

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



Comment at: llvm/cmake/modules/AddLLVM.cmake:1898
+  if(CMAKE_HOST_UNIX AND NOT MINGW)
 set(LLVM_LINK_OR_COPY create_symlink)
   else()

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).



Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:957
+  #
+  # FIXME; mingw lld driver doesn't support any of the options below
   if(APPLE)

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.



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"

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")`


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

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

[Tobias Hieta via Phabricator via cfe-commits]

thieta created this revision.
Herald added subscribers: llvm-commits, cfe-commits, mstorsjo, dexonsmith, 
mgorny.
Herald added projects: clang, LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80425

Files:
  clang/tools/libclang/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -66,9 +66,8 @@
   endif ()
 endif ()
 
-find_library(DL_LIBRARY_PATH dl)
-if (DL_LIBRARY_PATH)
-  list(APPEND LIBS dl)
+if (HAVE_LIBDL)
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 
 option(LIBCLANG_BUILD_STATIC
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -953,6 +953,8 @@
   # time a little since we re-link a lot of the same objects, and significantly
   # improves incremental build time.
   # FIXME: We should move all this logic into the clang driver.
+  #
+  # FIXME; mingw lld driver doesn't support any of the options below
   if(APPLE)
 append("-Wl,-cache_path_lto,${PROJECT_BINARY_DIR}/lto.cache"
CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
@@ -962,7 +964,7 @@
   elseif(LLVM_USE_LINKER STREQUAL "gold")
 append("-Wl,--plugin-opt,cache-dir=${PROJECT_BINARY_DIR}/lto.cache"
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"
CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
   endif()
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1868,7 +1868,7 @@
   if(NOT ARG_OUTPUT_DIR)
 # If you're not overriding the OUTPUT_DIR, we can make the link relative in
 # the same directory.
-if(CMAKE_HOST_UNIX)
+if(CMAKE_HOST_UNIX AND NOT MINGW)
   set(dest_binary "$")
 endif()
 if(CMAKE_CONFIGURATION_TYPES)
@@ -1894,7 +1894,7 @@
 endif()
   endif()
 
-  if(CMAKE_HOST_UNIX)
+  if(CMAKE_HOST_UNIX AND NOT MINGW)
 set(LLVM_LINK_OR_COPY create_symlink)
   else()
 set(LLVM_LINK_OR_COPY copy)


Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -66,9 +66,8 @@
   endif ()
 endif ()
 
-find_library(DL_LIBRARY_PATH dl)
-if (DL_LIBRARY_PATH)
-  list(APPEND LIBS dl)
+if (HAVE_LIBDL)
+  list(APPEND LIBS ${CMAKE_DL_LIBS})
 endif()
 
 option(LIBCLANG_BUILD_STATIC
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -953,6 +953,8 @@
   # time a little since we re-link a lot of the same objects, and significantly
   # improves incremental build time.
   # FIXME: We should move all this logic into the clang driver.
+  #
+  # FIXME; mingw lld driver doesn't support any of the options below
   if(APPLE)
 append("-Wl,-cache_path_lto,${PROJECT_BINARY_DIR}/lto.cache"
CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
@@ -962,7 +964,7 @@
   elseif(LLVM_USE_LINKER STREQUAL "gold")
 append("-Wl,--plugin-opt,cache-dir=${PROJECT_BINARY_DIR}/lto.cache"
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"
CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
   endif()
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1868,7 +1868,7 @@
   if(NOT ARG_OUTPUT_DIR)
 # If you're not overriding the OUTPUT_DIR, we can make the link relative in
 # the same directory.
-if(CMAKE_HOST_UNIX)
+if(CMAKE_HOST_UNIX AND NOT MINGW)
   set(dest_binary "$")