[CMake]Fully delete the deprecated LLVM_USE_CRT* #66850

Naville · 2023-09-20T02:37:38Z

This has been deprecated in favor of CMake's CMAKE_MSVC_RUNTIME_LIBRARY in c6bd873 .
Current release branch still contains it in deprecated status so no existing end-users will be affected

llvmbot · 2023-09-20T02:38:45Z

@llvm/pr-subscribers-clang

Changes

Ref

Notes

ChooseMSVCCRT.cmake is the automatic translation layer that's been completely removed
The old implementation seems to support MultiConfiguration generators that I personally don't use , so some guidance from the reviewer on that topic would help
The CRT allocator code has already been fixed to not use LLVM_USE_CRT

Full diff: https://github.com/llvm/llvm-project/pull/66850.diff

7 Files Affected:

(modified) clang/cmake/caches/CrossWinToARMLinux.cmake (-1)
(modified) clang/cmake/caches/Fuchsia-stage2.cmake (-1)
(modified) clang/cmake/caches/Fuchsia.cmake (-4)
(removed) llvm/cmake/modules/ChooseMSVCCRT.cmake (-58)
(modified) llvm/cmake/modules/HandleLLVMOptions.cmake (+1-2)
(modified) llvm/cmake/modules/LLVMConfig.cmake.in (+1-4)
(modified) llvm/docs/CMake.rst (-6)

diff --git a/clang/cmake/caches/CrossWinToARMLinux.cmake b/clang/cmake/caches/CrossWinToARMLinux.cmake
index e6f5650eac4668f..bbb4b9e71be2d3d 100644
--- a/clang/cmake/caches/CrossWinToARMLinux.cmake
+++ b/clang/cmake/caches/CrossWinToARMLinux.cmake
@@ -95,7 +95,6 @@ set(CLANG_DEFAULT_LINKER                    "lld" CACHE STRING "")
 
 if(WIN32)
   set(CMAKE_MSVC_RUNTIME_LIBRARY            "MultiThreaded" CACHE STRING "")
-  set(LLVM_USE_CRT_RELEASE                  "MT" CACHE STRING "")
 endif()
 
 # Set up RPATH for the target runtime/builtin libraries.
diff --git a/clang/cmake/caches/Fuchsia-stage2.cmake b/clang/cmake/caches/Fuchsia-stage2.cmake
index bc4d9f1462b1814..803d41a2c87179a 100644
--- a/clang/cmake/caches/Fuchsia-stage2.cmake
+++ b/clang/cmake/caches/Fuchsia-stage2.cmake
@@ -30,7 +30,6 @@ set(LLDB_ENABLE_CURSES OFF CACHE BOOL "")
 set(LLDB_ENABLE_LIBEDIT OFF CACHE BOOL "")
 
 if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")
   set(FUCHSIA_DISABLE_DRIVER_BUILD ON)
 endif()
 
diff --git a/clang/cmake/caches/Fuchsia.cmake b/clang/cmake/caches/Fuchsia.cmake
index c599f141f9e5b1b..9c68be0bfe54b69 100644
--- a/clang/cmake/caches/Fuchsia.cmake
+++ b/clang/cmake/caches/Fuchsia.cmake
@@ -70,10 +70,6 @@ foreach(variable ${_FUCHSIA_BOOTSTRAP_PASSTHROUGH})
   endif()
 endforeach()
 
-if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")
-endif()
-
 set(CLANG_DEFAULT_CXX_STDLIB libc++ CACHE STRING "")
 set(CLANG_DEFAULT_LINKER lld CACHE STRING "")
 set(CLANG_DEFAULT_OBJCOPY llvm-objcopy CACHE STRING "")
diff --git a/llvm/cmake/modules/ChooseMSVCCRT.cmake b/llvm/cmake/modules/ChooseMSVCCRT.cmake
deleted file mode 100644
index 6d52ac97bac2092..000000000000000
--- a/llvm/cmake/modules/ChooseMSVCCRT.cmake
+++ /dev/null
@@ -1,58 +0,0 @@
-# The macro choose_msvc_crt() takes a list of possible
-# C runtimes to choose from, in the form of compiler flags,
-# to present to the user. (MTd for /MTd, etc)
-#
-# The macro is invoked at the end of the file.
-#
-# This mechanism is deprecated, but kept for transitioning users.
-#
-# This reads the LLVM_USE_CRT_<CONFIG> options and sets
-# CMAKE_MSVC_RUNTIME_LIBRARY accordingly. The previous mechanism allowed
-# setting different choices for different build configurations (for
-# multi-config generators), but translating multiple differing choices to
-# the corresponding CMAKE_MSVC_RUNTIME_LIBRARY generator expression isn't
-# supported by this transitional helper.
-
-macro(choose_msvc_crt MSVC_CRT)
-  if(LLVM_USE_CRT)
-    message(FATAL_ERROR
-      "LLVM_USE_CRT is deprecated. Use the CMAKE_BUILD_TYPE-specific
-variables (LLVM_USE_CRT_DEBUG, etc) instead.")
-  endif()
-
-  foreach(build_type ${CMAKE_CONFIGURATION_TYPES} ${CMAKE_BUILD_TYPE})
-    string(TOUPPER "${build_type}" build)
-    if (NOT "${LLVM_USE_CRT_${build}}" STREQUAL "")
-      if (NOT ${LLVM_USE_CRT_${build}} IN_LIST ${MSVC_CRT})
-        message(FATAL_ERROR
-          "Invalid value for LLVM_USE_CRT_${build}: ${LLVM_USE_CRT_${build}}. Valid options are one of: ${${MSVC_CRT}}")
-      endif()
-      set(library "MultiThreaded")
-      if ("${LLVM_USE_CRT_${build}}" MATCHES "d$")
-        set(library "${library}Debug")
-      endif()
-      if ("${LLVM_USE_CRT_${build}}" MATCHES "^MD")
-        set(library "${library}DLL")
-      endif()
-      if(${runtime_library_set})
-        message(WARNING "Conflicting LLVM_USE_CRT_* options")
-      else()
-        message(WARNING "The LLVM_USE_CRT_* options are deprecated, use the CMake provided CMAKE_MSVC_RUNTIME_LIBRARY setting instead")
-      endif()
-      set(CMAKE_MSVC_RUNTIME_LIBRARY "${library}" CACHE STRING "" FORCE)
-      message(STATUS "Using VC++ CRT: ${CMAKE_MSVC_RUNTIME_LIBRARY}")
-      set(runtime_library_set 1)
-    endif()
-  endforeach(build_type)
-endmacro(choose_msvc_crt MSVC_CRT)
-
-
-# List of valid CRTs for MSVC
-set(MSVC_CRT
-  MD
-  MDd
-  MT
-  MTd)
-
-choose_msvc_crt(MSVC_CRT)
-
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 4e87c6d1c3eb868..f72c41aaffb5061 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -470,7 +470,6 @@ endif()
 option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
 
 if( MSVC )
-  include(ChooseMSVCCRT)
 
   # Add definitions that make MSVC much less annoying.
   add_compile_definitions(
@@ -951,7 +950,7 @@ if(LLVM_USE_SANITIZER)
       endif()
       # Prepare ASAN runtime if needed
       if (LLVM_USE_SANITIZER MATCHES ".*Address.*")
-        if (${LLVM_USE_CRT_${uppercase_CMAKE_BUILD_TYPE}} MATCHES "^(MT|MTd)$")
+        if (${CMAKE_MSVC_RUNTIME_LIBRARY} MATCHES "^(MultiThreaded|MultiThreadedDebug)$")
           append("/wholearchive:clang_rt.asan-${arch}.lib /wholearchive:clang_rt.asan_cxx-${arch}.lib"
             CMAKE_EXE_LINKER_FLAGS)
           append("/wholearchive:clang_rt.asan_dll_thunk-${arch}.lib"
diff --git a/llvm/cmake/modules/LLVMConfig.cmake.in b/llvm/cmake/modules/LLVMConfig.cmake.in
index 42dfd607f7e651a..5465e981f235545 100644
--- a/llvm/cmake/modules/LLVMConfig.cmake.in
+++ b/llvm/cmake/modules/LLVMConfig.cmake.in
@@ -14,10 +14,7 @@ set(LLVM_PACKAGE_BUGREPORT @PACKAGE_BUGREPORT@)
 
 set(LLVM_BUILD_TYPE @CMAKE_BUILD_TYPE@)
 
-set(LLVM_USE_CRT_DEBUG @LLVM_USE_CRT_DEBUG@)
-set(LLVM_USE_CRT_MINSIZEREL @LLVM_USE_CRT_MINSIZEREL@)
-set(LLVM_USE_CRT_RELEASE @LLVM_USE_CRT_RELEASE@)
-set(LLVM_USE_CRT_RELWITHDEBINFO @LLVM_USE_CRT_RELWITHDEBINFO@)
+set(CMAKE_MSVC_RUNTIME_LIBRARY @CMAKE_MSVC_RUNTIME_LIBRARY@)
 
 set(LLVM_USE_SPLIT_DWARF @LLVM_USE_SPLIT_DWARF@)
 
diff --git a/llvm/docs/CMake.rst b/llvm/docs/CMake.rst
index 67f740447ed7bfb..4d894212fb11d47 100644
--- a/llvm/docs/CMake.rst
+++ b/llvm/docs/CMake.rst
@@ -777,12 +777,6 @@ enabled sub-projects. Nearly all of these variable names begin with
   ``LLVM_USE_SANITIZER`` contains ``Undefined``. This can be used to override
   the default set of UBSan flags.
 
-**LLVM_USE_CRT_{target}**:STRING
-  On Windows, tells which version of the C runtime library (CRT) should be used.
-  For example, -DLLVM_USE_CRT_RELEASE=MT would statically link the CRT into the
-  LLVM tools and library. This is deprecated; use ``CMAKE_MSVC_RUNTIME_LIBRARY``
-  instead.
-
 **LLVM_USE_INTEL_JITEVENTS**:BOOL
   Enable building support for Intel JIT Events API. Defaults to OFF.

mstorsjo

This looks mostly good to me, but a few cases might need setting CMAKE_MSVC_RUNTIME_LIBRARY instead of the removed variable.

The commit message needs improving though. This isn't related to the discussion about crt allocators at all.

Instead the commit message should explain that this setting is deprecated by the new CMake mechanism that we now prefer, mention that this was already marked as deprecated since July (referencing the commit c6bd873), and mention that the current release branch, 17.x, still contains this option in deprecated form (so people upgrading through each major release won't have a silent break, but will get deprecation about using the old option form and have a chance to change it before it is removed).

mstorsjo · 2023-09-20T08:11:44Z

clang/cmake/caches/Fuchsia-stage2.cmake

@@ -30,7 +30,6 @@ set(LLDB_ENABLE_CURSES OFF CACHE BOOL "")
 set(LLDB_ENABLE_LIBEDIT OFF CACHE BOOL "")

 if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")


This case here seems to lack a corresponding setting of CMAKE_MSVC_RUNTIME_LIBRARY - I guess we should add that to make this a proper no-op?

It's been set at Line 57?

Right, I didn't see that one.

mstorsjo · 2023-09-20T08:12:03Z

clang/cmake/caches/Fuchsia.cmake

@@ -70,10 +70,6 @@ foreach(variable ${_FUCHSIA_BOOTSTRAP_PASSTHROUGH})
  endif()
 endforeach()

-if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")


Same here - we should probably set CMAKE_MSVC_RUNTIME_LIBRARY instead of just removing the setting?

Same as above, maybe Line57 is enough?

You mean line 91 here? Ok, that's also present elsewhere here.

mstorsjo · 2023-09-20T08:12:36Z

llvm/cmake/modules/HandleLLVMOptions.cmake

@@ -951,7 +950,7 @@ if(LLVM_USE_SANITIZER)
      endif()
      # Prepare ASAN runtime if needed
      if (LLVM_USE_SANITIZER MATCHES ".*Address.*")
-        if (${LLVM_USE_CRT_${uppercase_CMAKE_BUILD_TYPE}} MATCHES "^(MT|MTd)$")
+        if (${CMAKE_MSVC_RUNTIME_LIBRARY} MATCHES "^(MultiThreaded|MultiThreadedDebug)$")


Oh, I wonder how I had missed this one while doing the deprecation cleanup...

llvm/cmake/modules/LLVMConfig.cmake.in

mstorsjo

Ok, the PR seems fine code-wise then - please update the commit message (PR description) and this should be fine.

mstorsjo · 2023-09-20T08:46:45Z

clang/cmake/caches/Fuchsia-stage2.cmake

@@ -30,7 +30,6 @@ set(LLDB_ENABLE_CURSES OFF CACHE BOOL "")
 set(LLDB_ENABLE_LIBEDIT OFF CACHE BOOL "")

 if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")


Right, I didn't see that one.

mstorsjo · 2023-09-20T08:47:17Z

clang/cmake/caches/Fuchsia.cmake

@@ -70,10 +70,6 @@ foreach(variable ${_FUCHSIA_BOOTSTRAP_PASSTHROUGH})
  endif()
 endforeach()

-if(WIN32)
-  set(LLVM_USE_CRT_RELEASE "MT" CACHE STRING "")


You mean line 91 here? Ok, that's also present elsewhere here.

This has been deprecated in favor of CMake's CMAKE_MSVC_RUNTIME_LIBRARY in c6bd873 . Current release branch still contains it in deprecated status so no existing end-users will be affected

Naville · 2023-09-20T09:26:11Z

Updated the commit message @mstorsjo

mstorsjo

LGTM, thanks. (But let's wait with landing it until e.g. tomorrow, in case someone else has comments on it.)

mstorsjo · 2023-09-21T09:49:56Z

LGTM, thanks. (But let's wait with landing it until e.g. tomorrow, in case someone else has comments on it.)

I think this should be fine to land now. Do you have commit access, or do you want me to push the button?

Naville · 2023-09-21T10:22:41Z

Push the button for me please :D

…RT_RELEASE This was made an error in llvm/llvm-project#66850. Bug: 1485646 Change-Id: Id106d4f9beeea7299befa4d866a75dd3153ba3b5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4887702 Commit-Queue: Amy Huang <[email protected]> Commit-Queue: Arthur Eubanks <[email protected]> Reviewed-by: Amy Huang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1200349}

glandium · 2023-10-06T05:20:42Z

This unveiled an interesting problem in our builds: llvm-config --cxxflags doesn't include the /MT//MD flag, so when building something that links with one of the .lib from LLVM, you can end up with a failure because you're not building with the same flag as LLVM was built with. Which used to be MT and with this change is now MD.

glandium · 2023-10-06T05:24:29Z

Which used to be MT and with this change is now MD.

(because our script was still using LLVM_USE_CRT_RELEASE)

Naville · 2023-10-07T02:07:51Z

I ~~will~~ can take a look at into improving llvm-config if no one else will

The old flags were removed upstream: llvm#66850

llvmbot added cmake Build system in general and CMake in particular clang Clang issues not falling into any other category labels Sep 20, 2023

Naville marked this pull request as ready for review September 20, 2023 07:35

mstorsjo reviewed Sep 20, 2023

View reviewed changes

[CMake]Fully delete the deprecated LLVM_USE_CRT*

80c5e05

This has been deprecated in favor of CMake's CMAKE_MSVC_RUNTIME_LIBRARY in c6bd873 . Current release branch still contains it in deprecated status so no existing end-users will be affected

Naville changed the title [CMake]Remove LLVM_USE_CRT* [CMake]Fully delete the deprecated LLVM_USE_CRT* Sep 20, 2023

mstorsjo approved these changes Sep 20, 2023

View reviewed changes

mstorsjo merged commit 618e5d2 into llvm:main Sep 21, 2023

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

This was referenced Oct 18, 2023

Clangd weekly builds are broken on windows clangd/clangd#1793

Closed

Fix runtime linkage on windows clangd/clangd#1796

Merged

Boris-Rasin mentioned this pull request Mar 9, 2024

Windows/CMake: Setting LLVM_USE_CRT_DEBUG to MTd is being ignored. #63979

Open

akoeplinger added a commit to dotnet/llvm-project that referenced this pull request Jul 17, 2024

Fix building with debug msvc runtime library on Windows

40ce2f0

The old flags were removed upstream: llvm#66850

akoeplinger mentioned this pull request Jul 17, 2024

Fix building with debug msvc runtime library on Windows dotnet/llvm-project#597

Merged

radekdoulik pushed a commit to dotnet/llvm-project that referenced this pull request Jul 17, 2024

Fix building with debug msvc runtime library on Windows (#597)

1b6b23e

The old flags were removed upstream: llvm#66850

radekdoulik pushed a commit to dotnet/llvm-project that referenced this pull request Aug 16, 2024

Fix building with debug msvc runtime library on Windows (#597)

4f01da9

The old flags were removed upstream: llvm#66850

radekdoulik pushed a commit to dotnet/llvm-project that referenced this pull request Sep 2, 2024

Fix building with debug msvc runtime library on Windows (#597)

43b5c1f

The old flags were removed upstream: llvm#66850

radekdoulik pushed a commit to dotnet/llvm-project that referenced this pull request Sep 12, 2024

Fix building with debug msvc runtime library on Windows (#597)

ab0530e

The old flags were removed upstream: llvm#66850

aneshlya mentioned this pull request Sep 13, 2024

Fix xe-enabled build with LLVM 18 ispc/ispc#3003

Merged

radekdoulik pushed a commit to dotnet/llvm-project that referenced this pull request Sep 19, 2024

Fix building with debug msvc runtime library on Windows (#597)

3f1a2fd

The old flags were removed upstream: llvm#66850

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMake]Fully delete the deprecated LLVM_USE_CRT* #66850

[CMake]Fully delete the deprecated LLVM_USE_CRT* #66850

Naville commented Sep 20, 2023 •

edited

Loading

llvmbot commented Sep 20, 2023

Notes

mstorsjo left a comment

mstorsjo Sep 20, 2023

Naville Sep 20, 2023

mstorsjo Sep 20, 2023

mstorsjo Sep 20, 2023

Naville Sep 20, 2023

mstorsjo Sep 20, 2023

mstorsjo Sep 20, 2023

mstorsjo left a comment

mstorsjo Sep 20, 2023

mstorsjo Sep 20, 2023

Naville commented Sep 20, 2023

mstorsjo left a comment

mstorsjo commented Sep 21, 2023

Naville commented Sep 21, 2023 •

edited

Loading

glandium commented Oct 6, 2023

glandium commented Oct 6, 2023

Naville commented Oct 7, 2023 •

edited

Loading

[CMake]Fully delete the deprecated LLVM_USE_CRT* #66850

[CMake]Fully delete the deprecated LLVM_USE_CRT* #66850

Conversation

Naville commented Sep 20, 2023 • edited Loading

llvmbot commented Sep 20, 2023

Notes

mstorsjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstorsjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naville commented Sep 20, 2023

mstorsjo left a comment

Choose a reason for hiding this comment

mstorsjo commented Sep 21, 2023

Naville commented Sep 21, 2023 • edited Loading

glandium commented Oct 6, 2023

glandium commented Oct 6, 2023

Naville commented Oct 7, 2023 • edited Loading

Naville commented Sep 20, 2023 •

edited

Loading

Naville commented Sep 21, 2023 •

edited

Loading

Naville commented Oct 7, 2023 •

edited

Loading