[FileManager] getFile(open=true) after getFile(open=false) should ope…

…n the file.

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.

Reviewers: ilya-biryukov

Subscribers: ioeric, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D54691

llvm-svn: 347205

clang/include/clang/Basic/FileManager.h

@@ -70,14 +70,15 @@ class FileEntry {
   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;

clang/lib/Basic/FileManager.cpp

@@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
       *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;
+  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;
 
-  // By default, initialize it to invalid.
-  NamedFileEnt.second = NON_EXISTENT_FILE;
-
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   StringRef InterndFileName = NamedFileEnt.first();
@@ -267,6 +273,7 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   // 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;
 
@@ -283,6 +290,23 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
     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
@@ -313,21 +337,9 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   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;
 }
 
@@ -398,6 +410,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
   UFE->UID     = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+  UFE->DeferredOpen = false;
   return UFE;
 }
 

clang/unittests/Basic/FileManagerTest.cpp

@@ -222,6 +222,33 @@ TEST_F(FileManagerTest, getFileReturnsNULLForNonexistentFile) {
   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