Skip to content
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

[C++20] [Modules] [Driver] Don't enable -fdelayed-template-parsing by default on windows with C++20 modules #69431

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

ChuanqiXu9
Copy link
Member

There are already 3 issues about the broken state of -fdelayed-template-parsing and C++20 modules:

The problem is more complex than I thought. I am not sure how to fix it properly now. Given the complexities and -fdelayed-template-parsing is actually an extension to support old MS codes, I think it may make sense to not enable the -fdelayed-template-parsing option by default with C++20 modules to give more user friendly experience. Users who still want -fdelayed-template-parsing can specify it explicitly.

@ChuanqiXu9 ChuanqiXu9 added clang:modules C++20 modules and Clang Header Modules platform:windows labels Oct 18, 2023
@ChuanqiXu9 ChuanqiXu9 self-assigned this Oct 18, 2023
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 18, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 18, 2023

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

There are already 3 issues about the broken state of -fdelayed-template-parsing and C++20 modules:

The problem is more complex than I thought. I am not sure how to fix it properly now. Given the complexities and -fdelayed-template-parsing is actually an extension to support old MS codes, I think it may make sense to not enable the -fdelayed-template-parsing option by default with C++20 modules to give more user friendly experience. Users who still want -fdelayed-template-parsing can specify it explicitly.


Full diff: https://github.com/llvm/llvm-project/pull/69431.diff

5 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+4)
  • (modified) clang/include/clang/Driver/Types.h (+3)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+17-8)
  • (modified) clang/lib/Driver/Types.cpp (+4)
  • (added) clang/test/Driver/cl-delayed-template-parsing-modules.cppm (+7)
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 9c00fa50bb95580..0a64f8573931f37 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -527,6 +527,10 @@ def err_test_module_file_extension_format : Error<
 def err_drv_module_output_with_multiple_arch : Error<
   "option '-fmodule-output' can't be used with multiple arch options">;
 
+def warn_drv_delayed_template_parsing_with_module : Warning<
+  "-fdelayed-template-parsing is broken with C++20 modules">,
+  InGroup<DiagGroup<"delayed-template-parsing-in-modules">>;
+
 def err_drv_extract_api_wrong_kind : Error<
   "header file '%0' input '%1' does not match the type of prior input "
   "in api extraction; use '-x %2' to override">;
diff --git a/clang/include/clang/Driver/Types.h b/clang/include/clang/Driver/Types.h
index 121b58a6b477d9b..b6a05e1ec9d624a 100644
--- a/clang/include/clang/Driver/Types.h
+++ b/clang/include/clang/Driver/Types.h
@@ -80,6 +80,9 @@ namespace types {
   /// isCXX - Is this a "C++" input (C++ and Obj-C++ sources and headers).
   bool isCXX(ID Id);
 
+  /// isCXXModules - Is this a standard C++ modules input.
+  bool isCXXModules(ID Id);
+
   /// Is this LLVM IR.
   bool isLLVMIR(ID Id);
 
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 94c184435ae14de..21aee28a2b6ba9b 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6842,14 +6842,6 @@ 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.
-  if (Args.hasFlag(options::OPT_fdelayed_template_parsing,
-                   options::OPT_fno_delayed_template_parsing, IsWindowsMSVC))
-    CmdArgs.push_back("-fdelayed-template-parsing");
-
   // -fgnu-keywords default varies depending on language; only pass if
   // specified.
   Args.AddLastArg(CmdArgs, options::OPT_fgnu_keywords,
@@ -6872,6 +6864,23 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
 
   bool HaveModules =
       RenderModulesOptions(C, D, Args, Input, Output, Std, CmdArgs);
+  bool IsModulesInput = HaveModules && isCXXModules(Input.getType());
+  // It is possible to compile .pcm files with C++20 modules.
+  IsModulesInput |= Input.getType() == types::TY_ModuleFile;
+
+  // -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.
+  // FIXME: The -fdelayed-template-parsing mode is broken with C++20 modules.
+  if (Args.hasFlag(options::OPT_fdelayed_template_parsing,
+                   options::OPT_fno_delayed_template_parsing,
+                   IsWindowsMSVC && !IsModulesInput)) {
+    if (IsModulesInput)
+      D.Diag(clang::diag::warn_drv_delayed_template_parsing_with_module);
+
+    CmdArgs.push_back("-fdelayed-template-parsing");
+  }
 
   if (Args.hasFlag(options::OPT_fpch_validate_input_files_content,
                    options::OPT_fno_pch_validate_input_files_content, false))
diff --git a/clang/lib/Driver/Types.cpp b/clang/lib/Driver/Types.cpp
index 08df34ade7b6530..6977b5509c26a9b 100644
--- a/clang/lib/Driver/Types.cpp
+++ b/clang/lib/Driver/Types.cpp
@@ -249,6 +249,10 @@ bool types::isCXX(ID Id) {
   }
 }
 
+bool types::isCXXModules(ID Id) {
+  return Id == TY_CXXModule || Id == TY_PP_CXXModule;
+}
+
 bool types::isLLVMIR(ID Id) {
   switch (Id) {
   default:
diff --git a/clang/test/Driver/cl-delayed-template-parsing-modules.cppm b/clang/test/Driver/cl-delayed-template-parsing-modules.cppm
new file mode 100644
index 000000000000000..fee12ae7034aed2
--- /dev/null
+++ b/clang/test/Driver/cl-delayed-template-parsing-modules.cppm
@@ -0,0 +1,7 @@
+// RUN: %clang_cl -std:c++20 -### -- %s 2>&1 | FileCheck %s
+// RUN: %clang_cl -std:c++20 -### -fdelayed-template-parsing -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-EXPLICIT
+
+// CHECK-NOT: -fdelayed-template-parsing
+
+// CHECK-EXPLICIT: -fdelayed-template-parsing
+// CHECK-EXPLICIT: warning: -fdelayed-template-parsing is broken with C++20 modules

@dwblaikie
Copy link
Collaborator

Does MSVC have the delayed template parsing effects when using modules? If not, perhaps we should just disable the flag/not allow it to be composed together?

@ChuanqiXu9
Copy link
Member Author

Does MSVC have the delayed template parsing effects when using modules? If not, perhaps we should just disable the flag/not allow it to be composed together?

As far as I can reach, (from the issue reports in MSVC community), MSVC don't have problems with the delayed template parsing and the modules. So if our goal of -fdelayed-template-parsing is to keep the same/similar behavior with MSVC, we have a problem now. Given we don't know how to fix the real problem and it indeed affects multiple people now, I think the current patch may be a understandable workaround.

Copy link
Contributor

@iains iains left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is delayed template parsing an optimisation or a correctness measure?
If it's an optimisation, then it seems that we should disable it for modules (because that then makes the modules cases correct). If it's needed for correctness, then we have more of a problem - do we know how MSVC makes the two interact?

@ChuanqiXu9
Copy link
Member Author

Is delayed template parsing an optimisation or a correctness measure? If it's an optimisation, then it seems that we should disable it for modules (because that then makes the modules cases correct). If it's needed for correctness, then we have more of a problem - do we know how MSVC makes the two interact?

According to my readings, this is about the correctness for the extensions in older Windows SDK. The comment for -fdelayed-template-parsing is:

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

So while it is about the correctness for some legacy cases, it should be disabled by default at some point according to our plan. (Although I am not sure if any one is watching on this).

If it's needed for correctness, then we have more of a problem - do we know how MSVC makes the two interact?

I think we can only achieve this by reaching out Cameron.

@zero9178
Copy link
Member

According to the docs [0], MSVC actually defaults to -fno-delayed-template-parsing (/Zc:twoPhase- with MSVC CLI) if using C++20. This is due to -std:c++20 implying /permissive- which implies /Zc:twoPhase-. We could therefore just disable it based on language version alone, not just based on whether we are using modules.

I previously tried to make it the default everywhere in https://reviews.llvm.org/D103772. @rnk argued we could always make it the default. Given that MSVC is essentially phasing it out and making all their headers compatible with /Zc:twoPhase- for the sake of C++20 support, it should be more feasible than previously.

[0] https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170

@ChuanqiXu9
Copy link
Member Author

According to the docs [0], MSVC actually defaults to -fno-delayed-template-parsing (/Zc:twoPhase- with MSVC CLI) if using C++20. This is due to -std:c++20 implying /permissive- which implies /Zc:twoPhase-. We could therefore just disable it based on language version alone, not just based on whether we are using modules.

I previously tried to make it the default everywhere in https://reviews.llvm.org/D103772. @rnk argued we could always make it the default. Given that MSVC is essentially phasing it out and making all their headers compatible with /Zc:twoPhase- for the sake of C++20 support, it should be more feasible than previously.

[0] https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170

Thanks for the info! It looks like your patch can cover this patch. And we can be sure that this patch itself is correct.

Would you like to send that patch itself soon? If not, I'd like to land the patch soon to give better user experience. Then you can revert this. If you plan to send that patch soon, I'd like to discard the patch itself.

@rnk
Copy link
Collaborator

rnk commented Oct 19, 2023

I still support disabling delayed template parsing by default in all configurations. Ultimately, this feature is a source of bugs, and we should start the clock on its deprecation and removal. This, of course, involves real work, and I haven't allocated any time (mine or others') to it.

@iains
Copy link
Contributor

iains commented Oct 19, 2023

I still support disabling delayed template parsing by default in all configurations. Ultimately, this feature is a source of bugs, and we should start the clock on its deprecation and removal. This, of course, involves real work, and I haven't allocated any time (mine or others') to it.

So @ChuanqiXu9 's patch is at least a conservative step towards that (limiting the change to C++20 modules where we know there is a problem) - we could then extend that to C++20, in general (as per @zero9178 's comment above). As you say deprecation and removal is more work.

@dwblaikie
Copy link
Collaborator

I still support disabling delayed template parsing by default in all configurations. Ultimately, this feature is a source of bugs, and we should start the clock on its deprecation and removal. This, of course, involves real work, and I haven't allocated any time (mine or others') to it.

So @ChuanqiXu9 's patch is at least a conservative step towards that (limiting the change to C++20 modules where we know there is a problem) - we could then extend that to C++20, in general (as per @zero9178 's comment above). As you say deprecation and removal is more work.

Yeah, how about we at least match MSVC here - and. generalize it to C++20 today, leaving the older deprecation work to others/another time?

@rnk
Copy link
Collaborator

rnk commented Oct 19, 2023

We can definitely disable delayed template parsing in C++20 mode, I wouldn't argue against that. Whoever actually does the work should decide how much effort they are willing to put in. I'm just saying there are benefits to starting the deprecation clock sooner, since it will ultimately reduce tech debt sooner, and avoid additional confusing behavior differences between C++17 and C++20. I think all it takes is an extra RFC and a release note.

@zero9178
Copy link
Member

According to the docs [0], MSVC actually defaults to -fno-delayed-template-parsing (/Zc:twoPhase- with MSVC CLI) if using C++20. This is due to -std:c++20 implying /permissive- which implies /Zc:twoPhase-. We could therefore just disable it based on language version alone, not just based on whether we are using modules.
I previously tried to make it the default everywhere in https://reviews.llvm.org/D103772. @rnk argued we could always make it the default. Given that MSVC is essentially phasing it out and making all their headers compatible with /Zc:twoPhase- for the sake of C++20 support, it should be more feasible than previously.
[0] https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170

Thanks for the info! It looks like your patch can cover this patch. And we can be sure that this patch itself is correct.

Would you like to send that patch itself soon? If not, I'd like to land the patch soon to give better user experience. Then you can revert this. If you plan to send that patch soon, I'd like to discard the patch itself.

I sadly won't have the time to push this so feel free to proceed working on this PR.

I still support disabling delayed template parsing by default in all configurations. Ultimately, this feature is a source of bugs, and we should start the clock on its deprecation and removal. This, of course, involves real work, and I haven't allocated any time (mine or others') to it.

So @ChuanqiXu9 's patch is at least a conservative step towards that (limiting the change to C++20 modules where we know there is a problem) - we could then extend that to C++20, in general (as per @zero9178 's comment above). As you say deprecation and removal is more work.

Yeah, how about we at least match MSVC here - and. generalize it to C++20 today, leaving the older deprecation work to others/another time?

I would also support this approach as it can also be seen as matching MSVC behaviour

@ChuanqiXu9
Copy link
Member Author

I still support disabling delayed template parsing by default in all configurations. Ultimately, this feature is a source of bugs, and we should start the clock on its deprecation and removal. This, of course, involves real work, and I haven't allocated any time (mine or others') to it.

So @ChuanqiXu9 's patch is at least a conservative step towards that (limiting the change to C++20 modules where we know there is a problem) - we could then extend that to C++20, in general (as per @zero9178 's comment above). As you say deprecation and removal is more work.

Yeah, how about we at least match MSVC here - and. generalize it to C++20 today, leaving the older deprecation work to others/another time?

+1 and done in the latest commit!

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but generally looks good to me.

RenderModulesOptions(C, D, Args, Input, Output, Std, CmdArgs);
RenderModulesOptions(C, D, Args, Input, Output, HaveCxx20, CmdArgs);

// -fno-delayed-template-parsing is default, except when targeting MSVC.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rephrase to: -fdelayed-template-parsing when targeting MSVC (rather than multiple negations "no" and "except")

//
// According to
// https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170,
// the MSVC actually defaults to -fno-delayed-template-parsing (/Zc:twoPhase-
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the" can be removed here,

// https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170,
// the MSVC actually defaults to -fno-delayed-template-parsing (/Zc:twoPhase-
// with MSVC CLI) if using C++20. So we match the behavior with MSVC here to
// not enable -fno-delayed-template-parsing by default after C++20.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to not enable -fdelayed-template-parsing by default" (typo, the patch current has "-fno" here)

… default on windows after c++20

There are already 3 issues about the broken state of
-fdelayed-template-parsing and C++20 modules:
- llvm#61068
- llvm#64810
- llvm#65027

The problem is more complex than I thought. I am not sure how to fix it
properly now. Given the complexities and -fdelayed-template-parsing is
actually an extension to support old MS codes, I think it may make sense
to not enable the -fdelayed-template-parsing option by default with
C++20 modules to give more user friendly experience. Users who still want
-fdelayed-template-parsing can specify it explicitly.

Given the discussion in llvm#69551,
we decide to not enable -fdelayed-template-parsing by default on windows
after c++20
@ChuanqiXu9
Copy link
Member Author

Comments addressed.

@zeroomega
Copy link
Contributor

FYI, we are seeing libc++ test failures on Windows after this patch landed, for example:
llvm-libc++-static-clangcl.cfg.in :: std/numerics/c.math/cmath.pass.cpp:

# .---command stderr------------
# | In file included from C:\b\s\w\ir\x\w\llvm-llvm-project\libcxx\test\std\numerics\c.math\cmath.pass.cpp:11:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\cmath:319:
# | In file included from C:/b/s/w/ir/x/w/llvm_build/include/c++/v1\math.h:301:
# | In file included from C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\math.h:11:
# | C:\b\s\w\ir\cache\windows_sdk\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_math.h:401:16: error: call to 'fpclassify' is ambiguous
# |   401 |         return fpclassify(_X) <= 0;

Failed build task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8766483127060472193/overview

We will try to manually enable -fdelayed-template-parsing to see if it clears the error. However, these libc++ tests probably needs an update.

@petrhosek
Copy link
Member

This broke the following tests on Windows:

llvm-libc++-static-clangcl.cfg.in :: libcxx/atomics/diagnose_invalid_memory_order.verify.cpp
llvm-libc++-static-clangcl.cfg.in :: libcxx/fuzzing/random.pass.cpp
llvm-libc++-static-clangcl.cfg.in :: std/depr/depr.c.headers/math_h.pass.cpp
llvm-libc++-static-clangcl.cfg.in :: std/numerics/c.math/cmath.pass.cpp

@ChuanqiXu9
Copy link
Member Author

Yeah, this is the reason why I put the change in C++ Specific Potentially Breaking Changes section. I think specify -fdelayed-template-parsing explicitly may be a good solution/workaroud.

petrhosek added a commit to petrhosek/llvm-project that referenced this pull request Oct 30, 2023
Some tests starting passing/failing after llvm#69431 because Clang no longer
enables -fdelayed-template-parsing by default on Windows with C++20.
ldionne pushed a commit that referenced this pull request Oct 31, 2023
Some tests starting passing/failing after #69431 because Clang no longer
enables -fdelayed-template-parsing by default on Windows with C++20.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category platform:windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants