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

Fri May 22 14:30:20 PDT 2020

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80425