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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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:
> > 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866