-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bumping cmake_minimum_required
causes compiler-rt test link failures on Windows
#62719
Comments
Starting with CMake 3.15 it offers CMAKE_MSVC_RUNTIME_LIBRARY so probably that should be used instead of the current work-around. Maybe a quick fix would be to disable the CMP0091 which probably works with the current work-around. |
I'm not familiar with this new option, but you can check out ChooseMSVCRT.cmake for some past history of how we've tried to handle CRT selection. However, compiler-rt is compiled very differently, so it may not be relevant anymore. |
Sent out https://reviews.llvm.org/D150688 to set the CMP0091 policy. We hit another issue in Chromium where building the toolchain with rpmalloc failed (https://bugs.chromium.org/p/chromium/issues/detail?id=1445671). I've verified that this patch fixes that too. |
The compiler-rt label applies since it's compiler-rt's tests that fail to link. Also, it seems there are still breakages (https://bugs.chromium.org/p/chromium/issues/detail?id=1445671#c7). I'll try to get a repro for that. |
|
I tried
but that didn't help. Also the below didn't help:
|
@zmodem CMakePolicy.cmake needs to be included before project(). You need to move the set() and include() right below the cmake_minimum_required. |
Thanks @glandium, that seems to work. With the patch below on top of 7d47dac, I was able to build successfully.
|
…ehaviour With the new behaviour, the /MD or similar options aren't added to e.g. CMAKE_CXX_FLAGS_RELEASE, but are added separately by CMake. They can be changed by the cmake variable CMAKE_MSVC_RUNTIME_LIBRARY or with the target property MSVC_RUNTIME_LIBRARY. LLVM has had its own custom CMake flags, e.g. LLVM_USE_CRT_RELEASE, which affects which CRT is used for release mode builds. Deprecate these and direct users to use CMAKE_MSVC_RUNTIME_LIBRARY directly instead (and do a best effort attempt at setting CMAKE_MSVC_RUNTIME_LIBRARY based on the existing LLVM_USE_CRT_ flags). This only handles the simple cases, it doesn't handle multi-config generators with different LLVM_USE_CRT_* variables for different configs though, but that's probably fine - we should move over to the new upstream CMake mechanism anyway, and push users towards that. Change code in compiler-rt, that previously tried to override the CRT choice to /MT, to set CMAKE_MSVC_RUNTIME_LIBRARY instead of meddling in the old variables. This resolves the policy issue in #63286, and should handle the issues that were observed originally when the minimum CMake version was bumped, in #62719 and #62739. Differential Revision: https://reviews.llvm.org/D155233
…ersion to 3.20.0. This reverts commit d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6. Adds the patch by @hans from llvm/llvm-project#62719 This patch fixes the Windows build. d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 reverted the reviews D144509 [CMake] Bumps minimum version to 3.20.0. This partly undoes D137724. This change has been discussed on discourse https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-cmake-version/66193 Note this does not remove work-arounds for older CMake versions, that will be done in followup patches. D150532 [OpenMP] Compile assembly files as ASM, not C Since CMake 3.20, CMake explicitly passes "-x c" (or equivalent) when compiling a file which has been set as having the language C. This behaviour change only takes place if "cmake_minimum_required" is set to 3.20 or newer, or if the policy CMP0119 is set to new. Attempting to compile assembly files with "-x c" fails, however this is workarounded in many cases, as OpenMP overrides this with "-x assembler-with-cpp", however this is only added for non-Windows targets. Thus, after increasing cmake_minimum_required to 3.20, this breaks compiling the GNU assembly for Windows targets; the GNU assembly is used for ARM and AArch64 Windows targets when building with Clang. This patch unbreaks that. D150688 [cmake] Set CMP0091 to fix Windows builds after the cmake_minimum_required bump The build uses other mechanism to select the runtime. Fixes #62719 Reviewed By: #libc, Mordante Differential Revision: https://reviews.llvm.org/D151344 GitOrigin-RevId: 53ecfc3277a34dd2809dd76d4c07854de7ba8928 Original-Revision: 528483e0e18ef69085956fa350ca1e0305d5ab75 Roller-URL: https://ci.chromium.org/b/8779938680740208097 CQ-Do-Not-Cancel-Tryjobs: true Change-Id: Icbc27c7586140da89afed38f641ae850a89a34da Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/862996
This reverts commit d763c6e. Adds the patch by @hans from llvm/llvm-project#62719 This patch fixes the Windows build. d763c6e reverted the reviews D144509 [CMake] Bumps minimum version to 3.20.0. This partly undoes D137724. This change has been discussed on discourse https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-cmake-version/66193 Note this does not remove work-arounds for older CMake versions, that will be done in followup patches. D150532 [OpenMP] Compile assembly files as ASM, not C Since CMake 3.20, CMake explicitly passes "-x c" (or equivalent) when compiling a file which has been set as having the language C. This behaviour change only takes place if "cmake_minimum_required" is set to 3.20 or newer, or if the policy CMP0119 is set to new. Attempting to compile assembly files with "-x c" fails, however this is workarounded in many cases, as OpenMP overrides this with "-x assembler-with-cpp", however this is only added for non-Windows targets. Thus, after increasing cmake_minimum_required to 3.20, this breaks compiling the GNU assembly for Windows targets; the GNU assembly is used for ARM and AArch64 Windows targets when building with Clang. This patch unbreaks that. D150688 [cmake] Set CMP0091 to fix Windows builds after the cmake_minimum_required bump The build uses other mechanism to select the runtime. Fixes #62719 Reviewed By: #libc, Mordante Differential Revision: https://reviews.llvm.org/D151344
…ehaviour With the new behaviour, the /MD or similar options aren't added to e.g. CMAKE_CXX_FLAGS_RELEASE, but are added separately by CMake. They can be changed by the cmake variable CMAKE_MSVC_RUNTIME_LIBRARY or with the target property MSVC_RUNTIME_LIBRARY. LLVM has had its own custom CMake flags, e.g. LLVM_USE_CRT_RELEASE, which affects which CRT is used for release mode builds. Deprecate these and direct users to use CMAKE_MSVC_RUNTIME_LIBRARY directly instead (and do a best effort attempt at setting CMAKE_MSVC_RUNTIME_LIBRARY based on the existing LLVM_USE_CRT_ flags). This only handles the simple cases, it doesn't handle multi-config generators with different LLVM_USE_CRT_* variables for different configs though, but that's probably fine - we should move over to the new upstream CMake mechanism anyway, and push users towards that. Change code in compiler-rt, that previously tried to override the CRT choice to /MT, to set CMAKE_MSVC_RUNTIME_LIBRARY instead of meddling in the old variables. This resolves the policy issue in llvm/llvm-project#63286, and should handle the issues that were observed originally when the minimum CMake version was bumped, in llvm/llvm-project#62719 and llvm/llvm-project#62739. Differential Revision: https://reviews.llvm.org/D155233
Surprisingly, it seems the
cmake_minimum_required
bump from 3.13.4 to 3.20.0 in @mordante's ffb807a is causing some behavior change that makes compiler-rt tests fail to link on Windows.From the top of https://lab.llvm.org/buildbot/#/builders/127/builds/48120/steps/8/logs/stdio
I can also reproduce this locally. My CMake version is 3.20.21032501-MSVC_2
I thought LLVM builds defauled to
/MT
(and compiler-rt's cmake files does some flag massaging for it here: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398), but for some reason ffb807a changes that?If I add
-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
, it builds. Maybe that should be added as a default somewhere?The text was updated successfully, but these errors were encountered: