This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shivanshu3 on Oct 1 2020, 10:41 AM.

Details

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.

Diff Detail

Event Timeline

shivanshu3 created this revision.Oct 1 2020, 10:41 AM
shivanshu3 requested review of this revision.Oct 1 2020, 10:41 AM

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 <shigoyal@microsoft.com>"

rnk added a subscriber: zequanwu.Oct 1 2020, 5:06 PM

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 added a comment.EditedOct 1 2020, 5:46 PM
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 added a comment.Oct 1 2020, 9:58 PM

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.

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

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 added a comment.Oct 2 2020, 7:31 AM

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 added a comment.Oct 5 2020, 4:34 AM

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 updated this revision to Diff 296340.Oct 5 2020, 6:38 PM

Address Reid's comments:

  • Use hasFlag instead of hasArg
  • Add test cases
shivanshu3 marked an inline comment as done.Oct 5 2020, 6:39 PM

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 added inline comments.Oct 6 2020, 2:27 AM
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 updated this revision to Diff 296393.Oct 6 2020, 2:40 AM

Address Hans' comments

  • Prepend %s with "--" for clang-cl
  • Remove a redundant test case
shivanshu3 marked 2 inline comments as done.Oct 6 2020, 2:40 AM
hans accepted this revision.Oct 6 2020, 2:48 AM
This revision is now accepted and ready to land.Oct 6 2020, 2:48 AM
This revision was landed with ongoing or failed builds.Oct 6 2020, 7:23 AM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Oct 6 2020, 7:24 AM

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

hans added a comment.Oct 7 2020, 3:08 AM

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