Open nico opened 5 years ago
This landed and then got reverted, but I'd love to see it come back. :)
To reland, we need to make sure check-asan passes on Windows with the
patch With tests.
Still need to verify this does the right thing without an explicit -fmsc-version flag.
Maybe this does it. Need to run and update tests, and need to check that this correctly picks up the version of system cl.exe if no explicit -fmsc-version flag is passed.
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 690d4fa3fa4..c0f914bf989 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -4883,12 +4883,14 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA, !IsWindowsMSVC || IsMSVC2015Compatible)) CmdArgs.push_back("-fno-threadsafe-statics");
IsMSVCBefore2017Update3)) CmdArgs.push_back("-fdelayed-template-parsing");
// -fgnu-keywords default varies depending on language; only pass if
This came up again last June: https://reviews.llvm.org/D103772
Delayed template parsing continues to cause issues for AST consumers such as clang-tidy. The effect is that the body of a function template is simply a null pointer, which usually results in analyzer crashes. See this recent Discourse thread: https://discourse.llvm.org/t/how-do-you-access-the-body-of-a-template-function-in-the-ast/61829
So, I think it's worth attempting to make this change again.
I've been bad about updating this. Here's an update of things that happened (or didn't happen) in the last 2.5 years:
Most importantly, the premise is wrong. MSVC does not enable /Zc:twoPhase
by default, it only does this if you pass /permissive-
. https://godbolt.org/z/a63KdbsEK shows this. @StephanTLavavej explained that to me over email in 2019. The language on https://docs.microsoft.com/en-us/cpp/build/reference/zc-twophase?view=msvc-170&viewFallbackFrom=vs-2019 used to be misleading. The documentation got fixed to no longer be misleading. (cl.exe doesn't even have /Zc:twoPhase
, only /Zc:twoPhase-
which you can use if you want /permissive-
but don't want standard two-phase lookup).
So just based on cl.exe not doing it by default, clang-cl should neither I think.
There's some discussion of the revert at https://bugs.chromium.org/p/chromium/issues/detail?id=996675. It also mentions a few issues:
functional
header used to use typeof(void)
when RTTI was off with two-phase lookup enabled. This got fixed in https://github.com/microsoft/STL/commit/d0ff26f92e8ac3ba68382f33d5a7b975d18304de_HAS_STATIC_RTTI
. https://reviews.llvm.org/D103771 made it so that clang auto-defines it when using -fno-rtti
(…but not when using OPT__SLASH_GR_
with clang-cl, which is maybe something we want to do?)/permissive-
to clang-cl and made it imply two-phase lookup, among other things.So I think most things that we realistically want to do here are probably done.
Unless we want to deviate from cl.exe behavior, but I'm not sure that's a good idea.
I had some text addressing the angle of MSVC compatibility in the discourse thread:
As I understand it, flipping the default here would be inconsistent with MSVC. Some people look at this issue and say, “MSVC uses /Zc:twoPhase- by default, so clang-cl should follow MSVC”. However, Clang’s -fdelayed-template-parsing setting isn’t really the same thing as /Zc:twoPhase-, and I think if we can address the build failure from last time, users will get more value from a more C+±conforming default compiler behavior. So, I think we should go ahead and make the change anyway.
Basically, I hear the concern, but I still think we should do it anyway. It's true, the premise of the issue summary is wrong, but users will be better off if we sunset this error prone compatibility mode now.
I don't have an opinion on changing the default. (Would you want to change just the default of /Zc:twoPhase
, or of /permissive-
? Probably the former?)
Chrome builds with /Zc:twoPhase
and it builds fine. One target opts out since some system header couldn't handle it, as of 2021: https://source.chromium.org/chromium/chromium/src/+/main:v8/tools/v8windbg/BUILD.gn;l=9?q=zc:twophase%20file:%5C.gn So we couldn't completely sunset it. (It might still be the better default.)
I only think we should change the default for -fdelayed-template-parsing
.
The V8 thing is interesting, since it suggests that various Windows SDK headers (DbgModel.h) still don't compile with /permissive-
.
I think the next step here is to look at the _HAS_STATIC_RTTI thing, so that /GR-
works. If Clang has to define it, that's probably worth it.
TIL that /std:c++20
implies /permissive-
(which implies two-phase name lookup) in newer MSVCs. We don't implement that in clang-cl yet, but we should. Once that's in, maybe we don't need to do anything else here as it'll go away over time? On the other hand, might as well flip the default since it's easy to turn it off for people who can't handle the new default, and that's the long-term direction everyone wants.
(I mostly just wanted to mention that with C++20, MSVC has two-phase lookup by default now.)
Extended Description
https://docs.microsoft.com/en-us/cpp/build/reference/zc-twophase?view=vs-2019 claims that cl.exe enables /Zc:twoPhase by default as of Visual Studio 2017 version 15.3, so we should change clang-cl to behave like that as well.
15.3+ seems to correspond to _MSC_VER 1911+.