ASTReader: Bypass overridden files when reading PCHs

[dexonsmith]

Summary

If contents of a file that is part of a PCM are overridden when reading
it, but weren't overridden when the PCM was being built, the ASTReader
will emit an error. Now it creates a separate FileEntry for recovery,
bypassing the overridden content instead of discarding it. The
pre-existing testcase clang/test/PCH/remap-file-from-pch.cpp confirms
that the new recovery method works correctly.

This resolves a long-standing FIXME to avoid hypothetically invalidating
another precompiled module that's already using the overridden contents.

This also removes ContentCache-related API that would be unsafe to use
across CompilerInstances in an implicit modules build. This helps to
unblock us sinking it from SourceManager into FileManager in the future,
which would allow us to delete InMemoryModuleCache.

[arphaman]

Missing full context

[dexonsmith]

New diff with full context.

[bruno]

Nice! Is this something that can be tested for in unittests/Basic/FileManagerTest.cpp?

clang/include/clang/Basic/FileManager.h

338

Does it make sense to return/read from a FileEntryRef here? @arphaman, wdyt?

[dexonsmith]

Added a FileManagerTest and changed FileManager::getBypassFile to use FileEntryRef.

[dexonsmith]

clang/include/clang/Basic/FileManager.h

338

SGTM.

[arphaman]

LGTM

[bruno]

LGTM!

[dexonsmith]

r370546.

clang/include/clang/Basic/FileManager.h

 class FileEntryRef {
 public:
   FileEntryRef() = delete;
   FileEntryRef(StringRef Name, const FileEntry &Entry)
       : Name(Name), Entry(&Entry) {}
 
   const StringRef getName() const { return Name; }
 
+  bool isValid() const { return Entry->isValid(); }
+
   const FileEntry &getFileEntry() const { return *Entry; }
 
   off_t getSize() const { return Entry->getSize(); }
 
   unsigned getUID() const { return Entry->getUID(); }
 
   const llvm::sys::fs::UniqueID &getUniqueID() const {
     return Entry->getUniqueID();
   }
 
   time_t getModificationTime() const { return Entry->getModificationTime(); }
 
+  friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
+    return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name;
+  }
+  friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
+    return !(LHS == RHS);
+  }
+
 private:
   StringRef Name;
   const FileEntry *Entry;
 };
 
 /// Implements support for file system lookup, file system caching,
 /// and directory search management.
 ///
class FileManager : public RefCountedBase<FileManager> {
   /// The virtual directories that we have allocated.
   ///
   /// For each virtual file (e.g. foo/bar/baz.cpp), we add all of its parent
   /// directories (foo/ and foo/bar/) here.
   SmallVector<std::unique_ptr<DirectoryEntry>, 4> VirtualDirectoryEntries;
   /// The virtual files that we have allocated.
   SmallVector<std::unique_ptr<FileEntry>, 4> VirtualFileEntries;
 
+  /// A set of files that bypass the maps and uniquing.  They can have
+  /// conflicting filenames.
+  SmallVector<std::unique_ptr<FileEntry>, 0> BypassFileEntries;
+
   /// A cache that maps paths to directory entries (either real or
   /// virtual) we have looked up, or an error that occurred when we looked up
   /// the directory.
   ///
   /// The actual Entries for real directories/files are
   /// owned by UniqueRealDirs/UniqueRealFiles above, while the Entries
   /// for virtual directories/files are owned by
   /// VirtualDirectoryEntries/VirtualFileEntries above.
public:
 
   /// Retrieve a file entry for a "virtual" file that acts as
   /// if there were a file with the given name on disk.
   ///
   /// The file itself is not accessed.
   const FileEntry *getVirtualFile(StringRef Filename, off_t Size,
                                   time_t ModificationTime);
 
+  /// Retrieve a FileEntry that bypasses VFE, which is expected to be a virtual
+  /// file entry, to access the real file.  The returned FileEntry will have
+  /// the same filename as FE but a different identity and its own stat.
+  ///
+  /// This should be used only for rare error recovery paths because it
+  /// bypasses all mapping and uniquing, blindly creating a new FileEntry.
+  /// There is no attempt to deduplicate these; if you bypass the same file
+  /// twice, you get two new file entries.
+  llvm::Optional<FileEntryRef> getBypassFile(FileEntryRef VFE);
+
   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
                    bool ShouldCloseOpenFile = true);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBufferForFile(StringRef Filename, bool isVolatile = false) {
     return getBufferForFileImpl(Filename, /*FileSize=*/-1, isVolatile);
public:
   /// \returns true if \c Path changed to absolute.
   bool makeAbsolutePath(SmallVectorImpl<char> &Path) const;
 
   /// Produce an array mapping from the unique IDs assigned to each
   /// file to the corresponding FileEntry pointer.
   void GetUniqueIDMapping(
                     SmallVectorImpl<const FileEntry *> &UIDToFiles) const;
 
-  /// Modifies the size and modification time of a previously created
-  /// FileEntry. Use with caution.
-  static void modifyFileEntry(FileEntry *File, off_t Size,
-                              time_t ModificationTime);
-
   /// Retrieve the canonical name for a given directory.
   ///
   /// This is a very expensive operation, despite its results being cached,
   /// and should only be used when the physical layout of the file system is
   /// required, which is (almost) never.
   StringRef getCanonicalName(const DirectoryEntry *Dir);
 
   void PrintStats() const;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_FILEMANAGER_H

clang/include/clang/Basic/SourceManager.h

if (OverriddenFilesInfo) {
         return true;
       if (OverriddenFilesInfo->OverriddenFiles.find(File) !=
           OverriddenFilesInfo->OverriddenFiles.end())
         return true;
     }
     return false;
   }
 
-  /// Disable overridding the contents of a file, previously enabled
-  /// with #overrideFileContents.
+  /// Bypass the overridden contents of a file.  This creates a new FileEntry
+  /// and initializes the content cache for it.  Returns nullptr if there is no
+  /// such file in the filesystem.
   ///
   /// This should be called before parsing has begun.
-  void disableFileContentsOverride(const FileEntry *File);
+  const FileEntry *bypassFileContentsOverride(const FileEntry &File);
 
   /// Specify that a file is transient.
   void setFileIsTransient(const FileEntry *SourceFile);
 
   /// Specify that all files that are read during this compilation are
   /// transient.
   void setAllFilesAreTransient(bool Transient) {
     FilesAreTransient = Transient;

clang/lib/Basic/FileManager.cpp

FileManager::getVirtualFile(StringRef Filename, off_t Size,
   UFE->ModTime = ModificationTime;
   UFE->Dir     = *DirInfo;
   UFE->UID     = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }
 
+llvm::Optional<FileEntryRef> FileManager::getBypassFile(FileEntryRef VF) {
+  // Stat of the file and return nullptr if it doesn't exist.
+  llvm::vfs::Status Status;
+  if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr))
+    return None;
+
+  // Fill it in from the stat.
+  BypassFileEntries.push_back(std::make_unique<FileEntry>());
+  const FileEntry &VFE = VF.getFileEntry();
+  FileEntry &BFE = *BypassFileEntries.back();
+  BFE.Name = VFE.getName();
+  BFE.Size = Status.getSize();
+  BFE.Dir = VFE.Dir;
+  BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
+  BFE.UID = NextFileUID++;
+  BFE.IsValid = true;
+  return FileEntryRef(VF.getName(), BFE);
+}
+
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty()
       || llvm::sys::path::is_absolute(pathRef))
     return false;
 
   SmallString<128> NewPath(FileSystemOpts.WorkingDir);
if (llvm::ErrorOr<SeenFileEntryOrRedirect> Entry = FE->getValue()) {
         UIDToFiles[FE->getUID()] = FE;
     }
 
   // Map virtual file entries
   for (const auto &VFE : VirtualFileEntries)
     UIDToFiles[VFE->getUID()] = VFE.get();
 }
 
-void FileManager::modifyFileEntry(FileEntry *File,
-                                  off_t Size, time_t ModificationTime) {
-  File->Size = Size;
-  File->ModTime = ModificationTime;
-}
-
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
   // FIXME: use llvm::sys::fs::canonical() when it gets implemented
   llvm::DenseMap<const DirectoryEntry *, llvm::StringRef>::iterator Known
     = CanonicalDirNames.find(Dir);
   if (Known != CanonicalDirNames.end())
     return Known->second;
 
   StringRef CanonicalName(Dir->getName());

clang/lib/Basic/SourceManager.cpp

assert(SourceFile->getSize() == NewFile->getSize() &&
          "Different sizes, use the FileManager to create a virtual file with "
          "the correct size");
   assert(FileInfos.count(SourceFile) == 0 &&
          "This function should be called at the initialization stage, before "
          "any parsing occurs.");
   getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
 }
 
-void SourceManager::disableFileContentsOverride(const FileEntry *File) {
-  if (!isFileOverridden(File))
-    return;
+const FileEntry *
+SourceManager::bypassFileContentsOverride(const FileEntry &File) {
+  assert(isFileOverridden(&File));
+  llvm::Optional<FileEntryRef> BypassFile =
+      FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
 
-  const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
-  const_cast<SrcMgr::ContentCache *>(IR)->replaceBuffer(nullptr);
-  const_cast<SrcMgr::ContentCache *>(IR)->ContentsEntry = IR->OrigEntry;
+  // If the file can't be found in the FS, give up.
+  if (!BypassFile)
+    return nullptr;
 
-  assert(OverriddenFilesInfo);
-  OverriddenFilesInfo->OverriddenFiles.erase(File);
-  OverriddenFilesInfo->OverriddenFilesWithBuffer.erase(File);
+  const FileEntry *FE = &BypassFile->getFileEntry();
+  (void)getOrCreateContentCache(FE);
+  return FE;
 }
 
 void SourceManager::setFileIsTransient(const FileEntry *File) {
   const SrcMgr::ContentCache *CC = getOrCreateContentCache(File);
   const_cast<SrcMgr::ContentCache *>(CC)->IsTransient = true;
 }
 
 StringRef SourceManager::getBufferData(FileID FID, bool *Invalid) const {

clang/lib/Serialization/ASTReader.cpp

InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
   // that was part of the precompiled header. Overriding such a file
   // can lead to problems when lexing using the source locations from the
   // PCH.
   SourceManager &SM = getSourceManager();
   // FIXME: Reject if the overrides are different.
   if ((!Overridden && !Transient) && SM.isFileOverridden(File)) {
     if (Complain)
       Error(diag::err_fe_pch_file_overridden, Filename);
-    // After emitting the diagnostic, recover by disabling the override so
-    // that the original file will be used.
-    //
-    // FIXME: This recovery is just as broken as the original state; there may
-    // be another precompiled module that's using the overridden contents, or
-    // we might be half way through parsing it. Instead, we should treat the
-    // overridden contents as belonging to a separate FileEntry.
-    SM.disableFileContentsOverride(File);
-    // The FileEntry is a virtual file entry with the size of the contents
-    // that would override the original contents. Set it to the original's
-    // size/time.
-    FileMgr.modifyFileEntry(const_cast<FileEntry*>(File),
-                            StoredSize, StoredTime);
+
+    // After emitting the diagnostic, bypass the overriding file to recover
+    // (this creates a separate FileEntry).
+    File = SM.bypassFileContentsOverride(*File);
+    if (!File) {
+      F.InputFilesLoaded[ID - 1] = InputFile::getNotFound();
+      return InputFile();
+    }
   }
 
   bool IsOutOfDate = false;
 
   // For an overridden file, there is nothing to validate.
   if (!Overridden && //
       (StoredSize != File->getSize() ||
        (StoredTime && StoredTime != File->getModificationTime() &&

clang/unittests/Basic/FileManagerTest.cpp

#endif
   ASSERT_TRUE(file);
   ASSERT_TRUE((*file)->isValid());
   SmallString<64> ExpectedResult = CustomWorkingDir;
 
   llvm::sys::path::append(ExpectedResult, "tmp", "test");
   EXPECT_EQ((*file)->tryGetRealPathName(), ExpectedResult);
 }
 
+TEST_F(FileManagerTest, getBypassFile) {
+  SmallString<64> CustomWorkingDir;
+#ifdef _WIN32
+  CustomWorkingDir = "C:/";
+#else
+  CustomWorkingDir = "/";
+#endif
+
+  auto FS = IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>(
+      new llvm::vfs::InMemoryFileSystem);
+  // setCurrentworkingdirectory must finish without error.
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(CustomWorkingDir));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  // Inject fake files into the file system.
+  auto Cache = std::make_unique<FakeStatCache>();
+  Cache->InjectDirectory("/tmp", 42);
+  Cache->InjectFile("/tmp/test", 43);
+  Manager.setStatCache(std::move(Cache));
+
+  // Set up a virtual file with a different size than FakeStatCache uses.
+  const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0);
+  ASSERT_TRUE(File);
+  FileEntryRef Ref("/tmp/test", *File);
+  EXPECT_TRUE(Ref.isValid());
+  EXPECT_EQ(Ref.getSize(), 10);
+
+  // Calling a second time should not affect the UID or size.
+  unsigned VirtualUID = Ref.getUID();
+  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+  EXPECT_EQ(Ref.getUID(), VirtualUID);
+  EXPECT_EQ(Ref.getSize(), 10);
+
+  // Bypass the file.
+  llvm::Optional<FileEntryRef> BypassRef = Manager.getBypassFile(Ref);
+  ASSERT_TRUE(BypassRef);
+  EXPECT_TRUE(BypassRef->isValid());
+  EXPECT_EQ(BypassRef->getName(), Ref.getName());
+
+  // Check that it's different in the right ways.
+  EXPECT_NE(&BypassRef->getFileEntry(), File);
+  EXPECT_NE(BypassRef->getUID(), VirtualUID);
+  EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+
+  // The virtual file should still be returned when searching.
+  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+}
+
 } // anonymous namespace