[PATCH] D155233: [CMake] Switch the CMP0091 policy (MSVC_RUNTIME_LIBRARY) to the new behaviour

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: compiler-rt/CMakeLists.txt:391
   # FIXME: In fact, sanitizers should support both /MT and /MD, see PR20214.
-  if(COMPILER_RT_HAS_MT_FLAG)
-    foreach(flag_var
-      CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE
-      CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
-      CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE
-      CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO)
-      string(REGEX REPLACE "/M[DT]d" "/MT" ${flag_var} "${${flag_var}}")
-      string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}")
-      string(REGEX REPLACE "/D_DEBUG" "" ${flag_var} "${${flag_var}}")
-    endforeach()
-  endif()
+  set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded)
+  # Remove any /M[DT][d] flags, and strip any definitions of _DEBUG.
----------------
smeenai wrote:
> https://bugs.llvm.org/show_bug.cgi?id=20214 was marked as fixed back in 2014. I realized that we'd been building compiler-rt with `/MD` for years (our flags were set up in a way that wasn't caught by the flag replacement below, I guess), and it seems to be working fine. Should we make this user-configurable now?
Hmm, regarding ASAN, I think there's two separate concerns - which CRT the ASAN DLL itself is linked against (which is what this code here would affect) and what kind of CRT for the user code that ASAN intercepts. The referenced bug seems to be about the latter. So while the bug is fixed, it might still be the case that the sanitizer code needs to becompiled in one specific way. (But the comment indeed seems outdated.)

Curiosly, how did you pass `/MD` to the compiler so that it passed through unmodified here?

As for making it user-configurable - at least some parts of the compiler-rt libraries and/or their tests require this - see the original issue at https://github.com/llvm/llvm-project/issues/62719. If we can fix that issue at the root, to allow getting rid of this override here, that'd be nice. But I wouldn't be surprised if at least some of the sanitizer libraries require being compiled in a specific way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155233