[clang] d77588d - [llvm][vfs] For virtual directories, use the virtual path as the real path
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 10 10:41:22 PDT 2023
Author: Jan Svoboda
Date: 2023-07-10T10:41:15-07:00
New Revision: d77588df4553f0e93a74e6eab33e1ce87b576320
URL: https://github.com/llvm/llvm-project/commit/d77588df4553f0e93a74e6eab33e1ce87b576320
DIFF: https://github.com/llvm/llvm-project/commit/d77588df4553f0e93a74e6eab33e1ce87b576320.diff
LOG: [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
Added:
clang/test/ClangScanDeps/modules-canononical-module-map-case.c
Modified:
clang/lib/Lex/ModuleMap.cpp
llvm/include/llvm/Support/VirtualFileSystem.h
llvm/lib/Support/VirtualFileSystem.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index cf546c4b3f9fe3..7a22fead3a0896 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/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
diff --git a/clang/test/ClangScanDeps/modules-canononical-module-map-case.c b/clang/test/ClangScanDeps/modules-canononical-module-map-case.c
new file mode 100644
index 00000000000000..a7c9624e2de71c
--- /dev/null
+++ b/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
diff erent 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: }
diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h
index 3ef17a7de379d0..697343c7e763e5 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/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,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index bcf5389db6a0e6..acb0df2b15f58c 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/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,11 +2265,14 @@ 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);
}
@@ -2269,7 +2280,8 @@ RedirectingFileSystem::lookupPath(StringRef Path) const {
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;
}
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index e1b3ab693729cc..bd4048f025c0de 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/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),
More information about the cfe-commits
mailing list