[Preamble] Stop circular inclusion of main file when building preamble

[nik]

Summary

If a header file was processed for the second time, we could end up with
a wrong conditional stack and skipped ranges:

In the particular example, if the header guard is evaluated the second
time and it is decided to skip the conditional block, the corresponding
"#endif" is never seen since the preamble does not include it and we end
up in the Tok.is(tok::eof) case with a wrong conditional stack.

Detect the circular inclusion, emit a diagnostic and stop processing the
inclusion.

[ilya-biryukov]

Why does resetting the conditional stack is the right thing to do here?
I can see how it can hide the problem, but can't come up with with a consistent model to handle the fact that the file contents were trimmed.

Failing to build the preamble in that case seems like the best thing we could do. WDYT?

[nik]

In D53866#1281978, @ilya-biryukov wrote:

Why does resetting the conditional stack is the right thing to do here?

Because this case can be detected and handled without loosing the benefits of the preamble.

Failing to build the preamble in that case seems like the best thing we could do. WDYT?

See above - we would not have the benefits of the preamble anymore.

I can see how it can hide the problem, but can't come up with with a consistent model to handle the fact that the file contents were trimmed.

If we are in preamble-generation-mode and the main file #includes itself, then it should see the full file content of the file, not the truncated preamble version of it.
Would this be more consistent?

[ilya-biryukov]

Because this case can be detected and handled without loosing the benefits of the preamble.

The cases where recursive includes make sense are incredibly rare in practice. Most of the time, recursively including the same file is unintentional should be considered a bug and fixed.
Producing a warning that the file is being recursively included (along with an include chain that caused a recursive inclusion) seems to provide more benefit than preamble, especially when this is unintentional (I would argue that's the majority of the cases).

If we are in preamble-generation-mode and the main file #includes itself, then it should see the full file content of the file, not the truncated preamble version of it.
Would this be more consistent?

Yes, but I can't see an easy way to make it happen. And I don't think we want a complex fix for something bizzare that very rarely happens in practice.

[yvvan]

@ilya-biryukov
As far as I understand the problem the same thing happens when you are in the header a.h which includes b.h and b.h includes a.h at the same time. So you get this recursion indirectly and very often because that's why include guards are there.

[ilya-biryukov]

In D53866#1282582, @yvvan wrote:

@ilya-biryukov
As far as I understand the problem the same thing happens when you are in the header a.h which includes b.h and b.h includes a.h at the same time. So you get this recursion indirectly and very often because that's why include guards are there.

Cycles in include graphs are almost always unintentional and must be broken apart, there are very few cases where you actually want this.
E.g. in your example, since 'b.h' includes 'a.h' it needs something from 'a.h'. But if you include 'b.h' while processing the preamble of 'a.h', the declarations inside 'a.h' are not yet visible and that would result in (at times obscure and hard to read) compiler errors when parsing 'b.h'.
(Assuming a.h and b.h are normal headers and the use-case does not involve doing various preprocessor tricks)

In those cases it usually works by accident (e.g. when 'b.h' included 'a.h', but never actually used anything declared in 'a.h').

[nik]

I've only the minimal example at hand right know - I'm waiting for feedback about the real world case.

[nik]

I still don't have feedback for a real world case except "unintentional #include". Unfortunately, in real world cases the cyclic include might be not obvious at all.

@ilya: As far as I understand you prefer to make the preamble generation rather fail as long as we don't have more information. How do you suggest to implement this? I see that Clang->getPreprocessor().addPPCallbacks() is called in PrecompiledPreamble::Build(). Adding a custom PPCallbacks there that overrides "void InclusionDirective()" might be enough to detect the cyclic #include and to set a flag that is checked before PrecompiledPreamble::Build() returns - BuildPreambleError could get a new enumerator. Is there a better way to do this? For example, it would be desirable to not only observe this case, but then to also stop/abort/skip all the further parsing that is done for the preamble only as it's pointless then.

[ilya-biryukov]

In D53866#1301086, @nik wrote:

I still don't have feedback for a real world case except "unintentional #include". Unfortunately, in real world cases the cyclic include might be not obvious at all.
@ilya: As far as I understand you prefer to make the preamble generation rather fail as long as we don't have more information. How do you suggest to implement this? I see that Clang->getPreprocessor().addPPCallbacks() is called in PrecompiledPreamble::Build(). Adding a custom PPCallbacks there that overrides "void InclusionDirective()" might be enough to detect the cyclic #include and to set a flag that is checked before PrecompiledPreamble::Build() returns - BuildPreambleError could get a new enumerator. Is there a better way to do this? For example, it would be desirable to not only observe this case, but then to also stop/abort/skip all the further parsing that is done for the preamble only as it's pointless then.

Maybe produce a fatal error in the preprocessor? That seems to be the simplest option: the preprocessor is aware it's building the preamble and there's definitely some logic to produce fatal errors in other cases (include not found).
A fatal error currently aborts other stages of the compilation pipeline and producing one would make the run of the compiler that tries to produce the preamble fail, giving the empty preamble as a result.

[nik]

Maybe produce a fatal error in the preprocessor? That seems to be the simplest option: the preprocessor is aware it's building the preamble and there's definitely some logic to produce fatal errors in other cases (include not found).
A fatal error currently aborts other stages of the compilation pipeline and producing one would make the run of the compiler that tries to produce the preamble fail, giving the empty preamble as a result.

Done. I'm using diag::err_pp_error_opening_file as introducing an new artificial diagnostic error that the user will never see feels wrong.

Note that a preamble is generated for fatal errors like an unresolved #include and I think that's fine. As such, I need a way to propagate the error up to PrecompilePreambleConsumer to avoid writing the preamble. I've done that with an extra flag in Preprocessor. An alternative might be to put it into PreambleConditionalStackStore (as state? and rename that class to something more general?) - what do you think?

[nik]

Ping.

[ilya-biryukov]

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

There's a mechanism to handle preamble with errors, see PreprocessorOpts::AllowPCHWithCompilerErrors.
Maybe rely on it and avoid adding a different one?

lib/Lex/PPDirectives.cpp

1883

Maybe add a new error or find a more appropriate existing one to reuse?
"Error opening file", especially without any arguments does not provide enough context to figure out what went wrong.

[nik]

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

I'm not sure how to make use of AllowPCHWithCompilerErrors. It's clearly meant as an input/readonly option - do you suggest setting that one to "false" in case we detect the cyclic include (and later checking for it...)? That feels a bit hacky. Have you meant something else?

lib/Lex/PPDirectives.cpp

1883

Maybe add a new error or find a more appropriate existing one to reuse?

As stated above, introducing a new diagnostic that the user will never face feels wrong, but that's just a preference. If you prefer a dedicated diagnostics, that's fine for me.

Checking the existing ones, there are not many fatal errors to choose from:

err_pp_file_not_found
err_pp_through_header_not_found
err_pp_through_header_not_seen
err_pp_pragma_hdrstop_not_seen
err_pp_error_opening_file

The last one looks the best for me.

"Error opening file", especially without any arguments does not provide enough context to figure out what went wrong.

I've added some arguments which might be useful for debugging.

[nik]

Addressed comments.

[ilya-biryukov]

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

We emit an error, so the preamble will not be emitted.
Unless the users specify AllowPCHWithCompilerErrors, in which case they've signed up for this anyway.

I propose to not add the new flag at all. Would that work?

lib/Lex/PPDirectives.cpp

1883

Could we create a new error for this case?

1884

I thought the convention is to not capitalize the first word of a message, otherwise it would be a bit inconsistent with other errors:

cannot open file 'foo.cpp:  Main file can't include itself for preamble

also a small nit: maybe rephrase to main file cannot be included recursively for preamble
and another small nit: we probably want to remove the full stop at the end.

[nik]

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

As stated further above, no.

That's because for the libclang/c-index-test case, AllowPCHWithCompilerErrors=true - see clang_parseTranslationUnit_Impl. As such, the preamble will be generated/emitted as the second early return in PCHGenerator::HandleTranslationUnit is never hit.

[nik]

Added a dedicated diagnostic.

[ilya-biryukov]

include/clang/Basic/DiagnosticLexKinds.td

430

NIT: maybe rephrase for preamble to when building a preamble? "for preamble" might be a bit confusing.

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

if AllowPCHWithCompilerErrors=true is set to true, why not simply pretend the include was not found? That would still generate the preamble, but users have the error message to see that something went wrong.

c-index-test produces the errors, so we can check the error is present in the output.

[nik]

include/clang/Lex/Preprocessor.h

391 ↗

(On Diff #175210)

if AllowPCHWithCompilerErrors=true is set to true, why not simply pretend the include was not found?

Sounds good, especially that the preamble is still generated.

That would still generate the preamble, but users have the error message to see that something went wrong.

The vanilla diagnostic in this case might be a bit confusing, I kept the introduced diagnostics as it carries more information.

[nik]

Addressed comments.

[nik]

lib/Basic/SourceManager.cpp

1594

I've added the nullptr check here as LibclangReparseTest.ReparseWithModule fails/crashes otherwise.

[nik]

Ping.

[nik]

Ping.

[nik]

Ping. Ilya?

[nik]

Rebased for current trunk.

If I miss something obvious, please tell me. Otherwise I'm waiting.

[ilya-biryukov]

Sorry for losing this.
Neat change, minimal and focused, thanks!

Just wanted to clarify why we need the change in SourceManager.cpp, will LGTM as soon as we resolve this

lib/Basic/SourceManager.cpp

1594

Can this actually be`null`?

[nik]

lib/Basic/SourceManager.cpp

1594

The comment for OrigEntry states that it might be null:

/// Reference to the file entry representing this ContentCache.
///
/// This reference does not own the FileEntry object.
///
/// It is possible for this to be NULL if the ContentCache encapsulates
/// an imaginary text buffer.
const FileEntry *OrigEntry;

Note also that further down in this function, a null check for MainContentCache->OrigEntry is done, so I believe that this was just forgotten here (since there is also no assert) and we are the first one running into this with the introduced call to SourceMgr.translateFile(File).

I've tried to understand why we end up with a nullptr here, but this goes beyond me in the time I've taken for this. What I've observed is that module code path (LibclangReparseTest.ReparseWithModule vs LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary text buffer" from the comment) and I suspect this to be somehow related with the OrigEntry nullptr.

[nik]

Moved the MainFile / MainContentCache->OrigEntry check a bit further up, for
consistency with the same test further down in SourceManager::translateFile().

[ilya-biryukov]

lib/Basic/SourceManager.cpp

1594

Should we check for equality of MainContentCache->ContentsEntry in that case? I guess this is what should be set for "imaginary" files.

[nik]

lib/Basic/SourceManager.cpp

1594

Does not help in this particular case as ContentsEntry is also a nullptr. All the members of the ContentsCache seem to be either nullptr or 0 as that particular point of execution.

How to continue?

a) Accept as is.
b) Ask someone knowledgeable for whom this could be a no-brainer that could respond hopefully soon. Any candidate?
c) Or should I dig deeper? This can take quite some time as this area is new for me.

[ilya-biryukov]

LGTM. See the nit about naming of an error, though

include/clang/Basic/DiagnosticLexKinds.td

429

NIT: Maybe change to err_pp_including_mainfile_in_preamble?

lib/Basic/SourceManager.cpp

1594

Wow, I guess I don't understand this code deep enough.
Your change looks ok, I'm happy to LGTM this. But if you're actually interested in digging deeper, that would be super-useful.

Based on what you're describe, it feels like "imaginary" main file works by accident rather than being properly handled in this function.

BTW, thanks for moving the check up, it definitely looks nicer this way.

[nik]

Renamed to err_pp_including_mainfile_in_preamble.

[nik]

Huch, I forgot to add "Differential Revision: <URL>" to the commit message, so I'll close this manually once I know how to add the svn revision number to this. https://llvm.org/docs/Phabricator.html states:

In the web UI, under “Leap Into Action” put the SVN revision number in the Comment, set the Action to “Close Revision” and click Submit.

Where is the "Lean Into Action" thingy? Can't find it.

[ilya-biryukov]

I guess using Edit Related Object -> Edit Commits should do the trick.
I'm not sure what the "Lean Into Action" is either.

[jan-wassenberg]

@nik here's an unusual but real-world example that triggers this.
https://github.com/google/highway compiles the same source multiple times (with different macros set) for generating code for multiple SIMD instruction sets.
The main source file sets a macro to its filename and includes a "foreach_target.h" which includes the file identified by that macro.

I understand this is complicated for analysis, so foreach_target.h does nothing if it detects that an IDE/analyzer are parsing. This relies on predefined macros such as
CDT_PARSER INTELLISENSE Q_CREATOR_RUN. Is there such a macro for clangd?

include/clang/Basic/DiagnosticLexKinds.td

def err_pp_file_not_found_angled_include_not_fatal : Error<
   "'%0' file not found with <angled> %select{include|import}1; "
   "use \"quotes\" instead">;
 def err_pp_file_not_found_typo_not_fatal
     : Error<"'%0' file not found, did you mean '%1'?">;
 def note_pp_framework_without_header : Note<
   "did not find header '%0' in framework '%1' (loaded from '%2')">;
 def err_pp_error_opening_file : Error<
   "error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_in_preamble : Error<
+  "main file cannot be included recursively when building a preamble">;
 def err_pp_empty_filename : Error<"empty filename">;
 def err_pp_include_too_deep : Error<"#include nested too deeply">;
 def err_pp_expects_filename : Error<"expected \"FILENAME\" or <FILENAME>">;
 def err_pp_macro_not_identifier : Error<"macro name must be an identifier">;
 def err_pp_missing_macro_name : Error<"macro name missing">;
 def err_pp_missing_rparen_in_macro_def : Error<
   "missing ')' in macro parameter list">;
 def err_pp_invalid_tok_in_arg_list : Error<

lib/Basic/SourceManager.cpp

if (MainFileID.isValid()) {
     bool Invalid = false;
     const SLocEntry &MainSLoc = getSLocEntry(MainFileID, &Invalid);
     if (Invalid)
       return FileID();
 
     if (MainSLoc.isFile()) {
       const ContentCache *MainContentCache
         = MainSLoc.getFile().getContentCache();
-      if (!MainContentCache) {
+      if (!MainContentCache || !MainContentCache->OrigEntry) {
         // Can't do anything
       } else if (MainContentCache->OrigEntry == SourceFile) {
         FirstFID = MainFileID;
       } else {
         // Fall back: check whether we have the same base name and inode
         // as the main file.
         const FileEntry *MainFile = MainContentCache->OrigEntry;
         SourceFileName = llvm::sys::path::filename(SourceFile->getName());
         if (*SourceFileName == llvm::sys::path::filename(MainFile->getName())) {
           SourceFileUID = getActualFileUID(SourceFile);
           if (SourceFileUID) {
             if (Optional<llvm::sys::fs::UniqueID> MainFileUID =
                     getActualFileUID(MainFile)) {
               if (*SourceFileUID == *MainFileUID) {
                 FirstFID = MainFileID;
                 SourceFile = MainFile;
               }

lib/Lex/PPDirectives.cpp

#endif
   }
 
   if (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {
     if (isPCHThroughHeader(File))
       SkippingUntilPCHThroughHeader = false;
     return {ImportAction::None};
   }
 
+  // Check for circular inclusion of the main file.
+  // We can't generate a consistent preamble with regard to the conditional
+  // stack if the main file is included again as due to the preamble bounds
+  // some directives (e.g. #endif of a header guard) will never be seen.
+  // Since this will lead to confusing errors, avoid the inclusion.
+  if (File && PreambleConditionalStack.isRecording() &&
+      SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+    Diag(FilenameTok.getLocation(),
+         diag::err_pp_including_mainfile_in_preamble);
+    return {ImportAction::None};
+  }
+
   // Should we enter the source file? Set to Skip if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined, set to Import if it
   // is a modular header we've already built and should import.
   enum { Enter, Import, Skip, IncludeLimitReached } Action = Enter;
 
   if (PPOpts->SingleFileParseMode)
     Action = IncludeLimitReached;

test/Index/preamble-cyclic-include.cpp

+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:5:1:10:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+// CHECK: error: main file cannot be included recursively when building a preamble
+#ifndef A_H
+#define A_H
+#  include "preamble-cyclic-include.cpp"
+int bar();
+#endif