[clang] Fill RealPathName for virtual files.

[kadircet]

Summary

Absolute path information for virtual files were missing even if we
have already stat'd the files. This patch puts that information for virtual
files that can succesffully be stat'd.

[kadircet]

• Update tests

[ilya-biryukov]

include/clang/Basic/FileManager.h

181

The FIXME inside the function suggests we might change the name or semantics of RealPathName, so maybe simply refer to the field by name and avoid mentioning the semantics of the function.

unittests/Basic/FileManagerTest.cpp

237

This test seems to rely on the old behavior. Is there a way to test the file wasn't open other than looking at RealPathName?

238

Should we fill-in the RealPathName in that case as well?
I'm not sure what the semantics should be. WDYT?

253

We should find a way to test file was not "opened" (I think it actually does stat the file now, right?)

323

After a check EXPECT_EQ(file1, file2), this is clearly true.
Maybe add a smaller test with a single call to getVirtualFile() and an assertion that the real path is set?
Alternatively, add an assertion to this test right after the first call to getVirtualFile().

[kadircet]

Address comments

[kadircet]

unittests/Basic/FileManagerTest.cpp

237

There is also the vfs::file stored inside the FileEntry but unfortunately there are no getters exposing this one. But I believe this behavior is still correct for the getFile case, since RealPathName will only be filled when we open the file.

238

I don't think so, since we need the vfs::file to query the real path, I believe it is still ok to leave them empty for real files that hasn't been opened yet.

IIUC, stating and opening are very similar in nature(from the perspective of FileManager), the only difference is there is no deferred stating for virtual files therefore it is like a normal file that will always be opened.

253

AFAIK, any file accessed through getVirtualFile is immediately opened, then later on accessing a file with the same name through getFile with openfile=false won't have any affect, since this file will be alive and marked as opened in the cache.

So I believe opening and stating is dual of each other. Therefore checking for one should be equivalent to check for other.

[ilya-biryukov]

LGTM and a few nits

include/clang/Basic/FileManager.h

108

s/arther/rather
Let's leave out the RealPathName part, it seemed to be there for historical reasons.

182

We never use the returned value. Maybe drop it?

unittests/Basic/FileManagerTest.cpp

237

Using vfs::File LG, thanks!

253

I'm not sure about the semantics either, it seems we need to stat anyway, maybe open referes to reading the file contents in this case?
Anyhow, we're not actually changing semantics of the parts that were tested here, so there's no reason to go deeper into this.

[kadircet]

• Address comments

include/clang/Basic/FileManager.h

public:
 
   /// Check whether the file is a named pipe (and thus can't be opened by
   /// the native FileManager methods).
   bool isNamedPipe() const { return IsNamedPipe; }
 
   void closeFile() const {
     File.reset(); // rely on destructor to close File
   }
+
+  // Only for use in tests to see if deferred opens are happening, rather than
+  // relying on RealPathName being empty.
+  bool isOpenForTests() const { return File != nullptr; }
 };
 
 struct FileData;
 
 /// Implements support for file system lookup, file system caching,
 /// and directory search management.
 ///
 /// This also handles more advanced properties, such as uniquing files based
class FileManager : public RefCountedBase<FileManager> {
 
   bool getStatValue(StringRef Path, FileData &Data, bool isFile,
                     std::unique_ptr<llvm::vfs::File> *F);
 
   /// Add all ancestors of the given path (pointing to either a file
   /// or a directory) as virtual directories.
   void addAncestorsAsVirtualDirs(StringRef Path);
 
+  /// Fills the RealPathName in file entry.
+  void fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+
 public:
   FileManager(const FileSystemOptions &FileSystemOpts,
               IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
   ~FileManager();
 
   /// Installs the provided FileSystemStatCache object within
   /// the FileManager.
   ///

lib/Basic/FileManager.cpp

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();
-    }
+    if (auto PathName = F->getName())
+      fillRealPathName(&UFE, *PathName);
     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
if (getStatValue(InterndFileName, Data, true, nullptr) == 0) {
 
     // If we already have an entry with this inode, return it.
     if (UFE->isValid())
       return UFE;
 
     UFE->UniqueID = Data.UniqueID;
     UFE->IsNamedPipe = Data.IsNamedPipe;
     UFE->InPCH = Data.InPCH;
+    fillRealPathName(UFE, Data.Name);
   }
 
   if (!UFE) {
     VirtualFileEntries.push_back(llvm::make_unique<FileEntry>());
     UFE = VirtualFileEntries.back().get();
     NamedFileEnt.second = UFE;
   }
 
bool FileManager::makeAbsolutePath(SmallVectorImpl<char> &Path) const {
   if (!llvm::sys::path::is_absolute(StringRef(Path.data(), Path.size()))) {
     FS->makeAbsolute(Path);
     Changed = true;
   }
 
   return Changed;
 }
 
+void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
+  llvm::SmallString<128> AbsPath(FileName);
+  // 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();
+}
+
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
                               bool ShouldCloseOpenFile) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
   if (isVolatile)
     FileSize = -1;

unittests/Basic/FileManagerTest.cpp

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());
+  EXPECT_FALSE(file->isOpenForTests());
 
   file = manager.getFile("/tmp/test", /*OpenFile=*/true);
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
-#ifdef _WIN32
-  EXPECT_EQ("/tmp\\test", file->tryGetRealPathName());
-#else
-  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
-#endif
+  EXPECT_TRUE(file->isOpenForTests());
 
   // 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());
+  EXPECT_FALSE(file->isOpenForTests());
 }
 
 // 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) {
TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
   ASSERT_TRUE(file2->isValid());
   // Check that it's the same UFE:
   EXPECT_EQ(file1, file2);
   EXPECT_EQ(43U, file2->getUniqueID().getFile());
   // Check that the contents of the UFE are not overwritten by the entry in the
   // filesystem:
   EXPECT_EQ(123, file2->getSize());
 }
 
 #endif  // !_WIN32
 
 TEST_F(FileManagerTest, makeAbsoluteUsesVFS) {
   SmallString<64> CustomWorkingDir;
 #ifdef _WIN32
   CustomWorkingDir = "C:";
 #else
   CustomWorkingDir = "/";
#endif
 
   SmallString<64> ExpectedResult(CustomWorkingDir);
   llvm::sys::path::append(ExpectedResult, Path);
 
   ASSERT_TRUE(Manager.makeAbsolutePath(Path));
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+// getVirtualFile should always fill the real path.
+TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique<FakeStatCache>();
+  statCache->InjectDirectory("/tmp", 42);
+  statCache->InjectFile("/tmp/test", 43);
+  manager.addStatCache(std::move(statCache));
+
+  // Check for real path.
+  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 1);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  EXPECT_EQ(file->tryGetRealPathName(), "/tmp/test");
+}
+
 } // anonymous namespace