[ms] Enable /Zc:twoPhase by default if fmsc-version is 1911+

[nico]

Bugzilla Link	43032
Version	trunk
OS	All
CC	@DougGregor,@zygoloid,@rnk

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

[nico]

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");

-  // -fno-delayed-template-parsing is default, except when targeting MSVC.
-  // Many old Windows SDK versions require this to parse.
-  // FIXME: MSVC introduced /Zc:twoPhase- to disable this behavior in their
-  // compiler. We should be able to disable this by default at some point.
+  // -fno-delayed-template-parsing is default, except when targeting MSVC
+  // earlier than MSVC 2017 15.3 (_MSC_VER 1911).  Windows SDK versions
+  // 10.0.15063.0 (Creators Update or Redstone 2) and earlier require this to
+  // parse.
+  bool IsMSVCBefore2017Update3 = !MSVT.empty() && MSVT < VersionTuple(19, 11);
   if (Args.hasFlag(options::OPT_fdelayed_template_parsing,
-                   options::OPT_fno_delayed_template_parsing, IsWindowsMSVC))
+                   options::OPT_fno_delayed_template_parsing,
+                   IsMSVCBefore2017Update3))
     CmdArgs.push_back("-fdelayed-template-parsing");

   // -fgnu-keywords default varies depending on language; only pass if

[nico]

patch
With tests.

Still need to verify this does the right thing without an explicit -fmsc-version flag.

[nico]

https://reviews.llvm.org/D66394

[rnk]

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 header from MSVC 14.16.27023, either by raising our msvc compat check, or by tweaking the -fno-rtti / -fno-rtti-data settings somewhere.

[rnk]

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.

[nico]

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:

1. Enabling two-phase lookup caused problems in system headers around RTTI: MS's STL's functional header used to use typeof(void) when RTTI was off with two-phase lookup enabled. This got fixed in microsoft/STL@d0ff26f
2. For RTTI to work, you have to define the (obscure) macro _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?)
3. https://reviews.llvm.org/D103773 added /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.

[rnk]

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.

[nico]

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

[rnk]

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.

[nico]

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