[FileManager] getFile(open=true) after getFile(open=false) should open the file.

[sammccall]

Summary

Old behavior is to just return the cached entry regardless of opened-ness.
That feels buggy (though I guess nobody ever actually needed this).

This came up in the context of clangd+clang-tidy integration: we're
going to getFile(open=false) to replay preprocessor actions obscured by
the preamble, but the compilation may subsequently getFile(open=true)
for non-preamble includes.

[ilya-biryukov]

Overall LG. The only concerning bit is that the initialization of the UFE is now scattered across the function body.
Grouping File and DeferredOpen into a struct or adding a comment that those are initialized separately from the rest of the fields might make it more evident what's going on. WDYT?

include/clang/Basic/FileManager.h

72

NIT: not sure if it's actually that useful, but the fact that the comment mentioned that FileEntry could be uninitialized if IsValid is false was a hint that drew my attention to this.

lib/Basic/FileManager.cpp

263

Maybe add a comment that DeferredOpen might do an extra stat here that could technically be avoided?
No need to fix this, though, hopefully that won't matter.

306

Maybe add an assertion that DeferredOpen is false at this point?
To make it clear we'll never hit this branch twice.

[sammccall]

In D54691#1302622, @ilya-biryukov wrote:

Overall LG. The only concerning bit is that the initialization of the UFE is now scattered across the function body.
Grouping File and DeferredOpen into a struct or adding a comment that those are initialized separately from the rest of the fields might make it more evident what's going on. WDYT?

I added a comment in getFile() where ~all the UFE fields are initialized that File and DeferredOpen are initialized above.
Added a redundant initialization in getVirtualFile() for clarity.

[sammccall]

Address review comments.

[ilya-biryukov]

Thanks! LGTM

include/clang/Basic/FileManager.h

class FileEntry {
   std::string RealPathName;   // Real path to the file; could be empty.
   off_t Size;                 // File size in bytes.
   time_t ModTime;             // Modification time of file.
   const DirectoryEntry *Dir;  // Directory file lives in.
   unsigned UID;               // A unique (small) ID for the file.
   llvm::sys::fs::UniqueID UniqueID;
   bool IsNamedPipe;
   bool InPCH;
   bool IsValid;               // Is this \c FileEntry initialized and valid?
+  bool DeferredOpen;          // Created by getFile(OpenFile=0); may open later.
 
   /// The open file, if it is owned by the \p FileEntry.
   mutable std::unique_ptr<llvm::vfs::File> File;
 
 public:
   FileEntry()
-      : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
-  {}
+      : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
+        DeferredOpen(false) {}
 
   FileEntry(const FileEntry &) = delete;
   FileEntry &operator=(const FileEntry &) = delete;
 
   StringRef getName() const { return Name; }
   StringRef tryGetRealPathName() const { return RealPathName; }
   bool isValid() const { return IsValid; }
   off_t getSize() const { return Size; }

lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
                                       bool CacheFailure) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map.
   auto &NamedFileEnt =
       *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
-  if (NamedFileEnt.second)
-    return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
-                                                    : NamedFileEnt.second;
-
-  ++NumFileCacheMisses;
-
+  if (NamedFileEnt.second) {
+    if (NamedFileEnt.second == NON_EXISTENT_FILE)
+      return nullptr;
+    // Entry exists: return it *unless* it wasn't opened and open is requested.
+    if (!(NamedFileEnt.second->DeferredOpen && openFile))
+      return NamedFileEnt.second;
+    // We previously stat()ed the file, but didn't open it: do that below.
+    // FIXME: the below does other redundant work too (stats the dir and file).
+  } else {
     // By default, initialize it to invalid.
     NamedFileEnt.second = NON_EXISTENT_FILE;
+  }
+
+  ++NumFileCacheMisses;
 
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   StringRef InterndFileName = NamedFileEnt.first();
 
   // Look up the directory for the file.  When looking up something like
   // sys/foo.h we'll discover all of the search directories that have a 'sys'
   // subdirectory.  This will let us avoid having to waste time on known-to-fail
const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   }
 
   // FIXME: Use the directory info to prune this, before doing the stat syscall.
   // FIXME: This will reduce the # syscalls.
 
   // Nope, there isn't.  Check to see if the file exists.
   std::unique_ptr<llvm::vfs::File> F;
   FileData Data;
   if (getStatValue(InterndFileName, Data, true, openFile ? &F : nullptr)) {
     // There's no real file at the given path.
     if (!CacheFailure)
       SeenFileEntries.erase(Filename);
 
     return nullptr;
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
+  UFE.DeferredOpen = !openFile;
 
   NamedFileEnt.second = &UFE;
 
   // If the name returned by getStatValue is different than Filename, re-intern
   // the name.
   if (Data.Name != Filename) {
     auto &NamedFileEnt =
         *SeenFileEntries.insert(std::make_pair(Data.Name, nullptr)).first;
     if (!NamedFileEnt.second)
       NamedFileEnt.second = &UFE;
     else
       assert(NamedFileEnt.second == &UFE &&
              "filename from getStatValue() refers to wrong file");
     InterndFileName = NamedFileEnt.first().data();
   }
 
+  // If we opened the file for the first time, record the resulting info.
+  // Do this even if the cache entry was valid, maybe we didn't previously open.
+  if (F && !UFE.File) {
+    if (auto PathName = F->getName()) {
+      llvm::SmallString<128> AbsPath(*PathName);
+      // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+      // but can be very expensive on real file systems.
+      // FIXME: the semantic of RealPathName is unclear, and the name might be
+      // misleading. We need to clean up the interface here.
+      makeAbsolutePath(AbsPath);
+      llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+      UFE.RealPathName = AbsPath.str();
+    }
+    UFE.File = std::move(F);
+    assert(!UFE.DeferredOpen && "we just opened it!");
+  }
+
   if (UFE.isValid()) { // Already have an entry with this inode, return it.
 
     // FIXME: this hack ensures that if we look up a file by a virtual path in
     // the VFS that the getDir() will have the virtual path, even if we found
     // the file by a 'real' path first. This is required in order to find a
     // module's structure when its headers/module map are mapped in the VFS.
     // We should remove this as soon as we can properly support a file having
     // multiple names.
const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.Name    = InterndFileName;
   UFE.Size = Data.Size;
   UFE.ModTime = Data.ModTime;
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
-  UFE.File = std::move(F);
   UFE.IsValid = true;
+  // Note File and DeferredOpen were initialized above.
 
-  if (UFE.File) {
-    if (auto PathName = UFE.File->getName()) {
-      llvm::SmallString<128> AbsPath(*PathName);
-      // This is not the same as `VFS::getRealPath()`, which resolves symlinks
-      // but can be very expensive on real file systems.
-      // FIXME: the semantic of RealPathName is unclear, and the name might be
-      // misleading. We need to clean up the interface here.
-      makeAbsolutePath(AbsPath);
-      llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-      UFE.RealPathName = AbsPath.str();
-    }
-  }
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;
 
FileManager::getVirtualFile(StringRef Filename, off_t Size,
 
   UFE->Name    = InterndFileName;
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = DirInfo;
   UFE->UID     = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+  UFE->DeferredOpen = false;
   return UFE;
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty()
       || llvm::sys::path::is_absolute(pathRef))

unittests/Basic/FileManagerTest.cpp

TEST_F(FileManagerTest, getFileReturnsNULLForNonexistentFile) {
 
   // Create a virtual bar.cpp file.
   manager.getVirtualFile("bar.cpp", 200, 0);
 
   const FileEntry *file = manager.getFile("xyz.txt");
   EXPECT_EQ(nullptr, file);
 }
 
+// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is
+// opened for the second call.
+TEST_F(FileManagerTest, getFileDefersOpen) {
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
+      new llvm::vfs::InMemoryFileSystem());
+  FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));
+  FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));
+  FileManager manager(options, FS);
+
+  const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  // "real path name" reveals whether the file was actually opened.
+  EXPECT_EQ("", file->tryGetRealPathName());
+
+  file = manager.getFile("/tmp/test", /*OpenFile=*/true);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
+
+  // However we should never try to open a file previously opened as virtual.
+  ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
+  ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
+  file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
+  EXPECT_EQ("", file->tryGetRealPathName());
+}
+
 // The following tests apply to Unix-like system only.
 
 #ifndef _WIN32
 
 // getFile() returns the same FileEntry for real files that are aliases.
 TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedRealFiles) {
   // Inject two real files with the same inode.
   auto statCache = llvm::make_unique<FakeStatCache>();