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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:429
   "error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+  "main file cannot be included recursively when building a preamble">;
----------------
NIT: Maybe change to err_pp_including_mainfile_**in**_preamble?



================
Comment at: lib/Basic/SourceManager.cpp:1594
         SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-        if (*SourceFileName == llvm::sys::path::filename(MainFile->getName())) {
+        if (MainFile && *SourceFileName == llvm::sys::path::filename(MainFile->getName())) {
           SourceFileUID = getActualFileUID(SourceFile);
----------------
nik wrote:
> ilya-biryukov wrote:
> > nik wrote:
> > > ilya-biryukov wrote:
> > > > Can this actually be`null`? 
> > > 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.
> > Should we check for equality of `MainContentCache->ContentsEntry` in that case? I guess this is what should be set for "imaginary" files.
> 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.
> 
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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53866/new/

https://reviews.llvm.org/D53866