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

[Martin Storsjö via Phabricator via llvm-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 "$<TARGET_FILE_NAME:${target}>")
     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