[Preprocessor] Don't avoid entering included files after hitting a fatal error.

[vsapsai]

Summary

Change in r337953 violated the contract for CXTranslationUnit_KeepGoing:

Do not stop processing when fatal errors are encountered.

Use different approach to fix long processing times with multiple
inclusion cycles. Instead of stopping preprocessing for fatal errors, do
this after heuristically detecting an inclusion cycle.

rdar://problem/46108547

[erik.pilkington]

clang/lib/Lex/PPDirectives.cpp

1804–1812

Why not just always set HasIncludeCycle = true; if we've reached the max allowed include stack depth? Seems like that would always be due to recursive includes. But even if it wasn't caused by recursive includes, we still probably shouldn't descend into more included files anyway, right?

1950

Nit: may as well just make this if (HasIncludeCycle) now that we don't have to call the function.

[vsapsai]

clang/lib/Lex/PPDirectives.cpp

1804–1812

I wanted to support a use case like

#include "root-of-deep-include-chain.h"
#include "foo.h"

and still enter "foo.h". Setting HasIncludeCycle = true; without cycle checking will prevent it.

After more thinking now I want to support a use case like

#include "cycle.h"
#include "foo.h"

It seems in the spirit of CXTranslationUnit_KeepGoing to process "foo.h" despite a single bad include "cycle.h". And I think it is not too far-fetched to create cycles accidentally during code editing. If tools fail at such moments, it creates bad user experience and tools support is especially useful during erroneous situations.

I have an idea how to implement better cycle-checking (basically llvm::DenseSet<const FileEntry *> FilesCausingCycles), want to try and see if it's not too complicated.

1950

Good point.

[vsapsai]

• Keep indexing after include cycle.

That's the approach I had in mind when I was writing about it in code review.
But it turned out to be not my preferred solution. I'll post a different patch
shortly.

[vsapsai]

• Different approach: don't enter already processed files after reaching max include depth.

[erik.pilkington]

Nice, LGTM!

[vsapsai]

• Remove now unnecessary #include "llvm/ADT/DenseSet.h"

[vsapsai]

Thanks for the review, Erik.

clang/include/clang/Lex/Preprocessor.h

class Preprocessor {
 
   /// The number of bytes that we will initially skip when entering the
   /// main file, along with a flag that indicates whether skipping this number
   /// of bytes will place the lexer at the start of a line.
   ///
   /// This is used when loading a precompiled preamble.
   std::pair<int, bool> SkipMainFilePreamble;
 
+  /// Whether we hit an error due to reaching max allowed include depth. Allows
+  /// to avoid hitting the same error over and over again.
+  bool HasReachedMaxIncludeDepth = false;
+
 public:
   struct PreambleSkipInfo {
     SourceLocation HashTokenLoc;
     SourceLocation IfTokenLoc;
     bool FoundNonSkipPortion;
     bool FoundElse;
     SourceLocation ElseLoc;
 

clang/lib/Lex/PPDirectives.cpp

 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Pragma.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/Token.h"
 #include "clang/Lex/VariadicMacroSupport.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc,
   // we allow macros that expand to nothing after the filename, because this
   // falls into the category of "#include pp-tokens new-line" specified in
   // C99 6.10.2p4.
   CheckEndOfDirective(IncludeTok.getIdentifierInfo()->getNameStart(), true);
 
   // Check that we don't have infinite #include recursion.
   if (IncludeMacroStack.size() == MaxAllowedIncludeStackDepth-1) {
     Diag(FilenameTok, diag::err_pp_include_too_deep);
+    HasReachedMaxIncludeDepth = true;
     return;
   }
 
   // Complain about attempts to #include files in an audit pragma.
   if (PragmaARCCFCodeAuditedLoc.isValid()) {
     Diag(HashLoc, diag::err_pp_include_in_arc_cf_code_audited);
     Diag(PragmaARCCFCodeAuditedLoc, diag::note_pragma_entered_here);
 
     // Immediately leave the pragma.
     PragmaARCCFCodeAuditedLoc = SourceLocation();
   }
 
   // Complain about attempts to #include files in an assume-nonnull pragma.
   if (PragmaAssumeNonNullLoc.isValid()) {
     Diag(HashLoc, diag::err_pp_include_in_assume_nonnull);
     Diag(PragmaAssumeNonNullLoc, diag::note_pragma_entered_here);
 
     // Immediately leave the pragma.
     PragmaAssumeNonNullLoc = SourceLocation();
   }
 
   if (HeaderInfo.HasIncludeAliasMap()) {
#endif
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined or is a modular header
   // we've imported or already built.
   bool ShouldEnter = true;
 
   if (PPOpts->SingleFileParseMode)
     ShouldEnter = false;
 
-  // Any diagnostics after the fatal error will not be visible. As the
-  // compilation failed already and errors in subsequently included files won't
-  // be visible, avoid preprocessing those files.
-  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+  // If we've reached the max allowed include depth, it is usually due to an
+  // include cycle. Don't enter already processed files again as it can lead to
+  // reaching the max allowed include depth again.
+  if (ShouldEnter && HasReachedMaxIncludeDepth && File &&
+      HeaderInfo.getFileInfo(File).NumIncludes)
     ShouldEnter = false;
 
   // Determine whether we should try to import the module for this #include, if
   // there is one. Don't do so if precompiled module support is disabled or we
   // are processing this module textually (because we're building the module).
   if (ShouldEnter && File && SuggestedModule && getLangOpts().Modules &&
       !isForModuleBuilding(SuggestedModule.getModule(),
                            getLangOpts().CurrentModule,

clang/test/Index/Inputs/cycle.h

+#include "cycle.h"

clang/test/Index/keep-going-include-cycle.c

+#include "cycle.h"
+#include "foo.h"
+
+// RUN: env CINDEXTEST_KEEP_GOING=1 c-index-test -test-print-type -I%S/Inputs %s 2> %t.stderr.txt | FileCheck %s
+// RUN: FileCheck -check-prefix CHECK-DIAG %s < %t.stderr.txt
+
+// Verify that we don't stop preprocessing after an include cycle.
+// CHECK: VarDecl=global_var:1:12 [type=int] [typekind=Int] [isPOD=1]
+
+// CHECK-DIAG: cycle.h:1:10: error: #include nested too deeply

clang/test/Index/keep-going.cpp

 #include "missing1.h"
 
 template<class T>
 class A { T a; };
 
 class B : public A<int> { };
 
 #include "missing2.h"
 
 class C : public A<float> { };
 
-// RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_KEEP_GOING=1 c-index-test -test-print-type %s -std=c++03 2> %t.stderr.txt  | FileCheck %s
+// Not found includes shouldn't affect subsequent correct includes.
+#include "foo.h"
+
+// RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_KEEP_GOING=1 c-index-test -test-print-type -I%S/Inputs %s -std=c++03 2> %t.stderr.txt  | FileCheck %s
 // RUN: FileCheck -check-prefix CHECK-DIAG %s < %t.stderr.txt
 
+// Verify that even without CINDEXTEST_EDITING we don't stop processing after a fatal error.
+// RUN: env CINDEXTEST_KEEP_GOING=1 c-index-test -test-print-type -I%S/Inputs %s -std=c++03 2> %t.stderr.txt  | FileCheck -check-prefix CHECK-KEEP-GOING-ONLY %s
+
 // CHECK: inclusion directive=missing1.h ((null)) [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: inclusion directive=missing2.h ((null)) [type=] [typekind=Invalid] [isPOD=0]
+// CHECK: inclusion directive=foo.h ({{.*[/\\]}}test{{[/\\]}}Index{{[/\\]}}Inputs{{[/\\]}}foo.h) [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: ClassTemplate=A:4:7 (Definition) [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: TemplateTypeParameter=T:3:16 (Definition) [type=T] [typekind=Unexposed] [canonicaltype=type-parameter-0-0] [canonicaltypekind=Unexposed] [isPOD=0]
 // CHECK: FieldDecl=a:4:13 (Definition) [type=T] [typekind=Unexposed] [canonicaltype=type-parameter-0-0] [canonicaltypekind=Unexposed] [isPOD=0]
 // CHECK: TypeRef=T:3:16 [type=T] [typekind=Unexposed] [canonicaltype=type-parameter-0-0] [canonicaltypekind=Unexposed] [isPOD=0]
 // CHECK: ClassDecl=B:6:7 (Definition) [type=B] [typekind=Record] [isPOD=0]
 // CHECK: C++ base class specifier=A<int>:4:7 [access=public isVirtual=false] [type=A<int>] [typekind=Unexposed] [templateargs/1= [type=int] [typekind=Int]] [canonicaltype=A<int>] [canonicaltypekind=Record] [canonicaltemplateargs/1= [type=int] [typekind=Int]] [isPOD=0] [nbFields=1]
 // CHECK: TemplateRef=A:4:7 [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: ClassDecl=C:10:7 (Definition) [type=C] [typekind=Record] [isPOD=0]
 // CHECK: C++ base class specifier=A<float>:4:7 [access=public isVirtual=false] [type=A<float>] [typekind=Unexposed] [templateargs/1= [type=float] [typekind=Float]] [canonicaltype=A<float>] [canonicaltypekind=Record] [canonicaltemplateargs/1= [type=float] [typekind=Float]] [isPOD=0] [nbFields=1]
 // CHECK: TemplateRef=A:4:7 [type=] [typekind=Invalid] [isPOD=0]
 
+// CHECK-KEEP-GOING-ONLY: VarDecl=global_var:1:12 [type=int] [typekind=Int] [isPOD=1]
+
 // CHECK-DIAG: keep-going.cpp:1:10: fatal error: 'missing1.h' file not found
 // CHECK-DIAG: keep-going.cpp:8:10: fatal error: 'missing2.h' file not found