[scripts] Remove a redundant manual /D_DEBUG from CMAKE_<LANG>_FLAGS_DEBUG

[mstorsjo]

We're already setting ${VCPKG_CRT_LINK_FLAG_PREFIX}d here, which expands to either /MDd or /MTd. When the compiler is given either of these options, it implicitly sets the _DEBUG define. Therefore, explicitly passing /D_DEBUG in these configs is redundant.

Explicitly setting /D_DEBUG among these flags can be problematic; CMake projects that target CMake 3.15 or newer, with the policy CMP0091 set to NEW, may control the CRT to use via the variable CMAKE_MSVC_RUNTIME_LIBRARY. If this variable is overridden by the projects, setting it to a non-debug runtime library, we'd still have the explicitly set /D_DEBUG define left, which can cause conflicts.

For such projects that do use the new policy behaviour of CMP0091, explicitly setting the CRT selection options in CMAKE_<LANG>_FLAGS_<CONFIG> isn't ideal - but as long as we don't know that all projects use the new policy behaviour, we might need to keep setting the CRT selection options (as long as we feel the need to set the CMAKE_<LANG>_FLAGS_<CONFIG> variables at all).

For the cases where such projects override CMAKE_MSVC_RUNTIME_LIBRARY internally, it would only cause a mostly benign warning like cl : Command line warning D9025 : overriding '/MDd' with '/MT'.

[mstorsjo]

Some additional references; https://learn.microsoft.com/en-us/cpp/c-runtime-library/debug?view=msvc-170 explicitly says:

The compiler defines _DEBUG when you specify the /MTd or /MDd option.

Therefore, /D_DEBUG /MTd or /D_DEBUG /MDd is redundant.

This redundant define has passed through many layers of changes and can be traced back to being added initially in eba9f6a.

[Neumann-A]

But it is also totally unnecessary to remove it.

If this variable is overridden by the projects, setting it to a non-debug runtime library, we'd still have the explicitly set /D_DEBUG define left, which can cause conflicts.

That would leave vcpkg in an invalid state either way.

[mstorsjo]

But it is also totally unnecessary to remove it.

Currently, debug builds of llvm-project via vcpkg are allegedly broken, as reported by a user.

This, because llvm-project switched to cmake_minimum_required(VERSION 3.20), with the policy CMP0091 set to NEW. One subtarget within llvm-project, within compiler-rt, specifically wants to link with the static, non-debug runtime, so it does set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded).

But as vcpkg has injected /D_DEBUG /MDd into CMAKE_CXX_FLAGS_DEBUG, it ends up compiling with /D_DEBUG /MDd /MT, which effectively is /MT but with /D_DEBUG set, causing conflicts later. As /D_DEBUG is implicit in /MDd it is rather redundant, and by omitting it, /MT can safely override /MDd without the extra /D_DEBUG.

If this variable is overridden by the projects, setting it to a non-debug runtime library, we'd still have the explicitly set /D_DEBUG define left, which can cause conflicts.

That would leave vcpkg in an invalid state either way.

Can you elaborate on what you mean here - what is in an invalid state? The regular executables that are built within llvm-project do honor the CRT that was chosen by the user of the build (vcpkg). The project internal CRT override only affects one of LLVM's runtime libraries that may be used by code compiled by LLVM.

[Neumann-A]

within compiler-rt, specifically wants to link with the static, non-debug runtime, so it does set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded)

Just patch that line out. I assume compiler-rt only cares about Release builds.

[yurybura]

Related to #33600 and #34117

[mstorsjo]

Related to #33600 and #34117

FWIW; #34117 probably doesn't work as such. Even if vcpkg itself might require CMake 3.15, the individual projects that are built might be targeting an older CMake version (or with a project targeting a newer version, it could still set the policy CMP0091 to OLD). So those projects would expect getting the CRT choice options (/MD etc) in CMAKE_<LANG>_FLAGS_<CONFIG>. If vcpkg wouldn't be setting those variables at all, there wouldn't be any issue, but as long as they are set, they shouldn't be set without the /M[DT] options, for such projects.

However, whenever passing a debug CRT flag like /MDd or /MTd, passing /D_DEBUG is entirely redundant, and thus removing that should be entirely safe - which this PR does.

[BillyONeal]

Explicitly setting /D_DEBUG among these flags can be problematic; CMake projects that target CMake 3.15 or newer, with the policy CMP0091 set to NEW, may control the CRT to use via the variable CMAKE_MSVC_RUNTIME_LIBRARY. If this variable is overridden by the projects, setting it to a non-debug runtime library, we'd still have the explicitly set /D_DEBUG define left, which can cause conflicts.

Libraries distributed through vcpkg don't get to control this setting; this is controlled by the triplet via VCPKG_CRT_LINKAGE

For such projects that do use the new policy behaviour of CMP0091, explicitly setting the CRT selection options in CMAKE_<LANG>_FLAGS_<CONFIG> isn't ideal - but as long as we don't know that all projects use the new policy behaviour, we might need to keep setting the CRT selection options (as long as we feel the need to set the CMAKE_<LANG>_FLAGS_<CONFIG> variables at all).

We might also set CMAKE_MSVC_RUNTIME_LIBRARY; discussion for that is over in #34117

@data-queue @vicroms @ras0219-msft @JavierMatosD and I discussed this today.

I made the following pro/con list:

• + Might fix _DEBUG stepping on toes
• + Might be 'shiner' to not repeat things
• - Invalidates all ABI hashes and forces the world to rebuild

@JavierMatosD pointed out that we don't have a concrete problem this fixes (that is, my first bullet is theoretical) which argues to not take this.

@ras0219-msft adds an additional con that this delays potential detection of the failure of mismatched runtimes. If a project is changing this setting in a way incompatible with VCPKG_CRT_LINKAGE, the result is broken because the resulting binaries won't be link compatible with anything else.

We're going to close this for now. Please feel free to reopen the PR or submit a new PR if you have a real case this fixes. Thanks for your contribution to vcpkg and bringing this to our attention!

[mstorsjo]

Explicitly setting /D_DEBUG among these flags can be problematic; CMake projects that target CMake 3.15 or newer, with the policy CMP0091 set to NEW, may control the CRT to use via the variable CMAKE_MSVC_RUNTIME_LIBRARY. If this variable is overridden by the projects, setting it to a non-debug runtime library, we'd still have the explicitly set /D_DEBUG define left, which can cause conflicts.

Libraries distributed through vcpkg don't get to control this setting; this is controlled by the triplet via VCPKG_CRT_LINKAGE

Indeed, I understand that libraries in general don't get the freedom to do this. In this case, the libraries are upstream LLVM compiler-rt sanitizers; the sanitizers do more than enough low level CRT poking anyway, and their compatibility with other build configurations isn't up to whether vcpkg allows them to override this or not - the sanitizers, which are quite special libraries in all regards, override this anyway.

For such projects that do use the new policy behaviour of CMP0091, explicitly setting the CRT selection options in CMAKE_<LANG>_FLAGS_<CONFIG> isn't ideal - but as long as we don't know that all projects use the new policy behaviour, we might need to keep setting the CRT selection options (as long as we feel the need to set the CMAKE_<LANG>_FLAGS_<CONFIG> variables at all).

We might also set CMAKE_MSVC_RUNTIME_LIBRARY

Actually, afaik vcpkg sets both CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_<LANG>_FLAGS_<CONFIG>, in order to convey the intent to projects that use both forms of this setting.

discussion for that is over in #34117

FWIW, that PR is an attempt to fix the same thing as this one does, but it assumes that every CMake project that vcpkg builds itself targets CMake 3.15 or newer, and there's certainly plenty of projects that still target a lower version of CMake, and thus CMAKE_MSVC_RUNTIME_LIBRARY isn't enough for the ones that target older versions.

I made the following pro/con list:

• • Might fix _DEBUG stepping on toes
• • Might be 'shiner' to not repeat things
• • Invalidates all ABI hashes and forces the world to rebuild

Thanks, I wasn't aware of the implication of that last point.

@JavierMatosD pointed out that we don't have a concrete problem this fixes (that is, my first bullet is theoretical) which argues to not take this.

There is a real problem; @yurybura reported that LLVM 17.0.1 failed to build in vcpkg, when the vcpkg CRT choice had been switched to a debug CRT. (I'm not aware if there's a specific vcpkg issue filed for this detail, afaik this is part of a larger undertaking to update vcpkg to package LLVM 17.x.)

I have temporarily worked around this on the LLVM side in llvm/llvm-project@7bc09a4 - by adding extra cleanup of CMAKE_<LANG>_FLAGS_<CONFIG>. As LLVM solely uses CMAKE_MSVC_RUNTIME_LIBRARY, this handling is in principle redundant, so I would like this to be a temporary measure.

@ras0219-msft adds an additional con that this delays potential detection of the failure of mismatched runtimes. If a project is changing this setting in a way incompatible with VCPKG_CRT_LINKAGE, the result is broken because the resulting binaries won't be link compatible with anything else.

I feel this is a rather weak argument. There's plenty of other ways that one can set up a mismatch (e.g. switching between /MD and /MT) where one doesn't get a similarly loud error; thus keeping the inconvenience of /D_DEBUG doesn't give that much extra value IMO.

We're going to close this for now. Please feel free to reopen the PR or submit a new PR if you have a real case this fixes. Thanks for your contribution to vcpkg and bringing this to our attention!

There is a real issue: In LLVM, I would like to remove the temporary workaround from llvm/llvm-project@7bc09a4 which I have reinstated for the sake of vcpkg. But that's not possible as long as vcpkg forcibly injects /D_DEBUG into CMAKE_<LANG>_FLAGS_<CONFIG>.

[Neumann-A]

Really for me it looks like these projects should be disabled for debug config?

[BillyONeal]

There is a real problem; @yurybura reported that LLVM 17.0.1 failed to build in vcpkg, when the vcpkg CRT choice had been switched to a debug CRT. (I'm not aware if there's a specific vcpkg issue filed for this detail, afaik this is part of a larger undertaking to update vcpkg to package LLVM 17.x.)

I think the specifics of what happened here need to be understood.

I have temporarily worked around this on the LLVM side in llvm/llvm-project@7bc09a4 - by adding extra cleanup of CMAKE_FLAGS. As LLVM solely uses CMAKE_MSVC_RUNTIME_LIBRARY, this handling is in principle redundant, so I would like this to be a temporary measure.

It sounds then like LLVM is trying to pick a CRT that does not match VCPKG_CRT_LINKAGE, which is doomed.

[mstorsjo]

There is a real problem; @yurybura reported that LLVM 17.0.1 failed to build in vcpkg, when the vcpkg CRT choice had been switched to a debug CRT. (I'm not aware if there's a specific vcpkg issue filed for this detail, afaik this is part of a larger undertaking to update vcpkg to package LLVM 17.x.)

I think the specifics of what happened here need to be understood.

AFAIK the issue is what I've tried to summarize above; a debug build of LLVM 17.0.1 via vcpkg fails, as it forcibly injects /D_DEBUG into CMAKE_<LANG>_FLAGS_<CONFIG>. Minor parts of LLVM override the CRT choice via CMAKE_MSVC_RUNTIME_LIBRARY; the conflicting /MDd gets overridden by /MT nicely, while the conflicting /D_DEBUG remains.

If necessary, I think @yurybura can fill in with more details.

Note that this issue no longer should be reproducible on 17.0.2, as I reinstated a workaround, in llvm/llvm-project@7bc09a4. But I wouldn't want that workaround to be permanent.

I have temporarily worked around this on the LLVM side in llvm/llvm-project@7bc09a4 - by adding extra cleanup of CMAKE__FLAGS_. As LLVM solely uses CMAKE_MSVC_RUNTIME_LIBRARY, this handling is in principle redundant, so I would like this to be a temporary measure.

It sounds then like LLVM is trying to pick a CRT that does not match VCPKG_CRT_LINKAGE, which is doomed.

In general I would agree, but for this specific case - not really. And I guess this is the root issue to understand, in order to not dismiss the case as LLVM doing something wrong.

I do understand that mixing/matching CRT choices among vcpkg libraries, in general, is doomed and not supported.

However these are not regular libraries that are built as part of the vcpkg build, that other parts of vcpkg can link against and interact with directly. If you select a debug CRT via VCPKG_CRT_LINKAGE, then the LLVM tools, the Clang compiler etc, all of that do honor that choice. The thing that overrides the CRT choice is the sanitizer runtime libraries - and those aren't regular libraries that any other vcpkg library would link directly against as such.

Let's explain the full scenario:

Someone uses vcpkg to install Clang/LLVM. This Clang executable itself is built with the CRT choice set via vcpkg. Other vcpkg tools that use the LLVM libraries get those built with the same matching CRT choice. The user uses this Clang compiler to build their own private code, entirely outside of vcpkg's build system. They can use Clang to build their own code with any CRT choice, /MT, /MDd etc - irrespective of what CRT choice was used to build the Clang executable itself. Do we agree so far?

Now the sanitizer runtime libraries can come into play at this point. The other vcpkg packages/libraries won't interact with them as such. When the user uses Clang to build their own code, outside of vcpkg, and enables e.g. -fsanitize=address, the sanitizer runtime libraries are taken into use. And the CRT choice of the Clang executable itself is entirely irrelevant at this point - the only thing that matters for the sanitizers, is how the end user chooses to compile their own code. And in the paragraph above, we concluded that the vcpkg-built Clang can build user code with any CRT choice, irrespective of how the Clang binary (and the rest of vcpkg) was built.

In order to produce these sanitizer libraries, that will work with all of the CRT choices that the end user might make, those libraries are built and linked carefully in a very specific way. And as part of that process, for building the set of sanitizer libraries that can work in all those conditions, they currently require that they can control exactly the CRT choice used for building them.

[yurybura]

@mstorsjo, thank you for detailed explanation.
According to the CMake documentation, CMAKE_MSVC_RUNTIME_LIBRARY does not overwrite the /MT[d], /MD[d] flags manually specified in CMAKE_<LANG>_FLAGS_<CONFIG>. I think VCPKG should detect running CMake version and define CMAKE_MSVC_RUNTIME_LIBRARY (CMake 3.15 or later) or runtime flags with CMAKE_<LANG>_FLAGS_<CONFIG> (before CMake 3.15). This is what I going to do in #34117. Define _DEBUG is redundant anyway.

[mstorsjo]

According to the CMake documentation, CMAKE_MSVC_RUNTIME_LIBRARY does not overwrite the /MT[d], /MD[d] flags manually specified in CMAKE_<LANG>_FLAGS_<CONFIG>. I think VCPKG should detect running CMake version and define CMAKE_MSVC_RUNTIME_LIBRARY (CMake 3.15 or later) or runtime flags with CMAKE_<LANG>_FLAGS_<CONFIG> (before CMake 3.15). This is what I going to do in #34117. Define _DEBUG is redundant anyway.

Unfortunately, it's not that simple.

Even if you have CMake >= 3.15, if the project you're building has e.g. cmake_minimum_required(VERSION 3.13.0), then CMake will work according to the old mode. Or even if the project has set cmake_minimum_required to a version >= 3.15, the project can also set cmake_policy(SET CMP0091 OLD) to keep the old behaviour for CRT choice. (Or conversely, a project with a lower cmake_minimum_required can opt in to the newer behaviour with cmake_policy(SET CMP0091 NEW).

So in order to confidently pick between CMAKE_MSVC_RUNTIME_LIBRARY and CMAKE_<LANG>_FLAGS_<CONFIG>, vcpkg would need to know exactly which mechanism each project prefers - which changes over time as projects are upgraded for newer CMake behaviours.

I agree that this probably isn't workable, so I agree that the only reasonable thing vcpkg can do at the moment is to pass the choice both ways.

However the redundant /D_DEBUG shouldn't be necessary in any case, and is only a source for potential breakage.

[barcharcraz]

It sounds then like LLVM is trying to pick a CRT that does not match VCPKG_CRT_LINKAGE, which is doomed.

Unfortunately, LLVM is doing something cursed but not doomed here. It's picking the linkage for the sanitizer runtime libraries, which are designed to work with any program CRT linkage configuration. The asan runtimes linked against the release DLL CRT do support asan instrumented programs that are linked with the debug dll or debug static CRTs.