[llvm][vfs] For virtual directories, use the virtual path as the real…

… path

A follow-up to D135841. This patch returns the virtual path for directories from `RedirectingFileSystem`. This ensures the contents of `Path` are the same as the contents of `FS->getRealPath(Path)`. This also means we can drop the workaround in Clang's module map canonicalization, where we couldn't use the real path for a directory if it resolved to a different `DirectoryEntry`. In addition to that, we can also avoid introducing new workaround for a bug triggered by the newly introduced test case.

Reviewed By: benlangmuir

Differential Revision: https://reviews.llvm.org/D135849

clang/lib/Lex/ModuleMap.cpp

@@ -1325,18 +1325,8 @@ ModuleMap::canonicalizeModuleMapPath(SmallVectorImpl<char> &Path) {
 
   // Canonicalize the directory.
   StringRef CanonicalDir = FM.getCanonicalName(*DirEntry);
-  if (CanonicalDir != Dir) {
-    auto CanonicalDirEntry = FM.getDirectory(CanonicalDir);
-    // Only use the canonicalized path if it resolves to the same entry as the
-    // original. This is not true if there's a VFS overlay on top of a FS where
-    // the directory is a symlink. The overlay would not remap the target path
-    // of the symlink to the same directory entry in that case.
-    if (CanonicalDirEntry && *CanonicalDirEntry == *DirEntry) {
-      bool Done = llvm::sys::path::replace_path_prefix(Path, Dir, CanonicalDir);
-      (void)Done;
-      assert(Done && "Path should always start with Dir");
-    }
-  }
+  if (CanonicalDir != Dir)
+    llvm::sys::path::replace_path_prefix(Path, Dir, CanonicalDir);
 
   // In theory, the filename component should also be canonicalized if it
   // on a case-insensitive filesystem. However, the extra canonicalization is

clang/test/ClangScanDeps/modules-canononical-module-map-case.c

@@ -0,0 +1,71 @@
+// This test checks that VFS-mapped module map path has the correct spelling
+// after canonicalization, even if it was first accessed using different case.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- actual/One.h
+#import <FW/Two.h>
+//--- actual/Two.h
+// empty
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW {
+  header "One.h"
+  header "Two.h"
+}
+//--- tu.m
+#import <fw/One.h>
+
+//--- overlay.json.in
+{
+  "version": 0,
+  "case-sensitive": "false",
+  "roots": [
+    {
+      "contents": [
+        {
+          "external-contents": "DIR/actual/One.h",
+          "name": "One.h",
+          "type": "file"
+        },
+        {
+          "external-contents": "DIR/actual/Two.h",
+          "name": "Two.h",
+          "type": "file"
+        }
+      ],
+      "name": "DIR/frameworks/FW.framework/Headers",
+      "type": "directory"
+    }
+  ]
+}
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "file": "DIR/tu.m",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -ivfsoverlay DIR/overlay.json -F DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
+}]
+
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: sed -e "s|DIR|%/t|g" %t/overlay.json.in > %t/overlay.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:      {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
+// CHECK-NEXT:       "command-line": [
+// CHECK:              "-x"
+// CHECK-NEXT:         "objective-c"
+// CHECK-NEXT:         "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap"
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK:            ],
+// CHECK-NEXT:       "name": "FW"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ]
+// CHECK:      }

llvm/include/llvm/Support/VirtualFileSystem.h

@@ -872,6 +872,9 @@ class RedirectingFileSystem : public vfs::FileSystem {
 
   /// Represents the result of a path lookup into the RedirectingFileSystem.
   struct LookupResult {
+    /// Chain of parent directory entries for \c E.
+    llvm::SmallVector<Entry *, 32> Parents;
+
     /// The entry the looked-up path corresponds to.
     Entry *E;
 
@@ -895,6 +898,10 @@ class RedirectingFileSystem : public vfs::FileSystem {
         return FE->getExternalContentsPath();
       return std::nullopt;
     }
+
+    /// Get the (canonical) path of the found entry. This uses the as-written
+    /// path components from the VFS specification.
+    void getPath(llvm::SmallVectorImpl<char> &Path) const;
   };
 
 private:
@@ -984,9 +991,10 @@ class RedirectingFileSystem : public vfs::FileSystem {
   /// into the contents of \p From if it is a directory. Returns a LookupResult
   /// giving the matched entry and, if that entry is a FileEntry or
   /// DirectoryRemapEntry, the path it redirects to in the external file system.
-  ErrorOr<LookupResult> lookupPathImpl(llvm::sys::path::const_iterator Start,
-                                       llvm::sys::path::const_iterator End,
-                                       Entry *From) const;
+  ErrorOr<LookupResult>
+  lookupPathImpl(llvm::sys::path::const_iterator Start,
+                 llvm::sys::path::const_iterator End, Entry *From,
+                 llvm::SmallVectorImpl<Entry *> &Entries) const;
 
   /// Get the status for a path with the provided \c LookupResult.
   ErrorOr<Status> status(const Twine &CanonicalPath, const Twine &OriginalPath,

llvm/lib/Support/VirtualFileSystem.cpp

@@ -2239,6 +2239,14 @@ RedirectingFileSystem::LookupResult::LookupResult(
   }
 }
 
+void RedirectingFileSystem::LookupResult::getPath(
+    llvm::SmallVectorImpl<char> &Result) const {
+  Result.clear();
+  for (Entry *Parent : Parents)
+    llvm::sys::path::append(Result, Parent->getName());
+  llvm::sys::path::append(Result, E->getName());
+}
+
 std::error_code
 RedirectingFileSystem::makeCanonical(SmallVectorImpl<char> &Path) const {
   if (std::error_code EC = makeAbsolute(Path))
@@ -2257,19 +2265,23 @@ ErrorOr<RedirectingFileSystem::LookupResult>
 RedirectingFileSystem::lookupPath(StringRef Path) const {
   sys::path::const_iterator Start = sys::path::begin(Path);
   sys::path::const_iterator End = sys::path::end(Path);
+  llvm::SmallVector<Entry *, 32> Entries;
   for (const auto &Root : Roots) {
     ErrorOr<RedirectingFileSystem::LookupResult> Result =
-        lookupPathImpl(Start, End, Root.get());
-    if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
+        lookupPathImpl(Start, End, Root.get(), Entries);
+    if (Result || Result.getError() != llvm::errc::no_such_file_or_directory) {
+      Result->Parents = std::move(Entries);
       return Result;
+    }
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 ErrorOr<RedirectingFileSystem::LookupResult>
 RedirectingFileSystem::lookupPathImpl(
     sys::path::const_iterator Start, sys::path::const_iterator End,
-    RedirectingFileSystem::Entry *From) const {
+    RedirectingFileSystem::Entry *From,
+    llvm::SmallVectorImpl<Entry *> &Entries) const {
   assert(!isTraversalComponent(*Start) &&
          !isTraversalComponent(From->getName()) &&
          "Paths should not contain traversal components");
@@ -2298,10 +2310,12 @@ RedirectingFileSystem::lookupPathImpl(
   auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(From);
   for (const std::unique_ptr<RedirectingFileSystem::Entry> &DirEntry :
        llvm::make_range(DE->contents_begin(), DE->contents_end())) {
+    Entries.push_back(From);
     ErrorOr<RedirectingFileSystem::LookupResult> Result =
-        lookupPathImpl(Start, End, DirEntry.get());
+        lookupPathImpl(Start, End, DirEntry.get(), Entries);
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
+    Entries.pop_back();
   }
 
   return make_error_code(llvm::errc::no_such_file_or_directory);
@@ -2543,10 +2557,12 @@ RedirectingFileSystem::getRealPath(const Twine &OriginalPath,
     return P;
   }
 
-  // If we found a DirectoryEntry, still fallthrough to the original path if
-  // allowed, because directories don't have a single external contents path.
-  if (Redirection == RedirectKind::Fallthrough)
-    return ExternalFS->getRealPath(CanonicalPath, Output);
+  // We found a DirectoryEntry, which does not have a single external contents
+  // path. Use the canonical virtual path.
+  if (Redirection == RedirectKind::Fallthrough) {
+    Result->getPath(Output);
+    return {};
+  }
   return llvm::errc::invalid_argument;
 }
 

llvm/unittests/Support/VirtualFileSystemTest.cpp

@@ -2580,6 +2580,7 @@ TEST_F(VFSFromYAMLTest, GetRealPath) {
   Lower->addSymlink("/link");
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
       "{ 'use-external-names': false,\n"
+      "  'case-sensitive': false,\n"
       "  'roots': [\n"
       "{\n"
       "  'type': 'directory',\n"
@@ -2588,6 +2589,11 @@ TEST_F(VFSFromYAMLTest, GetRealPath) {
       "                  'type': 'file',\n"
       "                  'name': 'bar',\n"
       "                  'external-contents': '/link'\n"
+      "                },\n"
+      "                {\n"
+      "                  'type': 'directory',\n"
+      "                  'name': 'baz',\n"
+      "                  'contents': []\n"
       "                }\n"
       "              ]\n"
       "},\n"
@@ -2610,9 +2616,9 @@ TEST_F(VFSFromYAMLTest, GetRealPath) {
   EXPECT_FALSE(FS->getRealPath("//root/bar", RealPath));
   EXPECT_EQ(RealPath.str(), "/symlink");
 
-  // Directories should fall back to the underlying file system is possible.
-  EXPECT_FALSE(FS->getRealPath("//dir/", RealPath));
-  EXPECT_EQ(RealPath.str(), "//dir/");
+  // Directories should return the virtual path as written in the definition.
+  EXPECT_FALSE(FS->getRealPath("//ROOT/baz", RealPath));
+  EXPECT_EQ(RealPath.str(), "//root/baz");
 
   // Try a non-existing file.
   EXPECT_EQ(FS->getRealPath("/non_existing", RealPath),