[clang] bdc3ce9 - [clang] Make `FileEntryRef::getDir()` return the as-requested `DirectoryEntryRef`
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Thu May 25 12:37:13 PDT 2023
Author: Jan Svoboda
Date: 2023-05-25T12:36:57-07:00
New Revision: bdc3ce9e8f8dda42ed286f72094fbbdf0347cbfa
URL: https://github.com/llvm/llvm-project/commit/bdc3ce9e8f8dda42ed286f72094fbbdf0347cbfa
DIFF: https://github.com/llvm/llvm-project/commit/bdc3ce9e8f8dda42ed286f72094fbbdf0347cbfa.diff
LOG: [clang] Make `FileEntryRef::getDir()` return the as-requested `DirectoryEntryRef`
For redirected file entries, `FileEntryRef::getDir()` returns the parent directory entry of the target file entry. This differs from `FileEntry::getDir()` that always returns the parent directory that was last used to look up that file.
After switching from `FileEntry` to `FileEntryRef` for umbrella headers in D142113, this discrepancy became observable and caused Clang to emit incorrect diagnostics.
This patch changes Clang so that it always associates `FileEntryRef` with the parent directory that was used to look it up. This brings its behavior closer to `FileEntry`, but without the hacky mutation.
This also ensures that `llvm::sys::path::parent_path(FileRef->getNameAsRequested()) == FileRef->getDir()->getName()`. Previously, `FileRef->getDir()` would fall underneath the redirecting VFS into the world of on-disk paths.
Reviewed By: benlangmuir, rmaz
Differential Revision: https://reviews.llvm.org/D151398
Added:
clang/test/Modules/vfs-umbrella-same-dir.m
Modified:
clang/include/clang/Basic/FileEntry.h
clang/lib/Basic/FileManager.cpp
clang/unittests/Basic/FileEntryTest.cpp
clang/unittests/Basic/FileManagerTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h
index d86191aaa0c7b..fdeafe5348cd0 100644
--- a/clang/include/clang/Basic/FileEntry.h
+++ b/clang/include/clang/Basic/FileEntry.h
@@ -70,7 +70,7 @@ class FileEntryRef {
const FileEntry &getFileEntry() const {
return *getBaseMapEntry().second->V.get<FileEntry *>();
}
- DirectoryEntryRef getDir() const { return *getBaseMapEntry().second->Dir; }
+ DirectoryEntryRef getDir() const { return ME->second->Dir; }
inline off_t getSize() const;
inline unsigned getUID() const;
@@ -120,12 +120,12 @@ class FileEntryRef {
/// name that was used to lookup the file.
llvm::PointerUnion<FileEntry *, const MapEntry *> V;
- /// Directory the file was found in. Set if and only if V is a FileEntry.
- OptionalDirectoryEntryRef Dir;
+ /// Directory the file was found in.
+ DirectoryEntryRef Dir;
MapValue() = delete;
MapValue(FileEntry &FE, DirectoryEntryRef Dir) : V(&FE), Dir(Dir) {}
- MapValue(MapEntry &ME) : V(&ME) {}
+ MapValue(MapEntry &ME, DirectoryEntryRef Dir) : V(&ME), Dir(Dir) {}
};
/// Check if RHS referenced the file in exactly the same way.
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 7d6fc47dbc384..0f544f0215ac1 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -319,7 +319,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
// Cache the redirection in the previously-inserted entry, still available
// in the tentative return value.
- NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
+ NamedFileEnt->second = FileEntryRef::MapValue(Redirection, DirInfo);
}
FileEntryRef ReturnedRef(*NamedFileEnt);
diff --git a/clang/test/Modules/vfs-umbrella-same-dir.m b/clang/test/Modules/vfs-umbrella-same-dir.m
new file mode 100644
index 0000000000000..d576dc68a5d82
--- /dev/null
+++ b/clang/test/Modules/vfs-umbrella-same-dir.m
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- sources/FW/Private.h
+#include <FW/PrivateUnmapped.h>
+//--- sources/FW/PrivateUnmapped.h
+#include <FW/Public.h>
+//--- sources/FW/Public.h
+#include <FW/PublicPresent.h>
+//--- frameworks/FW.framework/Headers/PublicPresent.h
+// empty
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "Public.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "Private.h" }
+//--- vfs.json.in
+{
+ "case-sensitive": "false",
+ "version": 0,
+ "roots": [
+ {
+ "contents": [
+ {
+ "external-contents": "DIR/sources/FW/Public.h",
+ "name": "Public.h",
+ "type": "file"
+ }
+ ],
+ "name": "DIR/frameworks/FW.framework/Headers",
+ "type": "directory"
+ },
+ {
+ "contents": [
+ {
+ "external-contents": "DIR/sources/FW/Private.h",
+ "name": "Private.h",
+ "type": "file"
+ }
+ ],
+ "name": "DIR/frameworks/FW.framework/PrivateHeaders",
+ "type": "directory"
+ }
+ ]
+}
+
+//--- tu.m
+#import <FW/Private.h>
+// expected-no-diagnostics
+
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN: -ivfsoverlay %t/vfs.json -I %t/sources -F %t/frameworks -fsyntax-only %t/tu.m -verify
diff --git a/clang/unittests/Basic/FileEntryTest.cpp b/clang/unittests/Basic/FileEntryTest.cpp
index b1755e07f48fc..0878063994708 100644
--- a/clang/unittests/Basic/FileEntryTest.cpp
+++ b/clang/unittests/Basic/FileEntryTest.cpp
@@ -9,6 +9,7 @@
#include "clang/Basic/FileEntry.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Path.h"
#include "gtest/gtest.h"
using namespace llvm;
@@ -51,11 +52,14 @@ class FileEntryTestHelper {
.first);
}
FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+ auto Dir = addDirectory(llvm::sys::path::parent_path(Name));
+
return FileEntryRef(
*Files
.insert({Name, FileEntryRef::MapValue(
const_cast<FileEntryRef::MapEntry &>(
- Base.getMapEntry()))})
+ Base.getMapEntry()),
+ Dir)})
.first);
}
};
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index 146a6098dfb8b..4c8205b68a02f 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -310,6 +310,26 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
EXPECT_EQ(&F2->getFileEntry(), &F2Alias2->getFileEntry());
}
+TEST_F(FileManagerTest, getFileRefReturnsCorrectDirNameForDifferentStatPath) {
+ // Inject files with the same inode into distinct directories (name & inode).
+ auto StatCache = std::make_unique<FakeStatCache>();
+ StatCache->InjectDirectory("dir1", 40);
+ StatCache->InjectDirectory("dir2", 41);
+ StatCache->InjectFile("dir1/f.cpp", 42);
+ StatCache->InjectFile("dir2/f.cpp", 42, "dir1/f.cpp");
+
+ manager.setStatCache(std::move(StatCache));
+ auto Dir1F = manager.getFileRef("dir1/f.cpp");
+ auto Dir2F = manager.getFileRef("dir2/f.cpp");
+
+ ASSERT_FALSE(!Dir1F);
+ ASSERT_FALSE(!Dir2F);
+ EXPECT_EQ("dir1", Dir1F->getDir().getName());
+ EXPECT_EQ("dir2", Dir2F->getDir().getName());
+ EXPECT_EQ("dir1/f.cpp", Dir1F->getNameAsRequested());
+ EXPECT_EQ("dir2/f.cpp", Dir2F->getNameAsRequested());
+}
+
// getFile() returns the same FileEntry for virtual files that have
// corresponding real files that are aliases.
TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
More information about the cfe-commits
mailing list