FileManager: Try to compute RealPathName in getVirtualFile

[simark]

Summary

I noticed that when getVirtualFile is called for a file, subsequent
calls to getFile will return a FileEntry without the RealPathName
computed:

const FileEntry *VFE = Mgr->getVirtualFile("/foo/../bar", ...);
const FileEntry *FE = Mgr->getFile("/foo/../bar");

FE->tryGetRealPathName() returns an empty string.

This is because getVirtualFile creates a FileEntry without computing the
RealPathName field and stores it in SeenFileEntries. getFile then
simply retrieves this object and returns it, when asked for the same
file.

I think it's reasonable to try to compute RealPathName in getVirtualFile
too.

There might be a similar issue with the File field. If you call
getVirtualFile, then call getFile with open=true, you may get a
FileEntry with the File field NULL, which is not what you requested. I
have not addressed this issue in this patch though.

[simark]

Update commit message.

[simark]

I'm not sure who should review this patch, please add anybody you think is qualified/responsible.

[simark]

Background: I'm trying to fix the cases why we receive a FileEntry without a real path computed in clangd, so we can avoid having to compute it ourselves.

[ioeric]

lib/Basic/FileManager.cpp

395

It seems that at this point, we have failed to find FileName in vfs (with getStatValue above), so getRealPath would also fail?

In general, the virtual file here can be an actual *virtual* file that doesn't exist anywhere, and I think there are users who use this to map virtual file (possibly with relative paths) into file manager (while they should really use overlay vfs!).

[simark]

lib/Basic/FileManager.cpp

395

It seems that at this point, we have failed to find FileName in vfs (with getStatValue above), so getRealPath would also fail?

From what I understood, this code will be executed if the file actually exists but it's the first time we access it (we won't take the return at line 373). If we take the return at line 373, then some previous invocation of getFile or getVirtualFile has created the file, and that invocation would have been responsible for computing the real path.

In general, the virtual file here can be an actual *virtual* file that doesn't exist anywhere, and I think there are users who use this to map virtual file (possibly with relative paths) into file manager (while they should really use overlay vfs!).

My thinking was that the worst that could happen is that getRealPath fails in that case.

[ioeric]

In D49197#1158972, @simark wrote:

Background: I'm trying to fix the cases why we receive a FileEntry without a real path computed in clangd, so we can avoid having to compute it ourselves.

I'm curious how you use getVirtualFile in your clangd tests. In general, I'd highly recommend virtual file system in favor of remapped files for clangd tests.

lib/Basic/FileManager.cpp

395

From what I understood, this code will be executed if the file actually exists but it's the first time we access it (we won't take the return at line 373). If we take the return at line 373, then some previous invocation of getFile or getVirtualFile has created the file, and that invocation would have been responsible for computing the real path.

I see. Thanks for the explanation!

My thinking was that the worst that could happen is that getRealPath fails in that case.

It might make sense to take a look at how getVirtualFile is used in clang. For example, in CompilerInstance, it's used to create virtual FileEntry for remapped files (https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInstance.cpp#L329), and contents will be overwritten in SourceManager. This usage is arguable as we have overlay file system nowadays. But in this case, if we try to getRealPath on a remapped file, it might happen that we accidentally get an absolute path on the real file system, if there happens to be a file with the same path (relative to the CWD) on disk. This can be surprising and could be hard to debug. This is not unusual as people tend to use file remapping to overwrite the content of disk files with dirty buffers.

In general, virtual files in FileManager and virtual files in vfs are different things, and mixing them up further would likely cause more confusion in the future. We should really get rid of the remapped virtual files in FileManager and implement them properly with an overlay fs...

[simark]

In D49197#1159704, @ioeric wrote:

In D49197#1158972, @simark wrote:

Background: I'm trying to fix the cases why we receive a FileEntry without a real path computed in clangd, so we can avoid having to compute it ourselves.

I'm curious how you use getVirtualFile in your clangd tests. In general, I'd highly recommend virtual file system in favor of remapped files for clangd tests.

I don't use getVirtualFile directly. I just use ClangdServer and look at the paths it outputs. It just happens that for some file, clang first opened it internally using getVirtualFile before using getFile, so the real path ends up missing`. Patch D48687 adds code to compensate that, but I added some logging to know when it happens, so we can try to fix it at the root (ideally, clang could always give us the real path so we don't have to compute it ourselves):

./tools/clang/tools/extra/unittests/clangd/ClangdTests --gtest_filter='*RelPathsInCompileCommand*'
Note: Google Test filter = *RelPathsInCompileCommand*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GoToDefinition
[ RUN      ] GoToDefinition.RelPathsInCompileCommand
Updating file /clangd-test/src/foo.cpp with command [/clangd-test/build] clang -ffreestanding ../src/foo.cpp -resource-dir=/home/emaisin/build/llvm/tools/clang/tools/extra/unittests/clangd/../lib/clang/7.0.0
Preamble for file /clangd-test/src/foo.cpp cannot be reused. Attempting to rebuild it.
Built preamble of size 171284 for file /clangd-test/src/foo.cpp
FileEntry for ../src/foo.cpp did not contain the real path.     <<<<< HERE
FileEntry for /clangd-test/src/header_in_preamble.h did not contain the real path.     <<<<< AND HERE
[       OK ] GoToDefinition.RelPathsInCompileCommand (42 ms)
[----------] 1 test from GoToDefinition (42 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (43 ms total)
[  PASSED  ] 1 test.

I'm investigating why clang failed to provide that to us in these two cases.

[simark]

lib/Basic/FileManager.cpp

395

Ok, I'm not familiar with what file remapping is.

An alternative approach would be to compute the real path in getFile when there is a cache hit but the file entry previously represented a virtual file. Eessentially, we would "upgrade" the virtual file to a standard one.

lib/Basic/FileManager.cpp

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();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+    UFE->RealPathName = RealPathName.str();
+
   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

#endif
 
   SmallString<64> ExpectedResult(CustomWorkingDir);
   llvm::sys::path::append(ExpectedResult, Path);
 
   ASSERT_TRUE(Manager.makeAbsolutePath(Path));
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+      IntrusiveRefCntPtr<vfs::InMemoryFileSystem>(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace