This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Clean up old code for handling MSVC runtime setting the old way
ClosedPublic

Authored by mstorsjo on Jul 17 2023, 2:44 AM.

Details

Summary

This was left in place to reduce the risk of breaking anything,
and to keep the diff smaller, in D155233.

Diff Detail

Unit TestsFailed

Event Timeline

mstorsjo created this revision.Jul 17 2023, 2:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 2:44 AM
Herald added subscribers: ekilmer, Enna1. · View Herald Transcript
mstorsjo requested review of this revision.Jul 17 2023, 2:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 17 2023, 2:44 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
smeenai accepted this revision.Jul 18 2023, 10:05 AM

LGTM, very nice.

llvm/cmake/modules/ChooseMSVCCRT.cmake
12–13

Does using generator expressions for this not work, as suggested in https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html?

This revision is now accepted and ready to land.Jul 18 2023, 10:05 AM
mstorsjo added inline comments.Jul 18 2023, 11:49 AM
llvm/cmake/modules/ChooseMSVCCRT.cmake
12–13

I haven't tested myself, but I would believe them to work.

What I'm trying to say here, is that I'm not trying to make a generic function that reads a number of LLVM_USE_CRT_* options and generates one single generator expression for CMAKE_MSVC_RUNTIME_LIBRARY that would express exactly the chosen combination.

I.e. for trivial cases with LLVM_USE_CRT_* this should help you along and you'd get a kind warning asking you to move onwards to CMAKE_MSVC_RUNTIME_LIBRARY at some point. If you have a nontrivial case, you'll to express it with a generator expression directly in CMAKE_MSVC_RUNTIME_LIBRARY right away.

I.e. "isn't supported" refers to the transitionary implementation here, not about what CMAKE_MSVC_RUNTIME_LIBRARY can do.

I guess I could reword it to be clearer.

smeenai added inline comments.Jul 18 2023, 12:45 PM
llvm/cmake/modules/ChooseMSVCCRT.cmake
12–13

Ah, got it, that makes sense.

phosek accepted this revision.Jul 18 2023, 11:43 PM

LGTM

This revision was landed with ongoing or failed builds.Jul 19 2023, 1:25 AM
This revision was automatically updated to reflect the committed changes.
yurybura reopened this revision.Sep 26 2023, 1:27 PM
yurybura added a subscriber: yurybura.

This change breaks build on Windows for the Debug configuration with MSVC.

Due to /D_DEBUG and /MDd are still present on the command line, I see a build error and warning::

FAILED: projects/compiler-rt/lib/asan/CMakeFiles/RTAsan_dynamic.x86_64.dir/asan_win.cpp.obj 
C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\cl.exe   /TP -DASAN_DYNAMIC=1 -DINTERCEPTION_DYNAMIC_CRT -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\buildtrees\llvm\x64-windows-dbg\projects\compiler-rt\lib\asan -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan -ID:\buildtrees\llvm\x64-windows-dbg\include -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\llvm\include -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\.. /nologo /DWIN32 /D_WINDOWS /utf-8   /MP  /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /W4 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1  -std:c++17 -MT /Oy- /GS- /Zc:threadSafeInit- /Z7 /wd4146 /wd4291 /wd4391 /wd4722 /wd4800 /GR- "/experimental:external /external:W0 /external:anglebrackets" /showIncludes /Foprojects\compiler-rt\lib\asan\CMakeFiles\RTAsan_dynamic.x86_64.dir\asan_win.cpp.obj /Fdprojects\compiler-rt\lib\asan\CMakeFiles\RTAsan_dynamic.x86_64.dir\ /FS -c D:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\asan_win.cpp
cl : Command line warning D9025 : overriding '/MDd' with '/MT'
cl : Command line warning D9002 : ignoring unknown option '/experimental:external /external:W0 /external:anglebrackets'
D:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\asan_win.cpp(257): fatal error C1189: #error:  Please build the runtime with a non-debug CRT: /MD or /MT
This revision is now accepted and ready to land.Sep 26 2023, 1:27 PM

This change breaks build on Windows for the Debug configuration with MSVC.

Due to /D_DEBUG and /MDd are still present on the command line, I see a build error and warning::

FAILED: projects/compiler-rt/lib/asan/CMakeFiles/RTAsan_dynamic.x86_64.dir/asan_win.cpp.obj 
C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\cl.exe   /TP -DASAN_DYNAMIC=1 -DINTERCEPTION_DYNAMIC_CRT -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\buildtrees\llvm\x64-windows-dbg\projects\compiler-rt\lib\asan -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan -ID:\buildtrees\llvm\x64-windows-dbg\include -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\llvm\include -ID:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\.. /nologo /DWIN32 /D_WINDOWS /utf-8   /MP  /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /W4 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1  -std:c++17 -MT /Oy- /GS- /Zc:threadSafeInit- /Z7 /wd4146 /wd4291 /wd4391 /wd4722 /wd4800 /GR- "/experimental:external /external:W0 /external:anglebrackets" /showIncludes /Foprojects\compiler-rt\lib\asan\CMakeFiles\RTAsan_dynamic.x86_64.dir\asan_win.cpp.obj /Fdprojects\compiler-rt\lib\asan\CMakeFiles\RTAsan_dynamic.x86_64.dir\ /FS -c D:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\asan_win.cpp
cl : Command line warning D9025 : overriding '/MDd' with '/MT'
cl : Command line warning D9002 : ignoring unknown option '/experimental:external /external:W0 /external:anglebrackets'
D:\buildtrees\llvm\src\org-17.0.1-4a67238d53.clean\compiler-rt\lib\asan\asan_win.cpp(257): fatal error C1189: #error:  Please build the runtime with a non-debug CRT: /MD or /MT

I'm not able to reproduce this, when testing on latest git main and 17.0.1. Are you manually passing the /D_DEBUG /MDd options as cmake options somewhere? Have you done a properly clean reconfigure with cmake? Since the latest cmake policy setup, one is not supposed to pass those options manually anywhere, but just rely on setting CMAKE_MSVC_RUNTIME_LIBRARY, which does get hardcoded to MultiThreaded for the sanitizers.

yurybura added a comment.EditedSep 28 2023, 3:10 AM

I'm not able to reproduce this, when testing on latest git main and 17.0.1. Are you manually passing the /D_DEBUG /MDd options as cmake options somewhere? Have you done a properly clean reconfigure with cmake? Since the latest cmake policy setup, one is not supposed to pass those options manually anywhere, but just rely on setting CMAKE_MSVC_RUNTIME_LIBRARY, which does get hardcoded to MultiThreaded for the sanitizers.

Yes, the /D_DEBUG /MDd flags are defined manually. More precisely, I use VCPKG to build LLVM, and it declares them implicitly for debug configuration. LLVM and Clang themselves build with these flags, but the compiler-rt project fails (it is added as an external CMake project to the Clang build, and without patching the code I don't know how to fix this failure). I think it's better to leave the CMake code to remove unsupported compiler flags. You can find the same code in other project. For example please refer libcxx\CMakeLists.txt :

# FIXME: Remove all debug flags and flags that change which Windows
# default libraries are linked. Currently we only support linking the
# non-debug DLLs
remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md")

I'm not able to reproduce this, when testing on latest git main and 17.0.1. Are you manually passing the /D_DEBUG /MDd options as cmake options somewhere? Have you done a properly clean reconfigure with cmake? Since the latest cmake policy setup, one is not supposed to pass those options manually anywhere, but just rely on setting CMAKE_MSVC_RUNTIME_LIBRARY, which does get hardcoded to MultiThreaded for the sanitizers.

Yes, the /D_DEBUG /MDd flags are defined manually. More precisely, I use VCPKG to build LLVM, and it declares them implicitly for debug configuration.

Right - I'm not sure if we'd like to consider that supported - the right way of choosing CRT for the LLVM build is by using CMAKE_MSVC_RUNTIME_LIBRARY. I guess we could consider readding a couple lines in compiler-rt to strip that out though. But we don't really in general want to support users manually injecting conflicting CRT flags via the cmake flags. What do others think here, @hans @phosek?

and without patching the code I don't know how to fix this failure

Fixing the sanitizers to build correctly in that mode is most probably out of scope here; the fix you can do is stop passing such flags manually. That's the general migration the vcpkg build script should do, in order to handle CRT choice correctly in a world after https://cmake.org/cmake/help/latest/policy/CMP0091.html is in effect.

You can find the same code in other project. For example please refer libcxx\CMakeLists.txt :

# FIXME: Remove all debug flags and flags that change which Windows
# default libraries are linked. Currently we only support linking the
# non-debug DLLs
remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md")

That code is removed now as well; both since we expect to be using CMAKE_MSVC_RUNTIME_LIBRARY as the only mechanism for selecting the CRT, and also because libcxx now does support building with the other CRT configurations. See https://github.com/llvm/llvm-project/commit/e777e44546f903146e7cfcd241a9dd9d7f217865.

yurybura added a comment.EditedSep 28 2023, 4:12 AM

I guess we could consider readding a couple lines in compiler-rt to strip that out though. But we don't really in general want to support users manually injecting conflicting CRT flags via the cmake flags. What do others think here, @hans @phosek?

If we talk about CRT flags, then I can partially agree. By default CMake defines MultiThreaded$<$<CONFIG:Debug>:Debug>DLL (/MDd for Debug configuration). The Clang project can build with default flags without special handling. The compiler-rt project overwrites specified flags with MultiThreaded (/MT for Debug configuration). CMake doesn't provide any support for remove obsolete flags or warnings or other way to inform somehow about using conflicted/unsupported flags. At least for backward compatibility it is better to leave code to remove unsupported flags.

The main reason of the failure is /D_DEBUG. CMake doesn't provide custom variables to handle this... In either case, this definition is checked in the source code and causes the build to fail.

The main reason of the failure is /D_DEBUG. CMake doesn't provide custom variables to handle this... In either case, this definition is checked in the source code and causes the build to fail.

That define should be set implicitly if CMAKE_MSVC_RUNTIME_LIBRARY is set to one of the debug choices, no?

That define should be set implicitly if CMAKE_MSVC_RUNTIME_LIBRARY is set to one of the debug choices, no?

I think, no. At least I cannot find information about this in CMake documentation.

That define should be set implicitly if CMAKE_MSVC_RUNTIME_LIBRARY is set to one of the debug choices, no?

I think, no. At least I cannot find information about this in CMake documentation.

It's not set explicitly by CMake, but if you set CMAKE_MSVC_RUNTIME_LIBRARY to MultiThreadedDebug or MultiThreadedDebugDLL, it passes -MTd or -MDd to the compiler. When the compiler gets either of those, it implicitly sets -D_DEBUG. Example:

$ cat crtdefs.c 
#ifdef _DEBUG
#error _DEBUG
#endif
$ cl -c crtdefs.c -MDd
Microsoft (R) C/C++ Optimizing Compiler Version 19.30.30705 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

crtdefs.c
crtdefs.c(2): fatal error C1189: #error:  _DEBUG

That define should be set implicitly if CMAKE_MSVC_RUNTIME_LIBRARY is set to one of the debug choices, no?

I think, no. At least I cannot find information about this in CMake documentation.

It's not set explicitly by CMake, but if you set CMAKE_MSVC_RUNTIME_LIBRARY to MultiThreadedDebug or MultiThreadedDebugDLL, it passes -MTd or -MDd to the compiler. When the compiler gets either of those, it implicitly sets -D_DEBUG. Example:

$ cat crtdefs.c 
#ifdef _DEBUG
#error _DEBUG
#endif
$ cl -c crtdefs.c -MDd
Microsoft (R) C/C++ Optimizing Compiler Version 19.30.30705 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

crtdefs.c
crtdefs.c(2): fatal error C1189: #error:  _DEBUG

Once again, the default value for CMAKE_MSVC_RUNTIME_LIBRARY is MultiThreaded$<$<CONFIG:Debug>:Debug>DLL for the entire LLVM build, /D_DEBUG is added to CMAKE_CXX_FLAGS_DEBUG. The compiler-rt project overwrites CMAKE_MSVC_RUNTIME_LIBRARY, but does not change other compiler flags that conflict with the modified CRT version.

Once again, the default value for CMAKE_MSVC_RUNTIME_LIBRARY is MultiThreaded$<$<CONFIG:Debug>:Debug>DLL for the entire LLVM build, /D_DEBUG is added to CMAKE_CXX_FLAGS_DEBUG.

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

The compiler-rt project overwrites CMAKE_MSVC_RUNTIME_LIBRARY, but does not change other compiler flags that conflict with the modified CRT version.

The point is that by switching CMAKE_MSVC_RUNTIME_LIBRARY to the right choice, debug/non-debug, the define _DEBUG gets added automatically by the compiler if building in a debug mode. You shouldn't be adding that manually.

Once again, the default value for CMAKE_MSVC_RUNTIME_LIBRARY is MultiThreaded$<$<CONFIG:Debug>:Debug>DLL for the entire LLVM build, /D_DEBUG is added to CMAKE_CXX_FLAGS_DEBUG.

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

I just tested building a CMake project, setting CMAKE_BUILD_TYPE=Debug, not touching CMAKE_MSVC_RUNTIME_LIBRARY. CMake invokes cl.exe with /MDd, /D_DEBUG is never passed explicit to the compiler - this is implied by /MDd.

yurybura added a comment.EditedSep 28 2023, 6:33 AM

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

I'm saying that VCPKG adds these compiler flags. I use this package manager to build LLVM projects, including compiler-rt. Starting from LLVM 17.0.1 I see the build error.

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

I'm saying that VCPKG adds these compiler flags.

And I'm saying that it's redundant.

The point is that by switching CMAKE_MSVC_RUNTIME_LIBRARY to the right choice, debug/non-debug, the define _DEBUG gets added automatically by the compiler if building in a debug mode. You shouldn't be adding that manually.

As I understand _DEBUG is not added by default. How to add it for Debug config for the whole LLVM/Clang build but exclude adding it to compiler-rt or libcxx?

Why do you feel that you need to add _DEBUG at all?

If you set CMAKE_MSVC_RUNTIME_LIBRARY to a debug CRT, this define gets added automatically by the compiler. (And conversely, if something further down the build chain chooses to override CMAKE_MSVC_RUNTIME_LIBRARY, there's no conflicting _DEBUG define either.)

yurybura added a comment.EditedSep 28 2023, 6:45 AM

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

I'm saying that VCPKG adds these compiler flags.

And I'm saying that it's redundant.

Yes, it is. I'm just asking to save removing these flags for backward compatibility.

Are you saying that this gets added automatically by CMake in this variable, or that it is set manually by your build configuration? I don't think it's added automatically by CMake.

I'm saying that VCPKG adds these compiler flags.

And I'm saying that it's redundant.

Yes, it is. I'm just asking to save removing these flags for backward compatibility.

Right.

I'd be ok with restoring the removing of these flags temporarily (for a little while in git main, and backporting that to the 17.x release branch), but going forward, in an environment set up according to https://cmake.org/cmake/help/latest/policy/CMP0091.html, I think the user shouldn't be adding these flags manually.

Unfortunately, while we can keep this cleanup in 17.x, users of such setups won't necessarily get visible deprecation warnings telling them to move forward with their setups though. (Or I guess we could add similar logic scanning the variables and giving a warning if they are set there?)

And I'm saying that it's redundant.

Yes, it is. I'm just asking to save removing these flags for backward compatibility.

Right.

I'd be ok with restoring the removing of these flags temporarily (for a little while in git main, and backporting that to the 17.x release branch), but going forward, in an environment set up according to https://cmake.org/cmake/help/latest/policy/CMP0091.html, I think the user shouldn't be adding these flags manually.

I posted a PR for reinstating this flag removal in https://github.com/llvm/llvm-project/pull/67935.

mstorsjo closed this revision.Oct 2 2023, 3:24 AM

I posted a PR for reinstating this flag removal in https://github.com/llvm/llvm-project/pull/67935.

This was landed now.