[PATCH] D134165: [llvm] prefix linker flag on non-MSVC compilers with `-Wl,`

[Ashay Rane via Phabricator via llvm-commits]

ashay-github added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:133
       set(export_file_linker_flag "/DEF:\"${export_file_linker_flag}\"")
+    elseif(WIN32)
+      # If we're compiling on Windows and the compiler is not Visual Studio,
----------------
mstorsjo wrote:
> ashay-github wrote:
> > mstorsjo wrote:
> > > rnk wrote:
> > > > ashay-github wrote:
> > > > > beanz wrote:
> > > > > > Does the same issue occur if you use compilers other than clang? I'm wondering if this change should be clang-specific.
> > > > > Since both gcc and clang use `-Wl` to pass flags to the linker, I imagine that this problem is not clang specific. Unfortunately, I don't have gcc/MinGW setup to try this out.
> > > > I think if one configured a cmake project with clang-cl, CMake would invoke the linker directly, and it would go down the `if (MSVC)` codepath above. The linker would directly understand the /def flag.
> > > I’m not so sure about this change. If building in mingw mode, the linker doesn’t support any ‘/DEF:` flag at all, in pretty sure that this will break such builds.
> > > 
> > > So can you clarify, in exactly what setup does `if(MSVC)` not trigger, but you still have got an msvc style linker that supports the `/DEF:` option?
> > > So can you clarify, in exactly what setup does if(MSVC) not trigger, but you still have got an msvc style linker that supports the /DEF: option?
> > 
> > I see the error when I compile LLVM using clang.exe that comes bundled with Microsoft Visual Studio 2022. If I were to invoke `cl.exe`, then `.def` files are sent to the linker instead of being interpreted by the compiler.
> > 
> > It looks like `ld` can handle `.def` files without any special flags [https://sourceware.org/binutils/docs/ld/WIN32.html#index-using-a-DEF-file], so I think I'd need to add an exception for gcc so as to not break MinGW builds.  Does that seem right to you?  Oh, and thanks for catching this, @mstorsjo!
> So overall, I'm a bit sceptical to this build configuration - is there any reason why you can't build using clang-cl instead?
> 
> I think overall that building LLVM itself with clang in MSVC mode, without using clang-cl, isn't really supported. (CMake itself does support that configuration since CMake 3.15, but building LLVM does a lot of setting/testing/tweaking of compiler flags.)
> 
> I did test building this way, and while this issue seems to be the only hard error right now, the build spews endless amounts of warnings, so it's clearly not a configuration that is supported/maintained yet. On the other hand, if there's a good reason for doing it though and someone wants to work on it to improve it, I guess it can be accepted. But I'd like to hear a reason why the clang-cl interface doesn't work here.
> 
> Anyway, when it comes to linker flags for linkers on Windows, if we're going to support this configuration, there's three different cases:
> - link.exe/lld-link options (the default when using cl/clang-cl)
> - ld.bfd/ld.lld style options, passed via the compiler driver (where they need `-Wl`)
> - link.exe/lld-link options passed via the compiler driver with `-Wl`
> 
> Currently, we can check `if (MSVC)` to detect the first case, and the second one either as an `else()` to `if (MSVC)`, or `if (MINGW)`. But IMO this is the big issue, we don't have a very neat way to detect the third case.
> 
> From digging around a bit, I found that you can check `CMAKE_CXX_SIMULATE_ID`, which is set to MSVC when building with clang-cl - so you could have something like this:
> 
> ```
> if (MSVC)
>     use(-def:${DEFFILE})
> elseif(CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC")
>     use(-Wl,-def:${DEFFILE})
> elseif(MINGW)
>     use(${DEFFILE})
> endif()
> ```
> 
> is there any reason why you can't build using cl instead?

Coincidentally, someone else asked me the same question. If I run into more build issues, I'll probably revert to cl, but I'm hesitant to leave this particular issue as a known, but undocumented, problem.

For this error, though, I have something very similar to what you suggested. It took a while for me to run this through clang under MSVC, MingW gcc with ld, and MingW gcc with lld, but the following seems to satisfy those configurations.

```
     if(WIN32)
       if(MINGW)
         set(export_file_linker_flag "-Wl,\"${export_file_linker_flag}\"")
       elseif(MSVC)
         set(export_file_linker_flag "/DEF:\"${export_file_linker_flag}\"")
       else()
         set(export_file_linker_flag "-Wl,/DEF:\"${export_file_linker_flag}\"")
       endif()
     endif()
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134165