[PATCH] D155431: [CMake] Clean up old code for handling MSVC runtime setting the old way

Thu Sep 28 03:42:05 PDT 2023

mstorsjo added a comment.

In D155431#4651633 <https://reviews.llvm.org/D155431#4651633>, @yurybura wrote:

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155431/new/

https://reviews.llvm.org/D155431