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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
