[Preprocessor] Stop entering included files after hitting a fatal error.

[vsapsai]

Summary

Fixes a problem when we have multiple inclusion cycles and try to
enumerate all possible ways to reach the max inclusion depth.

rdar://problem/38871876

[vsapsai]

Ping.

[vsapsai]

Ping.

[jkorous]

clang/lib/Lex/PPDirectives.cpp

1875

I am not sure I understand this - does that mean that we are not displaying diagnostics that were produced before "now"?

[vsapsai]

clang/lib/Lex/PPDirectives.cpp

1875

I can see how the wording can cause the confusion. But I'm not entirely sure it is misguiding, I've copy-pasted it from Sema::InstantiatingTemplate::InstantiatingTemplate. Let me explain my reasoning in a different way to see if it makes sense. Entering a file is observable if it produces diagnostics or some other build artifact (object file in most cases). So when we encounter a fatal error, there is no visible indication of entering subsequent files. That's why we can skip entering those files: no difference in output and less work to do.

[jkorous]

LGTM with just a nit about comment wording.

Thanks for the patch!

clang/lib/Lex/PPDirectives.cpp

1875

Thanks for the explanation! I was not sure whether "only subsequent" OR "both subsequent and some/all prior" raised diagnostics would be not visible (could be just my shitty English though).

Maybe something along this line "any eventual subsequent diagnostics will not be visible" would be more clear?

[aaron.ballman]

LGTM as well

[bruno]

Thanks for working on this.

LGTM with improvements in the comments as mentioned by @jkorous

[vsapsai]

clang/lib/Lex/PPDirectives.cpp

1875

Curiously, the comment kinda means both. Some prior diagnostics might be invisible but in this case we care about subsequent diagnostics. I'll update the comment to make that more clear.

[vsapsai]

• Tweak the comment according to review comments.

Undiscussed changes: we don't stop preprocessing entirely, only in included
files; not using better-sounding "skip" deliberately as it might be confused
with FileSkipped API.

[vsapsai]

Thanks everyone for reviewing the change.

clang/lib/Lex/PPDirectives.cpp

#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())
+    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,
                            getLangOpts().ModuleName)) {
     // If this include corresponds to a module but that module is

clang/test/Preprocessor/Inputs/cycle/a.h

+// Presence of 2 inclusion cycles
+//    b.h -> a.h -> b.h -> ...
+//    c.h -> a.h -> c.h -> ...
+// makes it unfeasible to reach max inclusion depth in all possible ways. Need
+// to stop earlier.
+
+#include "b.h"
+#include "c.h"

clang/test/Preprocessor/Inputs/cycle/b.h

+#include "a.h"

clang/test/Preprocessor/Inputs/cycle/c.h

+#include "a.h"

clang/test/Preprocessor/include-cycle.c

+// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s
+
+// Test that preprocessing terminates even if we have inclusion cycles.
+
+#include "cycle/a.h"