[cmake] Set CMP0091 to fix Windows builds after the cmake_minimum_required bump

[hans]

Summary

The build uses other mechanism to select the runtime.

Fixes #62719

[Mordante]

Thanks for the patch! When this fixes the CI I'm happy with it.
I expect with CMake 3.15 the selection could be done cleaner, but that can be done in a separate patch by the compiler-rt maintainers.

cmake/Modules/CMakePolicy.cmake

18

Maybe put this on top so the policies are ordered by the CMake version introducing them.

[hans]

I expect with CMake 3.15 the selection could be done cleaner, but that can be done in a separate patch by the compiler-rt maintainers.

I think it's not just compiler-rt, but all of llvm that's affected. Agreed, this seems like a larger project that should be done separately by a cmake enthusiast :)

[mstorsjo]

Out of curiosity - what are the symptoms that this change fixes, and which build configurations does it affect? (I would expect that regular builds with just llvm+clang would pass at least?) But I do agree that the new behaviour of CMP0091 probably does conflict with ChooseMSVCCRT.cmake.

Tangentially; for libc++ builds, ChooseMSVCCRT.cmake isn't ever included and used so far. So for the sake of libc++ at the moment, I'd go with keeping CMP0091 set to the new behaviour - which probably is the more convenient way of interacting with the choice of CRT going forward.

I do see that CMakePolicy.cmake doesn't get included in libc++ builds either, so I think that should all be fine.

[glandium]

CMakePolicy.cmake is also not included in compiler-rt/CMakeLists.txt or runtimes/CMakeLists.txt, so this doesn't help with the issue when building compiler-rt standalone.

[hans]

In D150688#4347610, @mstorsjo wrote:

Out of curiosity - what are the symptoms that this change fixes, and which build configurations does it affect? (I would expect that regular builds with just llvm+clang would pass at least?) But I do agree that the new behaviour of CMP0091 probably does conflict with ChooseMSVCCRT.cmake.

I've seen two issues so far, but there could also be others.

1. Some compiler-rt tests failed to link: (from https://github.com/llvm/llvm-project/issues/62719)

> cmake -GNinja -DLLVM_ENABLE_PROJECTS="llvm;clang;compiler-rt" -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_TARGETS_TO_BUILD=X86 ..\llvm && ninja check-fuzzer
[...]
[2985/2987] Generating Fuzzer-x86_64-Test.exe
FAILED: projects/compiler-rt/lib/fuzzer/tests/Fuzzer-x86_64-Test.exe
cmd.exe /C "cd /D C:\src\llvm-project\build.nana\projects\compiler-rt\lib\fuzzer\tests && C:\src\llvm-project\build.nana\.\bin\clang++.exe FuzzerTestObjects.FuzzerUnittest.cpp.x86_64.o FuzzerTestObjects.gtest-all.cc.x86_64.o C:/src/llvm-project/build.nana/projects/compiler-rt/lib/fuzzer/tests/RTFuzzerTest.x86_64.lib -o C:/src/llvm-project/build.nana/projects/compiler-rt/lib/fuzzer/tests/./Fuzzer-x86_64-Test.exe --driver-mode=g++ -Wl,-defaultlib:libcmt,-defaultlib:oldnames"
RTFuzzerTest.x86_64.lib(FuzzerIOWindows.cpp.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in FuzzerTestObjects.FuzzerUnittest.cpp.x86_64.o

It seems the test object ended up with /MT but the library got /MD for some reason.

2. Builting with a custom allocator, -DLLVM_INTEGRATED_CRT_ALLOC (from https://bugs.chromium.org/p/chromium/issues/detail?id=1445671):

lld-link: error: duplicate symbol: calloc
>>> defined at LLVMSupport.lib(rpmalloc.c.obj)
>>> defined at api-ms-win-crt-heap-l1-1-0.dll

It seems we're still seeing some crt related link errors (https://bugs.chromium.org/p/chromium/issues/detail?id=1445671#c7) so maybe this wasn't enough.

Tangentially; for libc++ builds, ChooseMSVCCRT.cmake isn't ever included and used so far. So for the sake of libc++ at the moment, I'd go with keeping CMP0091 set to the new behaviour - which probably is the more convenient way of interacting with the choice of CRT going forward.

I do see that CMakePolicy.cmake doesn't get included in libc++ builds either, so I think that should all be fine.

Does libc++ do anything else to choose crt? compiler-rt has seems to have its own thing in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398

[mstorsjo]

In D150688#4349022, @hans wrote:

Tangentially; for libc++ builds, ChooseMSVCCRT.cmake isn't ever included and used so far. So for the sake of libc++ at the moment, I'd go with keeping CMP0091 set to the new behaviour - which probably is the more convenient way of interacting with the choice of CRT going forward.

I do see that CMakePolicy.cmake doesn't get included in libc++ builds either, so I think that should all be fine.

Does libc++ do anything else to choose crt?

Actually, I was mistaken, ChooseMSVCCRT.cmake does get included there too. However only the default of /MD is supported anyway, at the moment.

compiler-rt has seems to have its own thing in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398

Oh, I see. This seems to look for the literal form /MD, but after the CMake update, CMake seems to set the option in the form -MD, so if this pattern is updated to check for both / and -, this might start working as expected.

[mstorsjo]

In D150688#4349390, @mstorsjo wrote:

In D150688#4349022, @hans wrote:

compiler-rt has seems to have its own thing in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398

Oh, I see. This seems to look for the literal form /MD, but after the CMake update, CMake seems to set the option in the form -MD, so if this pattern is updated to check for both / and -, this might start working as expected.

Although, the new policy does place the flag in a different separate cmake variable, so it’s quite possible that it isn’t enough. In any case, with the new policy, the right thing to do is to set the variable (or target property) for deciding what CRT to use, not to manually exchange flags in the variables.

[hans]

In D150688#4349699, @mstorsjo wrote:

In D150688#4349390, @mstorsjo wrote:

In D150688#4349022, @hans wrote:

compiler-rt has seems to have its own thing in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398

Oh, I see. This seems to look for the literal form /MD, but after the CMake update, CMake seems to set the option in the form -MD, so if this pattern is updated to check for both / and -, this might start working as expected.

Although, the new policy does place the flag in a different separate cmake variable, so it’s quite possible that it isn’t enough. In any case, with the new policy, the right thing to do is to set the variable (or target property) for deciding what CRT to use, not to manually exchange flags in the variables.

Yes, I tried changing those regexes but it didn't help (https://github.com/llvm/llvm-project/issues/62719#issuecomment-1551493063).

[thakis]

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

[thakis]

…that comment was for D144509, but this was one of the follow-ups reverted in that rev.

[glandium]

Since you're mentioning /MD, I spotted this: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/orc/CMakeLists.txt#L134, which ends up *not* adding /MD on the command line because of the quotes (ORC_CFLAGS containing -Isomething, this ends up adding an invalid directory that ends with /MD to the include list). This is yet another thing that should be done through setting the runtime properly.

cmake/Modules/CMakePolicy.cmake

 # CMake policy settings shared between LLVM projects
 
+# CMP0091: MSVC runtime library flags are selected by an abstraction.
+# New in CMake 3.15. https://cmake.org/cmake/help/latest/policy/CMP0091.html
+if(POLICY CMP0091)
+  cmake_policy(SET CMP0091 OLD)
+endif()
 # CMP0114: ExternalProject step targets fully adopt their steps.
 # New in CMake 3.19: https://cmake.org/cmake/help/latest/policy/CMP0114.html
 if(POLICY CMP0114)
   cmake_policy(SET CMP0114 OLD)
 endif()
 # CMP0116: Ninja generators transform `DEPFILE`s from `add_custom_command()`
 # New in CMake 3.20. https://cmake.org/cmake/help/latest/policy/CMP0116.html
 if(POLICY CMP0116)
   cmake_policy(SET CMP0116 OLD)
 endif()