[compiler-rt][MSVC][CMake] Wrap Linker flags for ICX #118496
Conversation
RFC: https://discourse.llvm.org/t/rfc-cmake-linker-flags-need-wl-equivalent-for-intel-c-icx-on-windows/82446 My previous pass missed some flags because I used `-Werror=unknown-argument``, but `/D``, `/I` and `/O` are accepted by clang (even when only linking), but mean different things than intended for `link.exe``. Wrapping the linker with a script revealed these flags.
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Mészáros Gergely (Maetveis) ChangesMy previous pass missed some flags because I used The approach I used to find these is documented as github gist here: https://gist.github.com/Maetveis/00567488f0d6ff74095d91ed306fafc5 Full diff: https://github.com/llvm/llvm-project/pull/118496.diff 2 Files Affected:
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index 20054c6e85a407..80d5aaabfd8c3f 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -768,7 +768,7 @@ if (CMAKE_LINKER MATCHES "link.exe$")
# it, but CMake doesn't seem to have a way to set linker flags for
# individual static libraries, so we enable the suppression flag for
# the whole compiler-rt project.
- set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} /IGNORE:4221")
+ set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} ${CMAKE_CXX_LINKER_WRAPPER_FLAG}/IGNORE:4221")
endif()
add_subdirectory(include)
diff --git a/compiler-rt/lib/asan/CMakeLists.txt b/compiler-rt/lib/asan/CMakeLists.txt
index fb3d74283a61e0..5ec995ae159b73 100644
--- a/compiler-rt/lib/asan/CMakeLists.txt
+++ b/compiler-rt/lib/asan/CMakeLists.txt
@@ -141,7 +141,7 @@ append_list_if(COMPILER_RT_HAS_FTLS_MODEL_INITIAL_EXEC
# LLVM turns /OPT:ICF back on when LLVM_ENABLE_PDBs is set
# we _REALLY_ need to turn it back off for ASAN, because the way
# asan emulates weak functions from DLLs requires NOICF
-append_list_if(MSVC "/DEBUG;/OPT:NOICF" ASAN_DYNAMIC_LINK_FLAGS)
+append_list_if(MSVC "LINKER:/DEBUG;LINKER:/OPT:NOICF" ASAN_DYNAMIC_LINK_FLAGS)
set(ASAN_DYNAMIC_LIBS
${COMPILER_RT_UNWINDER_LINK_LIBS}
|
|
This lgtm, although I don't know if it's actually going to make asan work with the intel toolchain. What linker do they use? |
Do you mean will it make an asan runtime library built with the intel toolchain work, or will it make
MSVC's |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/749 Here is the relevant piece of the build log for the reference |
|
These lines in compiler-rt/CMakeLists.txt:763, Break Windows build of compiler-rt with: CMAKE_STATIC_LINKER_FLAGS are for Static .lib/.a archives given to llvm-ar , which does not accept "-Xlinker", and thus, terminate the build. CMAKE_CXX_LINKER_WRAPPER_FLAG contains -Xlinker which CMake provides from platform defaults. This is used when going through the Compiler for linking, and should not be included in the Linker flags themselves. The correct command is: Another problem is: |
Which version of CMake are you using? What exactly is the C++ compiler? At the time of writing this code |
Actually can you file an issue with a full reproducer? I can take a look at it then. |
I tested with Cmake 3.29.4 and the latest 4.0.2. I also tested with clang and clang-cl as the compiler. Here is the Here’s my latest understanding of the issue: For .exe and .dll builds, CMake lets the Compiler handle the linking. In this case, CMAKE_CXX_LINKER_WRAPPER_FLAG (e.g., -Xlinker) is given to the Compiler, to forward the Linker flags. However for Static libraries (.lib / .a), CMake directly calls llvm-ar or lib.exe, bypassing the Compiler. Therefore, CMAKE_STATIC_LINKER_FLAGS are passed as-is — and should not include CMAKE_CXX_LINKER_WRAPPER_FLAG, which only makes sense when going through the Compiler. That’s why this line is incorrect: I suspect that in your testing, either: CMAKE_CXX_LINKER_WRAPPER_FLAG was empty, or The conditional wrapping the above line didn’t fire (perhaps due to CMAKE_LINKER MATCHES "link.exe$" being false). Thankfully, running ninja on an already built LLVM, will not retrigger a whole new build, and you can test just the compiler-rt CMake quickly. |
Where is
That differs between compiler to compiler. For clang-cl, MSVC and clang you're correct, but for the Intel Compiler icx, linking static libraries also goes through the compiler. That said I'm aware that So this PR I probably broke building compiler-rt with clang (not clang-cl) on Windows, which is not something I was aware of anyone using. I'll see if I can reproduce and fix that. |
So I can reproduce the linker failure, but I'm also getting other failures too, notably this one for example: I don't think compiler-rt ever compiled cleanly when using |
Thank you for your diligence on the matter! At LLVM 18 and 19, I was using able to build clang, clang-tools-extra and compiler-rt with just clang on windows. In 20 and 21, most executables built, but clangd would run and but crash in vs code. Also the Clspv project, which is heavily based on LLVM, the debug builds were fine, but in the release build, clspv.exe would crash right away (during global variable construction). We wish LLVM became more independent of MS tools, and would be able to build itself on windows using just clang and lld-link. But that is not the case in V20+. I have tried to replicate the major internal flags of of clang-cl for clang, but have not succeeded yet. |
I'm not sure I follow exactly, clang-cl shouldn't depend on any more MS tools than clang, it is just a different command line in the end. Anyway if you wish to work on getting llvm buildable with clang instead of clang-cl then I can offer to review patches, this is a topic I had the misfortune of looking at quite a lot :). |
Cmake before 3.31, looks for and tests with ms mt.exe during setup, even when not used for LLVM, and now it finds llvm-mt.exe instead! I also renamed ms link.exe away, and clang-cl seems to be happy without it, using lld-link. So -fuse-ld=lld-link seems unnecessary. We may finally have ms independance building LLVM for windows!. This is great, because we may build it on a linux machine with just the ms includes and the .lib files. At least in theory, and without the ms marketing and lawyers involved. :-) Clang-cl behavior seems to be a lot more than the driver translating flags and setting different defaults. Cmake and its scripts behave differently in this mode, and give different flags (like /D_WINDOWS), that Cmake Clang mode does not. It also seems, Clang internals detect IsCLMode() and behave differently more than just the flags. Replicating all these settings to find a clang match is time consuming, because even with successful builds that take a while, produced exes may crash. It was nice to be able build LLVM upto 19 with Clang only. I like the purity of it, vs all these variant front ends. Relying on clang-cl perpetuates and accumulates archaic complexity going forward. I wish official build tests included LLVM windows with Clang. The problems introduced in 20 and 21 would have been caught early. -DLLVM_HOST_TRIPLE=x86_64-pc-win32 is still working here, but it seems to be one of those abandoned settings, partially ming, partially msvc, which noone uses! I do remember fixing another compiler-rt problem because of it. I will correct it to x86_64-pc-windows-msvc. Thank you for catching it. Cmake is horrors as a language from the days before Ninja existed and Python took off. We use a custom builder in Python and Ninja. In an ideal world, LLVM would migrate to something more modern than Cmake. |
RFC: https://discourse.llvm.org/t/rfc-cmake-linker-flags-need-wl-equivalent-for-intel-c-icx-on-windows/82446
My previous pass missed some flags because I used
-Werror=unknown-argument``, but/D, `/I` and `/O` are accepted by clang (even when only linking), but mean different things than intended for `link.exe. Wrapping the linker with a script revealed these flags.The approach I used to find these is documented as github gist here: https://gist.github.com/Maetveis/00567488f0d6ff74095d91ed306fafc5