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

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.


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


Repository:
  rC Clang

https://reviews.llvm.org/D49197