caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

FileManager: Shrink FileEntryRef to the size of a pointer

Browse Source 
Shrink `FileEntryRef` to the size of a pointer, by having it directly
reference the `StringMapEntry` the same way that `DirectoryEntryRef`
does. This makes `FileEntryRef::FileEntryRef` private as a side effect
(`FileManager` is a friend!).

There are two helper types added within `FileEntryRef`:

- `FileEntryRef::MapValue` is the type stored in
  `FileManager::SeenFileEntries`. It's a replacement for
  `SeenFileEntryOrRedirect`, where the second pointer type has been
  changed from `StringRef*` to `MapEntry*` (see next bullet).
- `FileEntryRef::MapEntry` is the instantiation of `StringMapEntry<>`
  where `MapValue` is stored. This is what `FileEntryRef` has a pointer
  to, in order to grab the name in addition to the value.

Differential Revision: https://reviews.llvm.org/D89488
This commit is contained in:
Duncan P. N. Exon Smith
2020-10-15 11:39:07 -04:00
parent 83154c5418
commit 917acac960
4 changed files with 148 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files

78
clang/include/clang/Basic/FileManager.h
View File

@@ -77,12 +77,10 @@ class FileEntry;
/// accessed by the FileManager's client.
class FileEntryRef {
public:
FileEntryRef() = delete;
FileEntryRef(StringRef Name, const FileEntry &Entry)
: Name(Name), Entry(&Entry) {}
const StringRef getName() const { return Name; }
const FileEntry &getFileEntry() const { return *Entry; }
const StringRef getName() const { return Entry->first(); }
const FileEntry &getFileEntry() const {
return *Entry->second->V.get<FileEntry *>();
}
inline bool isValid() const;
inline off_t getSize() const;
@@ -91,15 +89,43 @@ public:
inline time_t getModificationTime() const;
friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name;
return LHS.Entry == RHS.Entry;
}
friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
return !(LHS == RHS);
}
struct MapValue;
/// Type used in the StringMap.
using MapEntry = llvm::StringMapEntry<llvm::ErrorOr<MapValue>>;
/// Type stored in the StringMap.
struct MapValue {
/// The pointer at another MapEntry is used when the FileManager should
/// silently forward from one name to another, which occurs in Redirecting
/// VFSs that use external names. In that case, the \c FileEntryRef
/// returned by the \c FileManager will have the external name, and not the
/// name that was used to lookup the file.
llvm::PointerUnion<FileEntry *, const MapEntry *> V;
MapValue() = delete;
MapValue(FileEntry &FE) : V(&FE) {}
MapValue(MapEntry &ME) : V(&ME) {}
};
private:
StringRef Name;
const FileEntry *Entry;
friend class FileManager;
FileEntryRef() = delete;
explicit FileEntryRef(const MapEntry &Entry)
: Entry(&Entry) {
assert(Entry.second && "Expected payload");
assert(Entry.second->V && "Expected non-null");
assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
}
const MapEntry *Entry;
};
/// Cached information about one file (either on disk
@@ -110,7 +136,6 @@ private:
class FileEntry {
friend class FileManager;
StringRef Name; // Name of the file.
std::string RealPathName; // Real path to the file; could be empty.
off_t Size; // File size in bytes.
time_t ModTime; // Modification time of file.
@@ -123,6 +148,14 @@ class FileEntry {
/// The open file, if it is owned by the \p FileEntry.
mutable std::unique_ptr<llvm::vfs::File> File;
// First access name for this FileEntry.
//
// This is Optional only to allow delayed construction (FileEntryRef has no
// default constructor). It should always have a value in practice.
//
// TODO: remote this once everyone that needs a name uses FileEntryRef.
Optional<FileEntryRef> LastRef;
public:
FileEntry()
: UniqueID(0, 0), IsNamedPipe(false), IsValid(false)
@@ -131,7 +164,9 @@ public:
FileEntry(const FileEntry &) = delete;
FileEntry &operator=(const FileEntry &) = delete;
StringRef getName() const { return Name; }
StringRef getName() const { return LastRef->getName(); }
FileEntryRef getLastRef() const { return *LastRef; }
StringRef tryGetRealPathName() const { return RealPathName; }
bool isValid() const { return IsValid; }
off_t getSize() const { return Size; }
@@ -212,26 +247,21 @@ class FileManager : public RefCountedBase<FileManager> {
llvm::StringMap<llvm::ErrorOr<DirectoryEntry &>, llvm::BumpPtrAllocator>
SeenDirEntries;
/// A reference to the file entry that is associated with a particular
/// filename, or a reference to another filename that should be looked up
/// instead of the accessed filename.
///
/// The reference to another filename is specifically useful for Redirecting
/// VFSs that use external names. In that case, the \c FileEntryRef returned
/// by the \c FileManager will have the external name, and not the name that
/// was used to lookup the file.
using SeenFileEntryOrRedirect =
llvm::PointerUnion<FileEntry *, const StringRef *>;
/// A cache that maps paths to file entries (either real or
/// virtual) we have looked up, or an error that occurred when we looked up
/// the file.
///
/// \see SeenDirEntries
llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
llvm::BumpPtrAllocator>
llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>, llvm::BumpPtrAllocator>
SeenFileEntries;
/// A mirror of SeenFileEntries to give fake answers for getBypassFile().
///
/// Don't bother hooking up a BumpPtrAllocator. This should be rarely used,
/// and only on error paths.
std::unique_ptr<llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>>
SeenBypassFileEntries;
/// The canonical names of files and directories .
llvm::DenseMap<const void *, llvm::StringRef> CanonicalNames;

106
clang/lib/Basic/FileManager.cpp
View File

@@ -212,11 +212,10 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
FileEntry *FE;
if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
return FileEntryRef(*SeenFileInsertResult.first);
return FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>());
}
// We've not seen this before. Fill it in.
@@ -268,26 +267,29 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
// This occurs when one dir is symlinked to another, for example.
FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
NamedFileEnt->second = &UFE;
// If the name returned by getStatValue is different than Filename, re-intern
// the name.
if (Status.getName() != Filename) {
auto &NewNamedFileEnt =
*SeenFileEntries.insert({Status.getName(), &UFE}).first;
assert((*NewNamedFileEnt.second).get<FileEntry *>() == &UFE &&
if (Status.getName() == Filename) {
// The name matches. Set the FileEntry.
NamedFileEnt->second = FileEntryRef::MapValue(UFE);
} else {
// Name mismatch. We need a redirect. First grab the actual entry we want
// to return.
auto &Redirection =
*SeenFileEntries.insert({Status.getName(), FileEntryRef::MapValue(UFE)})
.first;
assert(Redirection.second->V.is<FileEntry *>() &&
"filename redirected to a non-canonical filename?");
assert(Redirection.second->V.get<FileEntry *>() == &UFE &&
"filename from getStatValue() refers to wrong file");
InterndFileName = NewNamedFileEnt.first().data();
// In addition to re-interning the name, construct a redirecting seen file
// entry, that will point to the name the filesystem actually wants to use.
StringRef *Redirect = new (CanonicalNameStorage) StringRef(InterndFileName);
auto SeenFileInsertResultIt = SeenFileEntries.find(Filename);
assert(SeenFileInsertResultIt != SeenFileEntries.end() &&
"unexpected SeenFileEntries cache miss");
SeenFileInsertResultIt->second = Redirect;
NamedFileEnt = &*SeenFileInsertResultIt;
// Cache the redirection in the previously-inserted entry, still available
// in the tentative return value.
NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
// Fix the tentative return value.
NamedFileEnt = &Redirection;
}
FileEntryRef ReturnedRef(*NamedFileEnt);
if (UFE.isValid()) { // Already have an entry with this inode, return it.
// FIXME: this hack ensures that if we look up a file by a virtual path in
@@ -299,20 +301,20 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
if (DirInfo != UFE.Dir && Status.IsVFSMapped)
UFE.Dir = DirInfo;
// Always update the name to use the last name by which a file was accessed.
// FIXME: Neither this nor always using the first name is correct; we want
// to switch towards a design where we return a FileName object that
// Always update LastRef to the last name by which a file was accessed.
// FIXME: Neither this nor always using the first reference is correct; we
// want to switch towards a design where we return a FileName object that
// encapsulates both the name by which the file was accessed and the
// corresponding FileEntry.
// FIXME: The Name should be removed from FileEntry once all clients
// adopt FileEntryRef.
UFE.Name = InterndFileName;
// FIXME: LastRef should be removed from FileEntry once all clients adopt
// FileEntryRef.
UFE.LastRef = ReturnedRef;
return FileEntryRef(InterndFileName, UFE);
return ReturnedRef;
}
// Otherwise, we don't have this file yet, add it.
UFE.Name = InterndFileName;
UFE.LastRef = ReturnedRef;
UFE.Size = Status.getSize();
UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
UFE.Dir = DirInfo;
@@ -329,7 +331,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
// We should still fill the path even if we aren't opening the file.
fillRealPathName(&UFE, InterndFileName);
}
return FileEntryRef(InterndFileName, UFE);
return ReturnedRef;
}
const FileEntry *
@@ -341,12 +343,12 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
auto &NamedFileEnt = *SeenFileEntries.insert(
{Filename, std::errc::no_such_file_or_directory}).first;
if (NamedFileEnt.second) {
SeenFileEntryOrRedirect Value = *NamedFileEnt.second;
FileEntryRef::MapValue Value = *NamedFileEnt.second;
FileEntry *FE;
if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
return FE;
return getVirtualFile(*Value.get<const StringRef *>(), Size,
ModificationTime);
return &FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>())
.getFileEntry();
}
// We've not seen this before, or the file is cached as non-existent.
@@ -372,7 +374,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
Status.getUser(), Status.getGroup(), Size,
Status.getType(), Status.getPermissions());
NamedFileEnt.second = UFE;
NamedFileEnt.second = FileEntryRef::MapValue(*UFE);
// If we had already opened this file, close it now so we don't
// leak the descriptor. We're not going to use the file
@@ -381,6 +383,9 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
UFE->closeFile();
// If we already have an entry with this inode, return it.
//
// FIXME: Surely this should add a reference by the new name, and return
// it instead...
if (UFE->isValid())
return UFE;
@@ -390,10 +395,10 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
} else {
VirtualFileEntries.push_back(std::make_unique<FileEntry>());
UFE = VirtualFileEntries.back().get();
NamedFileEnt.second = UFE;
NamedFileEnt.second = FileEntryRef::MapValue(*UFE);
}
UFE->Name = InterndFileName;
UFE->LastRef = FileEntryRef(NamedFileEnt);
UFE->Size = Size;
UFE->ModTime = ModificationTime;
UFE->Dir = *DirInfo;
@@ -409,17 +414,30 @@ llvm::Optional<FileEntryRef> FileManager::getBypassFile(FileEntryRef VF) {
if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr))
return None;
// Fill it in from the stat.
if (!SeenBypassFileEntries)
SeenBypassFileEntries = std::make_unique<
llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>>();
// If we've already bypassed just use the existing one.
auto Insertion = SeenBypassFileEntries->insert(
{VF.getName(), std::errc::no_such_file_or_directory});
if (!Insertion.second)
return FileEntryRef(*Insertion.first);
// Fill in the new entry from the stat.
BypassFileEntries.push_back(std::make_unique<FileEntry>());
const FileEntry &VFE = VF.getFileEntry();
FileEntry &BFE = *BypassFileEntries.back();
BFE.Name = VFE.getName();
Insertion.first->second = FileEntryRef::MapValue(BFE);
BFE.LastRef = FileEntryRef(*Insertion.first);
BFE.Size = Status.getSize();
BFE.Dir = VFE.Dir;
BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
BFE.UID = NextFileUID++;
BFE.IsValid = true;
return FileEntryRef(VF.getName(), BFE);
// Save the entry in the bypass table and return.
return FileEntryRef(*Insertion.first);
}
bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
@@ -534,13 +552,13 @@ void FileManager::GetUniqueIDMapping(
UIDToFiles.resize(NextFileUID);
// Map file entries
for (llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
for (llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>,
llvm::BumpPtrAllocator>::const_iterator
FE = SeenFileEntries.begin(),
FEEnd = SeenFileEntries.end();
FE != FEEnd; ++FE)
if (llvm::ErrorOr<SeenFileEntryOrRedirect> Entry = FE->getValue()) {
if (const auto *FE = (*Entry).dyn_cast<FileEntry *>())
if (llvm::ErrorOr<FileEntryRef::MapValue> Entry = FE->getValue()) {
if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
UIDToFiles[FE->getUID()] = FE;
}

2
clang/lib/Basic/SourceManager.cpp
View File

@@ -702,7 +702,7 @@ const FileEntry *
SourceManager::bypassFileContentsOverride(const FileEntry &File) {
assert(isFileOverridden(&File));
llvm::Optional<FileEntryRef> BypassFile =
FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)

43
clang/unittests/Basic/FileManagerTest.cpp
View File

@@ -285,9 +285,18 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp");
StatCache->InjectFile("dir/f2.cpp", 42);
StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
// This unintuitive rename-the-file-on-stat behaviour supports how the
// RedirectingFileSystem VFS layer responds to stats. However, even if you
// have two layers, you should only get a single filename back. As such the
// following stat cache behaviour is not supported (the correct stat entry
// for a double-redirection would be "dir/f1.cpp") and the getFileRef below
// should assert.
StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp");
manager.setStatCache(std::move(StatCache));
// With F2, test accessing the non-redirected name first.
// With F1, test accessing the non-redirected name first.
auto F1 = manager.getFileRef("dir/f1.cpp");
auto F1Alias = manager.getFileRef("dir/f1-alias.cpp");
auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
@@ -301,6 +310,11 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
EXPECT_EQ(&F1->getFileEntry(), &F1Alias->getFileEntry());
EXPECT_EQ(&F1->getFileEntry(), &F1Alias2->getFileEntry());
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
EXPECT_DEATH((void)manager.getFileRef("dir/f1-alias-alias.cpp"),
"filename redirected to a non-canonical filename?");
#endif
// With F2, test accessing the redirected name first.
auto F2Alias = manager.getFileRef("dir/f2-alias.cpp");
auto F2 = manager.getFileRef("dir/f2.cpp");
@@ -487,29 +501,34 @@ TEST_F(FileManagerTest, getBypassFile) {
// Set up a virtual file with a different size than FakeStatCache uses.
const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0);
ASSERT_TRUE(File);
FileEntryRef Ref("/tmp/test", *File);
EXPECT_TRUE(Ref.isValid());
EXPECT_EQ(Ref.getSize(), 10);
const FileEntry &FE = *File;
EXPECT_TRUE(FE.isValid());
EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
unsigned VirtualUID = Ref.getUID();
EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
EXPECT_EQ(Ref.getUID(), VirtualUID);
EXPECT_EQ(Ref.getSize(), 10);
unsigned VirtualUID = FE.getUID();
EXPECT_EQ(
&FE,
&expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
EXPECT_EQ(FE.getUID(), VirtualUID);
EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
llvm::Optional<FileEntryRef> BypassRef = Manager.getBypassFile(Ref);
llvm::Optional<FileEntryRef> BypassRef =
Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
EXPECT_EQ(BypassRef->getName(), Ref.getName());
EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(&BypassRef->getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
EXPECT_NE(BypassRef->getSize(), Ref.getSize());
EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
EXPECT_EQ(
&FE,
&expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace