This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hans on May 16 2023, 9:22 AM.

Details

Summary

The build uses other mechanism to select the runtime.

Fixes #62719

Diff Detail

Event Timeline

hans created this revision.May 16 2023, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 9:22 AM
Herald added a subscriber: ekilmer. · View Herald Transcript
hans requested review of this revision.May 16 2023, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 9:22 AM
Mordante accepted this revision.May 16 2023, 10:57 AM

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
13

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

This revision is now accepted and ready to land.May 16 2023, 10:57 AM
hans marked an inline comment as done.May 16 2023, 11:22 AM

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 :)

This revision was landed with ongoing or failed builds.May 16 2023, 11:22 AM
This revision was automatically updated to reflect the committed changes.

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.

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 added a comment.May 17 2023, 1:47 AM

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.

  1. 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

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.

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 added a comment.May 17 2023, 7:24 AM

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 added a subscriber: thakis.May 17 2023, 7:54 AM

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

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

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.