-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[compiler-rt] Reinstate removal of CRT choice flags from CMAKE_*_FLAGS* #67935
Conversation
This reverts one part of commit 9f4dfcb, with a modified comment added about the code. Ideally, this would only be reinstated temporarily - but given the situation in vcpkg, it looks likely that they would keep passing the duplicate options for quite some time. The conflicting CRT choice usually are benign but only would cause warnings about one option overriding the other, if passing e.g. "/MDd /MT". However when vcpkg currently sets these options in CMAKE_*_FLAGS_DEBUG, it passes the redundant option /D_DEBUG; thus the compiler finally ends up with e.g. "/D_DEBUG /MDd /MT", which has the effect of defining _DEBUG while using a release mode CRT, which allegedly breaks the build. There's a PR up for removing this redundant /D_DEBUG option in vcpkg in microsoft/vcpkg#34123. With that in place, this change wouldn't be strictly needed.
@llvm/pr-subscribers-platform-windows ChangesThis reverts one part of commit 9f4dfcb, with a modified comment added about the code. Ideally, this would only be reinstated temporarily - but given the situation in vcpkg, it looks likely that they would keep passing the duplicate options for quite some time. The conflicting CRT choice usually are benign but only would cause warnings about one option overriding the other, if passing e.g. "/MDd /MT". However when vcpkg currently sets these options in CMAKE_*_FLAGS_DEBUG, it passes the redundant option /D_DEBUG; thus the compiler finally ends up with e.g. "/D_DEBUG /MDd /MT", which has the effect of defining _DEBUG while using a release mode CRT, which allegedly breaks the build. There's a PR up for removing this redundant /D_DEBUG option in vcpkg in microsoft/vcpkg#34123. With that in place, this change wouldn't be strictly needed. Full diff: https://github.com/llvm/llvm-project/pull/67935.diff 1 Files Affected:
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index cc3190bd7f761e2..514ac250075a8f2 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -375,6 +375,25 @@ endif()
if(MSVC)
# FIXME: In fact, sanitizers should support both /MT and /MD, see PR20214.
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded)
+
+ # Remove any /M[DT][d] flags, and strip any definitions of _DEBUG.
+ # Since we're using CMAKE_MSVC_RUNTIME_LIBRARY (CMP0091 set to NEW),
+ # these options shouldn't be included in these flags variables. However,
+ # package managers that don't know which mechanism is used for passing
+ # CRT choice flags might be passing them both ways - which leads to
+ # duplicate CRT choice options. Thus make sure to strip out these flags
+ # from these variables, when we're forcing a CRT choice other than what
+ # the user requested here.
+ 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" "" ${flag_var} "${${flag_var}}")
+ string(REGEX REPLACE "[/-]MD" "" ${flag_var} "${${flag_var}}")
+ string(REGEX REPLACE "[/-]D_DEBUG" "" ${flag_var} "${${flag_var}}")
+ endforeach()
+
append_list_if(COMPILER_RT_HAS_Oy_FLAG /Oy- SANITIZER_COMMON_CFLAGS)
append_list_if(COMPILER_RT_HAS_GS_FLAG /GS- SANITIZER_COMMON_CFLAGS)
|
# FIXME: In fact, sanitizers should support both /MT and /MD, see PR20214. | ||
set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me ask a stupid question: Why is this even set? The VS sanitizers come in all flavors and are basically derived from llvm as far as I know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Microsoft haven’t yet upstreamed their changes where they make it possible to build the sanitizers with any CRT. The code here, in its current state, only support this one setting - allegedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…S* (#67935) This reverts one part of commit 9f4dfcb, with a modified comment added about the code. Ideally, this would only be reinstated temporarily - but given the situation in vcpkg, it looks likely that they would keep passing the duplicate options for quite some time. The conflicting CRT choice usually are benign but only would cause warnings about one option overriding the other, if passing e.g. "/MDd /MT". However when vcpkg currently sets these options in CMAKE_*_FLAGS_DEBUG, it passes the redundant option /D_DEBUG; thus the compiler finally ends up with e.g. "/D_DEBUG /MDd /MT", which has the effect of defining _DEBUG while using a release mode CRT, which allegedly breaks the build. There's a PR up for removing this redundant /D_DEBUG option in vcpkg in microsoft/vcpkg#34123. With that in place, this change wouldn't be strictly needed. (cherry picked from commit 7bc09a4)
This reverts one part of commit 9f4dfcb, with a modified comment added about the code.
Ideally, this would only be reinstated temporarily - but given the situation in vcpkg, it looks likely that they would keep passing the duplicate options for quite some time. The conflicting CRT choice usually are benign but only would cause warnings about one option overriding the other, if passing e.g. "/MDd /MT".
However when vcpkg currently sets these options in CMAKE_*_FLAGS_DEBUG, it passes the redundant option /D_DEBUG; thus the compiler finally ends up with e.g. "/D_DEBUG /MDd /MT", which has the effect of defining _DEBUG while using a release mode CRT, which allegedly breaks the build.
There's a PR up for removing this redundant /D_DEBUG option in vcpkg in microsoft/vcpkg#34123. With that in place, this change wouldn't be strictly needed.