[clang][modules] Adopt FileEntryRef in the HeaderFileInfo block in PCM files
#67383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang ChangesThis patch adopts First, this patch removes the loop over Second, since we now no longer rely on Note that which (Reincarnation of D143414 and D142780.) Full diff: https://github.com/llvm/llvm-project/pull/67383.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index 115558bfeee4ed5..56cb093dd8c376f 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -311,9 +311,9 @@ class FileManager : public RefCountedBase<FileManager> {
bool makeAbsolutePath(SmallVectorImpl<char> &Path) const;
/// Produce an array mapping from the unique IDs assigned to each
- /// file to the corresponding FileEntry pointer.
- void GetUniqueIDMapping(
- SmallVectorImpl<const FileEntry *> &UIDToFiles) const;
+ /// file to the corresponding FileEntryRef.
+ void
+ GetUniqueIDMapping(SmallVectorImpl<OptionalFileEntryRef> &UIDToFiles) const;
/// Retrieve the canonical name for a given directory.
///
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index c80fbfd7433f5be..3b834610dd55ef8 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -612,24 +612,21 @@ FileManager::getNoncachedStatValue(StringRef Path,
}
void FileManager::GetUniqueIDMapping(
- SmallVectorImpl<const FileEntry *> &UIDToFiles) const {
+ SmallVectorImpl<OptionalFileEntryRef> &UIDToFiles) const {
UIDToFiles.clear();
UIDToFiles.resize(NextFileUID);
- // Map file entries
- for (llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>,
- llvm::BumpPtrAllocator>::const_iterator
- FE = SeenFileEntries.begin(),
- FEEnd = SeenFileEntries.end();
- FE != FEEnd; ++FE)
- if (llvm::ErrorOr<FileEntryRef::MapValue> Entry = FE->getValue()) {
- if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
- UIDToFiles[FE->getUID()] = FE;
- }
-
- // Map virtual file entries
- for (const auto &VFE : VirtualFileEntries)
- UIDToFiles[VFE->getUID()] = VFE;
+ for (const auto &Entry : SeenFileEntries) {
+ // Only return existing non-virtual files.
+ if (!Entry.getValue() || !Entry.getValue()->V.is<FileEntry *>())
+ continue;
+ FileEntryRef FE(Entry);
+ // Add this file if it's the first one with the UID, or if its name is
+ // better than the existing one.
+ OptionalFileEntryRef &ExistingFE = UIDToFiles[FE.getUID()];
+ if (!ExistingFE || FE.getName() < ExistingFE->getName())
+ ExistingFE = FE;
+ }
}
StringRef FileManager::getCanonicalName(DirectoryEntryRef Dir) {
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 216ca94111e156b..1d939a3f5e7a819 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -167,23 +167,23 @@ std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
const HeaderSearch &HS = PP.getHeaderSearchInfo();
- SmallVector<const FileEntry *, 16> FilesByUID;
+ SmallVector<OptionalFileEntryRef, 16> FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
if (FilesByUID.size() > HS.header_file_size())
FilesByUID.resize(HS.header_file_size());
for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
- const FileEntry *File = FilesByUID[UID];
+ OptionalFileEntryRef File = FilesByUID[UID];
if (!File)
continue;
const HeaderFileInfo *HFI =
- HS.getExistingFileInfo(File, /*WantExternal*/ false);
+ HS.getExistingFileInfo(*File, /*WantExternal*/ false);
if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
continue;
- for (const auto &KH : HS.findResolvedModulesForHeader(File)) {
+ for (const auto &KH : HS.findResolvedModulesForHeader(*File)) {
if (!KH.getModule())
continue;
ModulesToProcess.push_back(KH.getModule());
@@ -2003,14 +2003,14 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
}
}
- SmallVector<const FileEntry *, 16> FilesByUID;
+ SmallVector<OptionalFileEntryRef, 16> FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
if (FilesByUID.size() > HS.header_file_size())
FilesByUID.resize(HS.header_file_size());
for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
- const FileEntry *File = FilesByUID[UID];
+ OptionalFileEntryRef File = FilesByUID[UID];
if (!File)
continue;
@@ -2021,7 +2021,7 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
// from a different module; in that case, we rely on the module(s)
// containing the header to provide this information.
const HeaderFileInfo *HFI =
- HS.getExistingFileInfo(File, /*WantExternal*/!Chain);
+ HS.getExistingFileInfo(*File, /*WantExternal*/!Chain);
if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
continue;
@@ -2035,13 +2035,13 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
SavedStrings.push_back(Filename.data());
}
- bool Included = PP->alreadyIncluded(File);
+ bool Included = PP->alreadyIncluded(*File);
HeaderFileInfoTrait::key_type Key = {
- Filename, File->getSize(), getTimestampForOutput(File)
+ Filename, File->getSize(), getTimestampForOutput(*File)
};
HeaderFileInfoTrait::data_type Data = {
- *HFI, Included, HS.getModuleMap().findResolvedModulesForHeader(File), {}
+ *HFI, Included, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
};
Generator.insert(Key, Data, GeneratorTrait);
++NumHeaderSearchEntries;
|
You can test this locally with the following command:git-clang-format --diff feb7b1914d513c709b9e024dfed709bb889cc853 86f275550976e443f11252debd552e72a0d9e0c8 -- clang/include/clang/Basic/FileManager.h clang/lib/Basic/FileManager.cpp clang/lib/Serialization/ASTWriter.cppView the diff from clang-format here.diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 1d939a3f5e7a..ef9d5462f1c7 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2021,7 +2021,7 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
// from a different module; in that case, we rely on the module(s)
// containing the header to provide this information.
const HeaderFileInfo *HFI =
- HS.getExistingFileInfo(*File, /*WantExternal*/!Chain);
+ HS.getExistingFileInfo(*File, /*WantExternal*/ !Chain);
if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
continue;
@@ -2037,12 +2037,13 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
bool Included = PP->alreadyIncluded(*File);
- HeaderFileInfoTrait::key_type Key = {
- Filename, File->getSize(), getTimestampForOutput(*File)
- };
+ HeaderFileInfoTrait::key_type Key = {Filename, File->getSize(),
+ getTimestampForOutput(*File)};
HeaderFileInfoTrait::data_type Data = {
- *HFI, Included, HS.getModuleMap().findResolvedModulesForHeader(*File), {}
- };
+ *HFI,
+ Included,
+ HS.getModuleMap().findResolvedModulesForHeader(*File),
+ {}};
Generator.insert(Key, Data, GeneratorTrait);
++NumHeaderSearchEntries;
}
|
|
LGTM, possible to come up with some test case with multiple virtual entries to the same file that would now be serialized once? |
If you're asking because of this:
then I should probably clarify that the number of file entries we serialize doesn't change. The loop I removed didn't do anything that would affect the output: it was overwriting existing entry in I could create a test that checks that we only serialize one entry for each UID if you think that adds value. But I would do that as either a prep or a follow-up patch, since this PR doesn't affect that. I think that the only thing that makes sense to test in this patch is that we now serialize the most " |
benlangmuir
approved these changes
Sep 26, 2023
clang/lib/Basic/FileManager.cpp
Outdated
| for (const auto &VFE : VirtualFileEntries) | ||
| UIDToFiles[VFE->getUID()] = VFE; | ||
| for (const auto &Entry : SeenFileEntries) { | ||
| // Only return existing non-virtual files. |
There was a problem hiding this comment.
"virtual" is an overloaded term here; maybe "VFS-mapped"?
There was a problem hiding this comment.
Let's say "not redirected", that seems to be more commonly used.
legrosbuffle
pushed a commit
to legrosbuffle/llvm-project
that referenced
this pull request
Sep 29, 2023
…n PCM files (llvm#67383) This patch adopts `FileEntryRef` in the `HeaderFileInfo`-writing part of `ASTWriter`. First, this patch removes the loop over `FileManager::VirtualFileEntries`. It's redundant, since all virtual file entries are also present in `SeenFileEntries` and thus already in `UIDToFiles`. Second, since we now no longer rely on `FileEntry::getLastRef()`/`FileEntry::getName()`, this patch takes care to establish which path gets used for each UID by picking the `FileEntryRef` with the most "`<`" name (instead of just relying on the `StringMap` iteration order). Note that which `FileEntry`/`FileEntryRef` objects we pick for each UID for serialization into the `llvm::OnDiskChainedHashTable` doesn't really matter. The hash function only includes the file size and modification time. The file name only plays role during resolution of hash collisions, in which case it goes through `FileManager` and resolves to a `FileEntry` that gets pointer-compared with the queried `FileEntry`. (Reincarnation of [D143414](https://reviews.llvm.org/D143414) and [D142780](https://reviews.llvm.org/D142780).)
jansvoboda11
added a commit
that referenced
this pull request
Sep 29, 2023
This is the `ASTReader` counterpart to PR #67383.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adopts
FileEntryRefin theHeaderFileInfo-writing part ofASTWriter.First, this patch removes the loop over
FileManager::VirtualFileEntries. It's redundant, since all virtual file entries are also present inSeenFileEntriesand thus already inUIDToFiles.Second, since we now no longer rely on
FileEntry::getLastRef()/FileEntry::getName(), this patch takes care to establish which path gets used for each UID by picking theFileEntryRefwith the most "<" name (instead of just relying on theStringMapiteration order).Note that which
FileEntry/FileEntryRefobjects we pick for each UID for serialization into thellvm::OnDiskChainedHashTabledoesn't really matter. The hash function only includes the file size and modification time. The file name only plays role during resolution of hash collisions, in which case it goes throughFileManagerand resolves to aFileEntrythat gets pointer-compared with the queriedFileEntry.(Reincarnation of D143414 and D142780.)