[PATCH] MSVC: Allow choosing different CRT for different build types

This patch splits LLVM_USE_CRT into new CMake variables
LLVM_USE_CRT_DEBUG, LLVM_USE_CRT_RELEASE, etc (one for each build
type). It also automatically detects which CRT was already chosen by
CMake's defaults in the CMAKE_CXX_FLAGS_* variables, and defaults to
those values.

(Before, it was using add_llvm_definitions(), which worked...somehow,
but CMAKE_CXX_FLAGS_RELEASE and friends were still defining their own
CRT flag. Now it edits the FLAGS variables instead.)

add-build-config-crt-selection-v4.patch (4.83 KB)

nobled <nobled@dreamwidth.org> writes:

This patch splits LLVM_USE_CRT into new CMake variables
LLVM_USE_CRT_DEBUG, LLVM_USE_CRT_RELEASE, etc (one for each build
type). It also automatically detects which CRT was already chosen by
CMake's defaults in the CMAKE_CXX_FLAGS_* variables, and defaults to
those values.

(Before, it was using add_llvm_definitions(), which worked...somehow,
but CMAKE_CXX_FLAGS_RELEASE and friends were still defining their own
CRT flag. Now it edits the FLAGS variables instead.)

I'm a bit wary about this patch. So much complexity for so petty
feature... Maybe the right thing is to determine the scenarios where
people set LLVM_USE_CRT. Maybe all we need is to define another build
type that inherits from Release which uses the debug version of the CRT.

Anyways, the patch have some issues: remove commented-out code, use the
`foreach' instead of `FOREACH', as in the rest of the code (same for
other keywords), either remove the user-configurable variable
LLVM_USE_CRT or exit with an error if the user sets it, add an extensive
comment about the motivation of the code and about how it achieves its
purpose. Maybe put it on a new file under cmake/modules for stuff
specific of Visual Studio.

But first I'll like to see if we can simplify the requirements.

[snip]

I'm a bit wary about this patch. So much complexity for so petty
feature... Maybe the right thing is to determine the scenarios where
people set LLVM_USE_CRT. Maybe all we need is to define another build
type that inherits from Release which uses the debug version of the CRT.

Anyways, the patch have some issues: remove commented-out code, use the
`foreach' instead of `FOREACH', as in the rest of the code (same for
other keywords), either remove the user-configurable variable
LLVM_USE_CRT or exit with an error if the user sets it, add an extensive
comment about the motivation of the code and about how it achieves its
purpose. Maybe put it on a new file under cmake/modules for stuff
specific of Visual Studio.

Sure; fixed-up version attached.

But first I'll like to see if we can simplify the requirements.

[snip]

I'm not sure it's possible to simplify it too much--we still need the
choice between dynamic and static CRT, too. Like Mesa/Gallium3d: by
default CMake has LLVM compiled with the dynamically linked CRT, but
Gallium needs a static CRT and right now it expects LLVM's Debug build
to use /MTd and Release to use /MT. If we add a bunch of new
build-types like DebugStatic, ReleaseStatic, ReleaseStaticDebug, etc
for all the use cases, it seems like it'd end up just as complicated
as having a separate option for each type.

add-build-config-crt-selection-v8.patch (4.77 KB)

nobled <nobled@dreamwidth.org> writes:

[snip]

Please move the new code to a new file named
cmake/modules/WindowsCRTControl.cmake and include it from the top level
CMakeLists when LLVM_ON_WIN32.

Isn't it only ever used when "if (MSVC)" though, not on WIN32 in general?

(I also refactored some logic with helper macros to try and make it
more readable.)

add-build-config-crt-selection-v9.patch (5.33 KB)

nobled <nobled@dreamwidth.org> writes:

Please move the new code to a new file named
cmake/modules/WindowsCRTControl.cmake and include it from the top level
CMakeLists when LLVM_ON_WIN32.

Isn't it only ever used when "if (MSVC)" though, not on WIN32 in general?

MinGW uses the VC++ CRT, so it could benefit from it.

Applied an slightly modified version on r 110296

Forgot to add the "Patch by nobled!" to the commit message, sorry. As
you don't use a proper name, maybe you don't care. If you want to be
credited, drop me an e-mail and I'll try to fix it.

[snip]