[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 "$<TARGET_FILE_NAME:${target}>")
     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