Handle delayed-template-parsing functions imported into a non-dtp TU

[sammccall]

Summary

DelayedTemplateParsing is marked as BENIGN_LANGOPT, so we are allowed to
use a delayed template in a non-delayed TU.
(This is clangd's default configuration on windows: delayed-template-parsing
is on for the preamble and forced off for the current file)

However today clang fails to parse implicit instantiations in a non-dtp
TU of templates defined in a dtp PCH file (and presumably module?).
In this case the delayed parser is not registered, so the function is
simply marked "delayed" again. We then hit an assert:
end of TU template instantiation should not create more late-parsed templates

[kadircet]

Looking at this some more, it looks like whenever clang is building a TU prefix(preamble), the assumption is that any delayed templates will be parsed at the end of the TU. That is unfortunately not true, if the rest of the TU is built with no-delayed-template-parsing. (as you've already said in the description)

This suggests that DelayedTemplateParsing shouldn't be a benign langopt. But since it is, we should be setting late template parser at the end of TU if there are any delayed templates coming from either the preamble or in the rest of the TU.
The latter is already done by checking for langopt, we can check for the former using Actions.ExternalSemaSource. But in the absence of delayed templates, setting the parser is (currently) a no-op. So setting it all the time should hopefully be OK.
Hence LGTM.

Another option would be to change preamble building logic to parse all the late parsed templates at the end, instead of just serializing the tokens. That sounds like a more intrusive change though, so I am more comfortable with current one.

[sammccall]

Thanks for reviewing! Going to land this now as the bug is surfaced by other pending changes.

In D81474#2087095, @kadircet wrote:

Looking at this some more, it looks like whenever clang is building a TU prefix(preamble), the assumption is that any delayed templates will be parsed at the end of the TU. That is unfortunately not true, if the rest of the TU is built with no-delayed-template-parsing. (as you've already said in the description)

This suggests that DelayedTemplateParsing shouldn't be a benign langopt. But since it is, we should be setting late template parser at the end of TU if there are any delayed templates coming from either the preamble or in the rest of the TU.
The latter is already done by checking for langopt, we can check for the former using Actions.ExternalSemaSource. But in the absence of delayed templates, setting the parser is (currently) a no-op. So setting it all the time should hopefully be OK.
Hence LGTM.

Yeah I think checking ExternalSemaSource is probably correct. Not checking it is simpler though :-)

Another option would be to change preamble building logic to parse all the late parsed templates at the end, instead of just serializing the tokens. That sounds like a more intrusive change though, so I am more comfortable with current one.

This has different semantics - the point of delayed template parsing is to wait for more symbols to be visible. If we parse at the end of the PCH, then symbols defined in the main file (but before instantiation) are not visible.

clang/lib/Parse/Parser.cpp

if (PP.getMaxTokens() != 0 && PP.getTokenCount() > PP.getMaxTokens()) {
           << PP.getTokenCount() << PP.getMaxTokens();
       SourceLocation OverrideLoc = PP.getMaxTokensOverrideLoc();
       if (OverrideLoc.isValid()) {
         PP.Diag(OverrideLoc, diag::note_max_tokens_total_override);
       }
     }
 
     // Late template parsing can begin.
-    if (getLangOpts().DelayedTemplateParsing)
-      Actions.SetLateTemplateParser(LateTemplateParserCallback, nullptr,
-                                    this);
+    Actions.SetLateTemplateParser(LateTemplateParserCallback, nullptr, this);
     if (!PP.isIncrementalProcessingEnabled())
       Actions.ActOnEndOfTranslationUnit();
     //else don't tell Sema that we ended parsing: more input might come.
     return true;
 
   case tok::identifier:
     // C++2a [basic.link]p3:
     //   A token sequence beginning with 'export[opt] module' or

clang/test/PCH/delayed-template-parsing.cpp

+// Check any combination of delayed-template-parsing between PCH and TU works.
+// RUN: %clang_cc1 %s -emit-pch -o %t.pch
+// RUN: %clang_cc1 -fdelayed-template-parsing %s -emit-pch -o %t.delayed.pch
+// RUN: %clang_cc1 -DMAIN_FILE -include-pch %t.pch %s
+// RUN: %clang_cc1 -DMAIN_FILE -fdelayed-template-parsing -include-pch %t.pch %s
+// RUN: %clang_cc1 -DMAIN_FILE -include-pch %t.delayed.pch %s
+// RUN: %clang_cc1 -DMAIN_FILE -fdelayed-template-parsing -include-pch %t.delayed.pch %s
+
+#ifndef MAIN_FILE
+template <typename T>
+T successor(T Value) { return Value + 1; }
+#else
+int x = successor(42);
+#endif