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

2023-10-18 Thread Chuanqi Xu via cfe-commits

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/69431

There are already 3 issues about the broken state of -fdelayed-template-parsing 
and C++20 modules:
- https://github.com/llvm/llvm-project/issues/61068
- https://github.com/llvm/llvm-project/issues/64810
- https://github.com/llvm/llvm-project/issues/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.

>From 197a64c2520a224ec81dcec363d96a6b42c4d567 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 18 Oct 2023 15:58:03 +0800
Subject: [PATCH] [C++20] [Modules] [Driver] Don't enable
 -fdelayed-template-parsing by default on windows with C++20 modules

There are already 3 issues about the broken state of
-fdelayed-template-parsing and C++20 modules:
- https://github.com/llvm/llvm-project/issues/61068
- https://github.com/llvm/llvm-project/issues/64810
- https://github.com/llvm/llvm-project/issues/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.
---
 .../clang/Basic/DiagnosticDriverKinds.td  |  4 +++
 clang/include/clang/Driver/Types.h|  3 +++
 clang/lib/Driver/ToolChains/Clang.cpp | 25 +--
 clang/lib/Driver/Types.cpp|  4 +++
 .../cl-delayed-template-parsing-modules.cppm  |  7 ++
 5 files changed, 35 insertions(+), 8 deletions(-)
 create mode 100644 clang/test/Driver/cl-delayed-template-parsing-modules.cppm

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>;
+
 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 introd

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

2023-10-18 Thread via cfe-commits

llvmbot wrote:




@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:
- https://github.com/llvm/llvm-project/issues/61068
- https://github.com/llvm/llvm-project/issues/64810
- https://github.com/llvm/llvm-project/issues/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.

---
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
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>;
+
 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) {
   }
 }
 

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

2023-10-18 Thread David Blaikie via cfe-commits

dwblaikie wrote:

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?

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-18 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Iain Sandoe via cfe-commits

https://github.com/iains commented:

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?


https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

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


https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Markus Böck via cfe-commits

zero9178 wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Reid Kleckner via cfe-commits

rnk wrote:

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.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Iain Sandoe via cfe-commits

iains wrote:

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


https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread David Blaikie via cfe-commits

dwblaikie wrote:

> > 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?

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Reid Kleckner via cfe-commits

rnk wrote:

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.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-19 Thread Markus Böck via cfe-commits

zero9178 wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread Chuanqi Xu via cfe-commits

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/69431

>From 076f2ea9de7d0f979431363b5426931239c7c494 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 18 Oct 2023 15:58:03 +0800
Subject: [PATCH] [C++20] [Modules] [Driver] Don't enable
 -fdelayed-template-parsing by default on windows after c++20

There are already 3 issues about the broken state of
-fdelayed-template-parsing and C++20 modules:
- https://github.com/llvm/llvm-project/issues/61068
- https://github.com/llvm/llvm-project/issues/64810
- https://github.com/llvm/llvm-project/issues/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 https://github.com/llvm/llvm-project/pull/69551,
we decide to not enable -fdelayed-template-parsing by default on windows
after c++20
---
 clang/docs/ReleaseNotes.rst   |  4 ++
 .../clang/Basic/DiagnosticDriverKinds.td  |  4 ++
 clang/lib/Driver/ToolChains/Clang.cpp | 54 +++
 .../cl-delayed-template-parsing-cxx20.cpp | 10 
 4 files changed, 51 insertions(+), 21 deletions(-)
 create mode 100644 clang/test/Driver/cl-delayed-template-parsing-cxx20.cpp

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 99525b00239a4ca..09406adcb5b87f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -100,6 +100,10 @@ C++ Specific Potentially Breaking Changes
   system headers and macros. It will be turned into a hard (non-downgradable)
   error in the next Clang release.
 
+- The flag `-fdelayed-template-parsing` won't be enabled by default with C++20
+  when targetting MSVC to match the behavior of MSVC. 
+  (`MSVC Docs 
`_)
+
 ABI Changes in This Version
 ---
 - Following the SystemV ABI for x86-64, ``__int128`` arguments will no longer
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td 
b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 9c00fa50bb95580..6110c6d6ea195b8 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_after_cxx20 : Warning<
+  "-fdelayed-template-parsing is deprecated after C++20">,
+  InGroup>;
+
 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/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 94c184435ae14de..cf1157c9ab4716f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3724,20 +3724,10 @@ bool 
Driver::getDefaultModuleCachePath(SmallVectorImpl &Result) {
 
 static bool RenderModulesOptions(Compilation &C, const Driver &D,
  const ArgList &Args, const InputInfo &Input,
- const InputInfo &Output, const Arg *Std,
+ const InputInfo &Output, bool HaveStd20,
  ArgStringList &CmdArgs) {
   bool IsCXX = types::isCXX(Input.getType());
-  // FIXME: Find a better way to determine whether the input has standard c++
-  // modules support by default.
-  bool HaveStdCXXModules =
-  IsCXX && Std &&
-  (Std->containsValue("c++2a") || Std->containsValue("gnu++2a") ||
-   Std->containsValue("c++20") || Std->containsValue("gnu++20") ||
-   Std->containsValue("c++2b") || Std->containsValue("gnu++2b") ||
-   Std->containsValue("c++23") || Std->containsValue("gnu++23") ||
-   Std->containsValue("c++2c") || Std->containsValue("gnu++2c") ||
-   Std->containsValue("c++26") || Std->containsValue("gnu++26") ||
-   Std->containsValue("c++latest") || Std->containsValue("gnu++latest"));
+  bool HaveStdCXXModules = IsCXX && HaveStd20;
   bool HaveModules = HaveStdCXXModules;
 
   // -fmodules enables the use of precompiled modules (off by default).
@@ -6842,14 +6832,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: 

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

2023-10-20 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

> > > 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!

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread David Blaikie via cfe-commits

https://github.com/dwblaikie edited 
https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread David Blaikie via cfe-commits

https://github.com/dwblaikie approved this pull request.

Some minor comments but generally looks good to me.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread David Blaikie via cfe-commits


@@ -6870,8 +6852,38 @@ void Clang::ConstructJob(Compilation &C, const JobAction 
&JA,
 
   Args.AddLastArg(CmdArgs, options::OPT_finline_max_stacksize_EQ);
 
+  // FIXME: Find a better way to determine whether we are in C++20.
+  bool HaveCxx20 =
+  Std &&
+  (Std->containsValue("c++2a") || Std->containsValue("gnu++2a") ||
+   Std->containsValue("c++20") || Std->containsValue("gnu++20") ||
+   Std->containsValue("c++2b") || Std->containsValue("gnu++2b") ||
+   Std->containsValue("c++23") || Std->containsValue("gnu++23") ||
+   Std->containsValue("c++2c") || Std->containsValue("gnu++2c") ||
+   Std->containsValue("c++26") || Std->containsValue("gnu++26") ||
+   Std->containsValue("c++latest") || Std->containsValue("gnu++latest"));
   bool HaveModules =
-  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.

dwblaikie wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread David Blaikie via cfe-commits


@@ -6870,8 +6852,38 @@ void Clang::ConstructJob(Compilation &C, const JobAction 
&JA,
 
   Args.AddLastArg(CmdArgs, options::OPT_finline_max_stacksize_EQ);
 
+  // FIXME: Find a better way to determine whether we are in C++20.
+  bool HaveCxx20 =
+  Std &&
+  (Std->containsValue("c++2a") || Std->containsValue("gnu++2a") ||
+   Std->containsValue("c++20") || Std->containsValue("gnu++20") ||
+   Std->containsValue("c++2b") || Std->containsValue("gnu++2b") ||
+   Std->containsValue("c++23") || Std->containsValue("gnu++23") ||
+   Std->containsValue("c++2c") || Std->containsValue("gnu++2c") ||
+   Std->containsValue("c++26") || Std->containsValue("gnu++26") ||
+   Std->containsValue("c++latest") || Std->containsValue("gnu++latest"));
   bool HaveModules =
-  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.
+  // Many old Windows SDK versions require this to parse.
+  //
+  // 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-

dwblaikie wrote:

"the" can be removed here, 

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-20 Thread David Blaikie via cfe-commits


@@ -6870,8 +6852,38 @@ void Clang::ConstructJob(Compilation &C, const JobAction 
&JA,
 
   Args.AddLastArg(CmdArgs, options::OPT_finline_max_stacksize_EQ);
 
+  // FIXME: Find a better way to determine whether we are in C++20.
+  bool HaveCxx20 =
+  Std &&
+  (Std->containsValue("c++2a") || Std->containsValue("gnu++2a") ||
+   Std->containsValue("c++20") || Std->containsValue("gnu++20") ||
+   Std->containsValue("c++2b") || Std->containsValue("gnu++2b") ||
+   Std->containsValue("c++23") || Std->containsValue("gnu++23") ||
+   Std->containsValue("c++2c") || Std->containsValue("gnu++2c") ||
+   Std->containsValue("c++26") || Std->containsValue("gnu++26") ||
+   Std->containsValue("c++latest") || Std->containsValue("gnu++latest"));
   bool HaveModules =
-  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.
+  // Many old Windows SDK versions require this to parse.
+  //
+  // 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-
+  // 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.

dwblaikie wrote:

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

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-22 Thread Chuanqi Xu via cfe-commits

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/69431

>From 9c0d81ef5fdae40d378170eebd848f099902dc98 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Wed, 18 Oct 2023 15:58:03 +0800
Subject: [PATCH] [C++20] [Modules] [Driver] Don't enable
 -fdelayed-template-parsing by default on windows after c++20

There are already 3 issues about the broken state of
-fdelayed-template-parsing and C++20 modules:
- https://github.com/llvm/llvm-project/issues/61068
- https://github.com/llvm/llvm-project/issues/64810
- https://github.com/llvm/llvm-project/issues/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 https://github.com/llvm/llvm-project/pull/69551,
we decide to not enable -fdelayed-template-parsing by default on windows
after c++20
---
 clang/docs/ReleaseNotes.rst   |  4 ++
 .../clang/Basic/DiagnosticDriverKinds.td  |  4 ++
 clang/lib/Driver/ToolChains/Clang.cpp | 54 +++
 .../cl-delayed-template-parsing-cxx20.cpp | 10 
 4 files changed, 51 insertions(+), 21 deletions(-)
 create mode 100644 clang/test/Driver/cl-delayed-template-parsing-cxx20.cpp

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 99525b00239a4ca..09406adcb5b87f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -100,6 +100,10 @@ C++ Specific Potentially Breaking Changes
   system headers and macros. It will be turned into a hard (non-downgradable)
   error in the next Clang release.
 
+- The flag `-fdelayed-template-parsing` won't be enabled by default with C++20
+  when targetting MSVC to match the behavior of MSVC. 
+  (`MSVC Docs 
`_)
+
 ABI Changes in This Version
 ---
 - Following the SystemV ABI for x86-64, ``__int128`` arguments will no longer
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td 
b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 9c00fa50bb95580..6110c6d6ea195b8 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_after_cxx20 : Warning<
+  "-fdelayed-template-parsing is deprecated after C++20">,
+  InGroup>;
+
 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/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 94c184435ae14de..5d77f69ae703f6c 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3724,20 +3724,10 @@ bool 
Driver::getDefaultModuleCachePath(SmallVectorImpl &Result) {
 
 static bool RenderModulesOptions(Compilation &C, const Driver &D,
  const ArgList &Args, const InputInfo &Input,
- const InputInfo &Output, const Arg *Std,
+ const InputInfo &Output, bool HaveStd20,
  ArgStringList &CmdArgs) {
   bool IsCXX = types::isCXX(Input.getType());
-  // FIXME: Find a better way to determine whether the input has standard c++
-  // modules support by default.
-  bool HaveStdCXXModules =
-  IsCXX && Std &&
-  (Std->containsValue("c++2a") || Std->containsValue("gnu++2a") ||
-   Std->containsValue("c++20") || Std->containsValue("gnu++20") ||
-   Std->containsValue("c++2b") || Std->containsValue("gnu++2b") ||
-   Std->containsValue("c++23") || Std->containsValue("gnu++23") ||
-   Std->containsValue("c++2c") || Std->containsValue("gnu++2c") ||
-   Std->containsValue("c++26") || Std->containsValue("gnu++26") ||
-   Std->containsValue("c++latest") || Std->containsValue("gnu++latest"));
+  bool HaveStdCXXModules = IsCXX && HaveStd20;
   bool HaveModules = HaveStdCXXModules;
 
   // -fmodules enables the use of precompiled modules (off by default).
@@ -6842,14 +6832,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: 

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

2023-10-22 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

Comments addressed.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-22 Thread Chuanqi Xu via cfe-commits

https://github.com/ChuanqiXu9 closed 
https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-23 Thread via cfe-commits

zeroomega wrote:

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.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-23 Thread Petr Hosek via cfe-commits

petrhosek wrote:

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
```

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2023-10-23 Thread Chuanqi Xu via cfe-commits

ChuanqiXu9 wrote:

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.

https://github.com/llvm/llvm-project/pull/69431
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits