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

Wed Sep 21 09:04:28 PDT 2022

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:
> > > 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()
> > ```
> Thanks for taking the time to actually test building this - that does indeed take some time.
> 
> I don't think we should add an extra `if(WIN32)` here - the whole section about making a def file is specific for windows already in itself. Instead we can make the surrounding `else()` into `elseif(WIN32)`.
> 
> Secondly, while your suggestion works, I would prefer to explicitly check for this configuration (`CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC"`), than to assume with a generic `else()` that it refers to this rather specific/uncommon environment. When reading cmake code, it's otherwise not very evident to the reader that a generic `else()` means clang-msvc-mode-with-gnu-driver.
> 
> Also thirdly, I realized that we don't need a specific action for mingw here - the current behaviour (where there's just an `if (MSVC)` and doesn't do anything for mingw) works fine for mingw, since it just passes the file name as linker flag, which gets forwarded through the compiler driver to the linker as an input file.
> 
> That is, I think my preferred form would be something like this:
> ```
> @@ -118,7 +118,7 @@ function(add_llvm_symbol_exports target_name export_file)
>        set_property(TARGET ${target_name} APPEND_STRING PROPERTY
>                     LINK_FLAGS "  -Wl,--version-script,\"${CMAKE_CURRENT_BINARY_DIR}/${native_export_file}\"")
>      endif()
> -  else()
> +  elseif(WIN32)
>      set(native_export_file "${target_name}.def")
> 
>      add_custom_command(OUTPUT ${native_export_file}
> @@ -130,6 +130,12 @@ function(add_llvm_symbol_exports target_name export_file)
>      set(export_file_linker_flag "${CMAKE_CURRENT_BINARY_DIR}/${native_export_file}")
>      if(MSVC)
>        set(export_file_linker_flag "/DEF:\"${export_file_linker_flag}\"")
> +    elseif(CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC")
> +      set(export_file_linker_flag "-Wl,/DEF:\"${export_file_linker_flag}\"")
> +    elseif(MINGW)
> +      # ${export_file_linker_flag}, which is the plain file name, works as is
> +      # when passed to the compiler driver, which then passes it on to the
> +      # linker as an input file.
>      endif()
>      set_property(TARGET ${target_name} APPEND_STRING PROPERTY
>                   LINK_FLAGS " ${export_file_linker_flag}")
> ```
> That makes the mingw case clearer so that it isn't forgotten/missed by the reader. Alternatively we could add at least quoting around it, like for the other `/DEF` cases too. Passing it with `-Wl,` doesn't seem to be necessary, so let's not change that aspect here since it's not needed.
> 
> 
> Maybe we should even add comments explaining the two `if (MSVC)` and `(CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC")` cases, e.g. like this:
> ```
> if(MSVC) # cl.exe or clang-cl, i.e. MSVC style command line interface
> ..
> elseif (CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC") # clang in msvc mode, calling a link.exe/lld-link style linker
> ...
> ```
Thanks for practically rewriting the change! :)

I did take the liberty of adding a catch-all block so that any unsupported case doesn't go unhandled.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134165