Add ability to turn off -fpch-instantiate-templates in clang-cl

[shivanshu3]

Summary

A lot of our code building with clang-cl.exe using Clang 11 was failing with the following 2 type of errors:

1. explicit specialization of 'foo' after instantiation
2. no matching function for call to 'bar'

Note that we also use -fdelayed-template-parsing in our builds.

I tried pretty hard to get a small repro for these failures, but couldn't. So there is some subtle edge case in the -fpch-instantiate-templates feature introduced by this change:
https://reviews.llvm.org/D69585

When I tried turning this off using -fno-pch-instantiate-templates, builds would silently fail with the same error without any indication that -fno-pch-instantiate-templates was being ignored by the compiler. Then I realized this "no" option wasn't actually working when I ran Clang under a debugger.

[shivanshu3]

Note that I do not have commit access and this change will have to be committed by someone else on my behalf. Please use the following commit author details:
"Shivan Goyal <[e-mail]>"

[rnk]

I think the flag was originally intended to be an internal -cc1 flag not exposed to users. You should be able to work around your problem with -Xclang -fno-pch-instantiate-templates, btw.

---

@zequanwu, can you patch this in locally and take over this patch? Please address the hasFlag comment above, add a test to clang/test/Driver, and make sure the flag works with --driver-mode=gcc and --driver-mode=cl. Follow the examples of the other tests, run clang with -###, and make sure this flag does or does not appear on the -cc1 line.

clang/lib/Driver/ToolChains/Clang.cpp

1217

This should probably be hasFlag(fpch_ins..., fno_pch_inst..., true) so that -fno-pch-inst -fpch-inst... works.

[shivanshu3]

In D88680#2307564, @rnk wrote:

I think the flag was originally intended to be an internal -cc1 flag not exposed to users. You should be able to work around your problem with -Xclang -fno-pch-instantiate-templates, btw.

---

@zequanwu, can you patch this in locally and take over this patch? Please address the hasFlag comment above, add a test to clang/test/Driver, and make sure the flag works with --driver-mode=gcc and --driver-mode=cl. Follow the examples of the other tests, run clang with -###, and make sure this flag does or does not appear on the -cc1 line.

@rnk Should we keep this flag internal though, or is it OK to make it a core flag? Our codebase had hundreds of compile errors caused by this default behavior which was hard to track down, so I'm guessing other people compiling with clang-cl might face similar problems? Or maybe we should re-think about making this the default behavior for clang-cl. Personally I don't have any strong preferences because we have a workaround now.

[llunak]

I tried pretty hard to get a small repro for these failures, but couldn't.

Can you get at least some testcase, even if not small? You can use -E -frewrite-includes to create a single large file from all the input. Although the patch looks fine to me as such, I consider it a workaround. The reason for -fpch-instantiate-templates being the default for clang-cl is that MSVC does some kind of template instantiation for PCHs too (although seeing your email you probably know more than I do). So preferably the core problem should be fixed.

In D88680#2307564, @rnk wrote:

I think the flag was originally intended to be an internal -cc1 flag not exposed to users.

No, it's intentionally exposed in the gcc-compatible driver, because there the option is not the default because of corner cases if the PCH is not self-contained. But since MSVC requires PCHs to be self-contained, it was deemed safe to always enable it in clang-cl. And, as said above, I still think it generally should.

[shivanshu3]

Can you get at least some testcase, even if not small?

Yes, in a few days I can get you the test case. I'll have to figure out what our company policy is for sharing code and if I should obfuscate it so I don't get into trouble :)

MSVC does some kind of template instantiation for PCHs too

MSVC's cl.exe compiles the precomp header with an empty .cpp file, which is where the template instantiations occur, and which is also the reason why the precomp header has to be self contained. So yes, in principle your change should be safe when enabled by default for clang-cl but there is a bug somewhere which we should figure out.

@llunak

[shivanshu3]

Also, I wanted to mention that clang-cl.exe actually crashes deterministically when running precompiles for 2 of our headers when using '-fpch-instantiate-templates' with the following signature:

1.      <eof> parser at end of file
2.      Per-file LLVM IR generation
3.      {msvc}\lib\native\include\xmemory:807:41: Generating code for declaration 'std::allocator<std::_Container_proxy>::allocate'
4.      {msvc}\lib\native\include\xmemory:44:30: LLVM IR generation of declaration 'std::_New_alignof'

Once I'm able to get a smaller repro which I can share, I will update you guys with it. @llunak FYI

[zahen]

This patch doesn't need a test case outside of the one that @rnk requested to make sure that the flag flows from the clang-cl driver appropriately. pch-instantiate-templates as authored doesn't match cl.exe behavior and being unable to turn it off will prevent our adoption of clang 11. This is a release blocking issue at least for 11.1 if not 11.0 (I leave it to @hans to make the decision). I suspect there are other users that will run into the same problem we have.

Of course we'll continue to work on a reduced repro to understand where the feature and msvc diverge but that will be a follow up patch or new bug.

[hans]

In D88680#2308601, @zahen wrote:

This patch doesn't need a test case outside of the one that @rnk requested to make sure that the flag flows from the clang-cl driver appropriately. pch-instantiate-templates as authored doesn't match cl.exe behavior and being unable to turn it off will prevent our adoption of clang 11. This is a release blocking issue at least for 11.1 if not 11.0 (I leave it to @hans to make the decision). I suspect there are other users that will run into the same problem we have.

Of course we'll continue to work on a reduced repro to understand where the feature and msvc diverge but that will be a follow up patch or new bug.

It's very late in the process for 11, but since this is just a flags change which seems safe, I'll consider it if we'll do another release candidate. Of course the patch has to land first :)

[shivanshu3]

Address Reid's comments:

• Use hasFlag instead of hasArg
• Add test cases

[llunak]

LGTM, although I'm not sure if me saying that is enough. I'll commit this in few days if nobody says anything else.

[hans]

clang/test/Driver/pch-instantiate-templates.c

3

Please always put "--" before the %s argument to avoid the compiler possibly misinterpreting the filename as a command-line flag (see comment at the top of clang/test/Driver/cl-options.c). This applies to all cl-mode clang invocations.

4

This "%clang -### --driver-mode=cl" invocation seems redundant with the %clang_cl test above. I'd suggest dropping it.

[shivanshu3]

Address Hans' comments

• Prepend %s with "--" for clang-cl
• Remove a redundant test case

[hans]

I've committed this as 66e4f07198761bbb4dcd55235024c1081ed15c75 so it has a chance to make it into the 11.0.0 release.

[hans]

In D88680#2314415, @hans wrote:

I've committed this as 66e4f07198761bbb4dcd55235024c1081ed15c75 so it has a chance to make it into the 11.0.0 release.

Pushed to 11.x as e84852be644d34867a604997fd328bf411b1977d

clang/include/clang/Driver/Options.td

def fpch_validate_input_files_content:
   Flag <["-"], "fpch-validate-input-files-content">,
   Group<f_Group>, Flags<[DriverOption]>,
   HelpText<"Validate PCH input files based on content if mtime differs">;
 def fno_pch_validate_input_files_content:
   Flag <["-"], "fno_pch-validate-input-files-content">,
   Group<f_Group>, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
-  Group<f_Group>, Flags<[CC1Option]>,
+  Group<f_Group>, Flags<[CC1Option, CoreOption]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
-  Group<f_Group>, Flags<[CC1Option]>;
+  Group<f_Group>, Flags<[CC1Option, CoreOption]>;
 defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
   "code for uses of this PCH that assumes an explicit object file will be built for the PCH">;
 defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate ",
   "debug info for types in an object file built from this PCH and do not generate them elsewhere">;
 
 def fmodules : Flag <["-"], "fmodules">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,
   HelpText<"Enable the 'modules' language feature">;

clang/lib/Driver/ToolChains/Clang.cpp

void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   // .gch files.
 
   if (getToolChain().getDriver().IsCLMode()) {
     const Arg *YcArg = Args.getLastArg(options::OPT__SLASH_Yc);
     const Arg *YuArg = Args.getLastArg(options::OPT__SLASH_Yu);
     if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
         JA.getKind() <= Action::AssembleJobClass) {
       CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
+      // -fpch-instantiate-templates is the default when creating
+      // precomp using /Yc
+      if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+                       options::OPT_fno_pch_instantiate_templates, true))
         CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
     }
     if (YcArg || YuArg) {
       StringRef ThroughHeader = YcArg ? YcArg->getValue() : YuArg->getValue();
       if (!isa<PrecompileJobAction>(JA)) {
         CmdArgs.push_back("-include-pch");
         CmdArgs.push_back(Args.MakeArgString(D.GetClPchPath(
             C, !ThroughHeader.empty()
                    ? ThroughHeader

clang/test/Driver/pch-instantiate-templates.c

+// CL driver test cases
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -- %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC %s
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -fno-pch-instantiate-templates -- %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC_DISABLE %s
+
+// CLANG_CL_YC: "-fpch-instantiate-templates"
+// CLANG_CL_YC_DISABLE-NOT: "-fpch-instantiate-templates"
+
+// GCC driver test cases
+// RUN: %clang -### -x c-header %s -o %t/foo.pch 2>&1 | FileCheck -check-prefix=GCC_DEFAULT %s
+// RUN: %clang -### -x c-header %s -o %t/foo.pch -fpch-instantiate-templates 2>&1 | FileCheck -check-prefix=GCC_DEFAULT_ENABLE %s
+
+// GCC_DEFAULT-NOT: "-fpch-instantiate-templates"
+// GCC_DEFAULT_ENABLE: "-fpch-instantiate-templates"