[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/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.



================
Comment at: lib/Basic/FileManager.cpp:395
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+    UFE->RealPathName = RealPathName.str();
----------------
simark wrote:
> ioeric wrote:
> > 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!).
> > 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.
> 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...


Repository:
  rC Clang

https://reviews.llvm.org/D49197