jansvoboda11 updated this revision to Diff 537689.
jansvoboda11 added a comment.
Add unit tests, use `SmallVector`, make `clang-scan-deps` test portable
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135849/new/
https://reviews.llvm.org/D135849
Files:
clang/lib/Lex/ModuleMap.cpp
clang/test/ClangScanDeps/modules-canononical-module-map-case.c
llvm/include/llvm/Support/VirtualFileSystem.h
llvm/lib/Support/VirtualFileSystem.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===================================================================
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2580,6 +2580,7 @@
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 @@
" '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 @@
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),
Index: llvm/lib/Support/VirtualFileSystem.cpp
===================================================================
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -2239,6 +2239,14 @@
}
}
+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 @@
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 @@
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 @@
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 @@
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;
}
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===================================================================
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -872,6 +872,9 @@
/// 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 @@
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 @@
/// 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,
Index: clang/test/ClangScanDeps/modules-canononical-module-map-case.c
===================================================================
--- /dev/null
+++ 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: }
Index: clang/lib/Lex/ModuleMap.cpp
===================================================================
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1325,18 +1325,8 @@
// 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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
