[clang] Only modify FileEntryRef names that are externally remapped

[benlangmuir]

Summary

As progress towards having FileEntryRef contain the requested name of the file, this commit narrows the "remap" hack to only apply to paths that were remapped to an external contents path by a VFS. That was always the original intent of this code, and the fact it was making relative paths absolute was an unintended side effect.

[bnbarham]

clang/lib/Basic/FileManager.cpp

277–279

Is it possible for an ExposesExternalVFSPath to have the same filename as the one that was requested? Just in the case of mapping A -> A or something equally pointless? Could check for that in the VFS, but probably doesn't matter in the long run.

[benlangmuir]

clang/lib/Basic/FileManager.cpp

277–279

I think in practice it won't happen, but in theory the VFS can do whatever it wants, so I didn't want to introduce any possibility of circularity here. I will flip these checks so the boolean comes before the string compare though, since that will be cheaper.

[benlangmuir]

Flip the order of comparisons to avoid unnecessary string comparisons.

clang/lib/Basic/FileManager.cpp

FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry *&UFE = UniqueRealFiles[Status.getUniqueID()];
   bool ReusingEntry = UFE != nullptr;
   if (!UFE)
     UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-    // The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+    // Use the requested name. Set the FileEntry.
     NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
     // Name mismatch. We need a redirect. First grab the actual entry we want
     // to return.
     //
     // This redirection logic intentionally leaks the external name of a
     // redirected file that uses 'use-external-name' in \a
     // vfs::RedirectionFileSystem. This allows clang to report the external
     // name to users (in diagnostics) and to tools that don't have access to
     // the VFS (in debug info and dependency '.d' files).
     //
     // FIXME: This is pretty complex and has some very complicated interactions
     // with the rest of clang. It's also inconsistent with how "real"
     // filesystems behave and confuses parts of clang expect to see the
     // name-as-accessed on the \a FileEntryRef.
     //
-    // Further, it isn't *just* external names, but will also give back absolute
-    // paths when a relative path was requested - the check is comparing the
-    // name from the status, which is passed an absolute path resolved from the
-    // current working directory. `clang-apply-replacements` appears to depend
-    // on this behaviour, though it's adjusting the working directory, which is
-    // definitely not supported. Once that's fixed this hack should be able to
-    // be narrowed to only when there's an externally mapped name given back.
-    //
     // A potential plan to remove this is as follows -
-    //   - Add API to determine if the name has been rewritten by the VFS.
-    //   - Fix `clang-apply-replacements` to pass down the absolute path rather
-    //     than changing the CWD. Narrow this hack down to just externally
-    //     mapped paths.
     //   - Expose the requested filename. One possibility would be to allow
     //     redirection-FileEntryRefs to be returned, rather than returning
     //     the pointed-at-FileEntryRef, and customizing `getName()` to look
     //     through the indirection.
     //   - Update callers such as `HeaderSearch::findUsableModuleForHeader()`
     //     to explicitly use the requested filename rather than just using
     //     `getName()`.
     //   - Add a `FileManager::getExternalPath` API for explicitly getting the

clang/unittests/Basic/FileManagerTest.cpp

if (is_style_posix(llvm::sys::path::Style::native)) {
 
       if (StatPath) {
         NormalizedStatPath = StatPath;
         llvm::sys::path::native(NormalizedStatPath);
         StatPath = NormalizedStatPath.c_str();
       }
     }
 
-    if (!StatPath)
-      StatPath = Path;
-
     auto fileType = IsFile ?
       llvm::sys::fs::file_type::regular_file :
       llvm::sys::fs::file_type::directory_file;
-    llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+    llvm::vfs::Status Status(StatPath ? StatPath : Path,
+                             llvm::sys::fs::UniqueID(1, INode),
                              /*MTime*/{}, /*User*/0, /*Group*/0,
                              /*Size*/0, fileType,
                              llvm::sys::fs::perms::all_all);
+    if (StatPath)
+      Status.ExposesExternalVFSPath = true;
     StatCalls[Path] = Status;
   }
 
 public:
   // Inject a file with the given inode value to the fake file system.
   void InjectFile(const char *Path, ino_t INode,
                   const char *StatPath = nullptr) {
     InjectFileOrDirectory(Path, INode, /*IsFile=*/true, StatPath);