[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
dexonsmith closed this revision. dexonsmith marked an inline comment as done. dexonsmith added a comment. r370546. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
bruno accepted this revision. bruno added a comment. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/include/clang/Basic/FileManager.h:320 + /// twice, you get two new file entries. + const FileEntry *getBypassFile(const FileEntry ); + bruno wrote: > Does it make sense to return/read from a FileEntryRef here? @arphaman, wdyt? SGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
dexonsmith updated this revision to Diff 217754.
dexonsmith added a comment.
Added a FileManagerTest and changed FileManager::getBypassFile to use
FileEntryRef.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66710/new/
https://reviews.llvm.org/D66710
Files:
clang/include/clang/Basic/FileManager.h
clang/include/clang/Basic/SourceManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/lib/Serialization/ASTReader.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -397,4 +397,54 @@
EXPECT_EQ((*file)->tryGetRealPathName(), ExpectedResult);
}
+TEST_F(FileManagerTest, getBypassFile) {
+ SmallString<64> CustomWorkingDir;
+#ifdef _WIN32
+ CustomWorkingDir = "C:/";
+#else
+ CustomWorkingDir = "/";
+#endif
+
+ auto FS = IntrusiveRefCntPtr(
+ new llvm::vfs::InMemoryFileSystem);
+ // setCurrentworkingdirectory must finish without error.
+ ASSERT_TRUE(!FS->setCurrentWorkingDirectory(CustomWorkingDir));
+
+ FileSystemOptions Opts;
+ FileManager Manager(Opts, FS);
+
+ // Inject fake files into the file system.
+ auto Cache = std::make_unique();
+ Cache->InjectDirectory("/tmp", 42);
+ Cache->InjectFile("/tmp/test", 43);
+ Manager.setStatCache(std::move(Cache));
+
+ // 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);
+
+ // 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);
+
+ // Bypass the file.
+ llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ ASSERT_TRUE(BypassRef);
+ EXPECT_TRUE(BypassRef->isValid());
+ EXPECT_EQ(BypassRef->getName(), Ref.getName());
+
+ // Check that it's different in the right ways.
+ EXPECT_NE(>getFileEntry(), File);
+ EXPECT_NE(BypassRef->getUID(), VirtualUID);
+ EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+
+ // The virtual file should still be returned when searching.
+ EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+}
+
} // anonymous namespace
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2315,19 +2315,14 @@
if ((!Overridden && !Transient) && SM.isFileOverridden(File)) {
if (Complain)
Error(diag::err_fe_pch_file_overridden, Filename);
-// After emitting the diagnostic, recover by disabling the override so
-// that the original file will be used.
-//
-// FIXME: This recovery is just as broken as the original state; there may
-// be another precompiled module that's using the overridden contents, or
-// we might be half way through parsing it. Instead, we should treat the
-// overridden contents as belonging to a separate FileEntry.
-SM.disableFileContentsOverride(File);
-// The FileEntry is a virtual file entry with the size of the contents
-// that would override the original contents. Set it to the original's
-// size/time.
-FileMgr.modifyFileEntry(const_cast(File),
-StoredSize, StoredTime);
+
+// After emitting the diagnostic, bypass the overriding file to recover
+// (this creates a separate FileEntry).
+File = SM.bypassFileContentsOverride(*File);
+if (!File) {
+ F.InputFilesLoaded[ID - 1] = InputFile::getNotFound();
+ return InputFile();
+}
}
bool IsOutOfDate = false;
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -669,17 +669,19 @@
getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
}
-void SourceManager::disableFileContentsOverride(const FileEntry *File) {
- if (!isFileOverridden(File))
-return;
-
- const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
- const_cast(IR)->replaceBuffer(nullptr);
- const_cast(IR)->ContentsEntry = IR->OrigEntry;
+const FileEntry *
+SourceManager::bypassFileContentsOverride(const FileEntry ) {
+ assert(isFileOverridden());
+ llvm::Optional BypassFile =
+ FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+
+ // If the file can't be found in the FS, give up.
+ if (!BypassFile)
+return nullptr;
- assert(OverriddenFilesInfo);
- OverriddenFilesInfo->OverriddenFiles.erase(File);
- OverriddenFilesInfo->OverriddenFilesWithBuffer.erase(File);
+ const
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
bruno added a comment. Nice! Is this something that can be tested for in `unittests/Basic/FileManagerTest.cpp`? Comment at: clang/include/clang/Basic/FileManager.h:320 + /// twice, you get two new file entries. + const FileEntry *getBypassFile(const FileEntry ); + Does it make sense to return/read from a FileEntryRef here? @arphaman, wdyt? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
dexonsmith updated this revision to Diff 217229.
dexonsmith added a comment.
New diff with full context.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66710/new/
https://reviews.llvm.org/D66710
Files:
clang/include/clang/Basic/FileManager.h
clang/include/clang/Basic/SourceManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/lib/Serialization/ASTReader.cpp
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2315,19 +2315,14 @@
if ((!Overridden && !Transient) && SM.isFileOverridden(File)) {
if (Complain)
Error(diag::err_fe_pch_file_overridden, Filename);
-// After emitting the diagnostic, recover by disabling the override so
-// that the original file will be used.
-//
-// FIXME: This recovery is just as broken as the original state; there may
-// be another precompiled module that's using the overridden contents, or
-// we might be half way through parsing it. Instead, we should treat the
-// overridden contents as belonging to a separate FileEntry.
-SM.disableFileContentsOverride(File);
-// The FileEntry is a virtual file entry with the size of the contents
-// that would override the original contents. Set it to the original's
-// size/time.
-FileMgr.modifyFileEntry(const_cast(File),
-StoredSize, StoredTime);
+
+// After emitting the diagnostic, bypass the overriding file to recover
+// (this creates a separate FileEntry).
+File = SM.bypassFileContentsOverride(*File);
+if (!File) {
+ F.InputFilesLoaded[ID - 1] = InputFile::getNotFound();
+ return InputFile();
+}
}
bool IsOutOfDate = false;
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -671,17 +671,17 @@
getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
}
-void SourceManager::disableFileContentsOverride(const FileEntry *File) {
- if (!isFileOverridden(File))
-return;
+const FileEntry *
+SourceManager::bypassFileContentsOverride(const FileEntry ) {
+ assert(isFileOverridden());
+ auto *BypassFile = FileMgr.getBypassFile(File);
- const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
- const_cast(IR)->replaceBuffer(nullptr);
- const_cast(IR)->ContentsEntry = IR->OrigEntry;
+ // If the file can't be found in the FS, give up.
+ if (!BypassFile)
+return nullptr;
- assert(OverriddenFilesInfo);
- OverriddenFilesInfo->OverriddenFiles.erase(File);
- OverriddenFilesInfo->OverriddenFilesWithBuffer.erase(File);
+ (void)getOrCreateContentCache(BypassFile);
+ return BypassFile;
}
void SourceManager::setFileIsTransient(const FileEntry *File) {
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -389,6 +389,24 @@
return UFE;
}
+const FileEntry *FileManager::getBypassFile(const FileEntry ) {
+ // Stat of the file and return nullptr if it doesn't exist.
+ llvm::vfs::Status Status;
+ if (getStatValue(VFE.Name, Status, /*isFile=*/true, /*F=*/nullptr))
+return nullptr;
+
+ // Fill it in from the stat.
+ BypassFileEntries.push_back(std::make_unique());
+ auto = *BypassFileEntries.back();
+ BFE.Name = VFE.Name;
+ BFE.Size = Status.getSize();
+ BFE.Dir = VFE.Dir;
+ BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
+ BFE.UID = NextFileUID++;
+ BFE.IsValid = true;
+ return
+}
+
bool FileManager::FixupRelativePath(SmallVectorImpl ) const {
StringRef pathRef(path.data(), path.size());
@@ -532,12 +550,6 @@
UIDToFiles[VFE->getUID()] = VFE.get();
}
-void FileManager::modifyFileEntry(FileEntry *File,
- off_t Size, time_t ModificationTime) {
- File->Size = Size;
- File->ModTime = ModificationTime;
-}
-
StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
// FIXME: use llvm::sys::fs::canonical() when it gets implemented
llvm::DenseMap::iterator Known
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -952,11 +952,12 @@
return false;
}
- /// Disable overridding the contents of a file, previously enabled
- /// with #overrideFileContents.
+ /// Bypass the overridden contents of a file. This creates a new FileEntry
+ /// and initializes the content cache for it. Returns nullptr if there is no
+ /// such file in the filesystem.
///
/// This should be called before parsing has begun.
- void disableFileContentsOverride(const FileEntry *File);
+ const
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
arphaman added a comment. Missing full context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66710/new/ https://reviews.llvm.org/D66710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs
dexonsmith created this revision.
dexonsmith added reviewers: rsmith, arphaman, akyrtzi, bruno.
Herald added a subscriber: ributzka.
If contents of a file that is part of a PCM are overridden when reading
it, but weren't overridden when the PCM was being built, the ASTReader
will emit an error. Now it creates a separate FileEntry for recovery,
bypassing the overridden content instead of discarding it. The
pre-existing testcase clang/test/PCH/remap-file-from-pch.cpp confirms
that the new recovery method works correctly.
This resolves a long-standing FIXME to avoid hypothetically invalidating
another precompiled module that's already using the overridden contents.
This also removes ContentCache-related API that would be unsafe to use
across `CompilerInstance`s in an implicit modules build. This helps to
unblock us sinking it from SourceManager into FileManager in the future,
which would allow us to delete `InMemoryModuleCache`.
https://reviews.llvm.org/D66710
Files:
clang/include/clang/Basic/FileManager.h
clang/include/clang/Basic/SourceManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/lib/Serialization/ASTReader.cpp
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2315,19 +2315,14 @@
if ((!Overridden && !Transient) && SM.isFileOverridden(File)) {
if (Complain)
Error(diag::err_fe_pch_file_overridden, Filename);
-// After emitting the diagnostic, recover by disabling the override so
-// that the original file will be used.
-//
-// FIXME: This recovery is just as broken as the original state; there may
-// be another precompiled module that's using the overridden contents, or
-// we might be half way through parsing it. Instead, we should treat the
-// overridden contents as belonging to a separate FileEntry.
-SM.disableFileContentsOverride(File);
-// The FileEntry is a virtual file entry with the size of the contents
-// that would override the original contents. Set it to the original's
-// size/time.
-FileMgr.modifyFileEntry(const_cast(File),
-StoredSize, StoredTime);
+
+// After emitting the diagnostic, bypass the overriding file to recover
+// (this creates a separate FileEntry).
+File = SM.bypassFileContentsOverride(*File);
+if (!File) {
+ F.InputFilesLoaded[ID - 1] = InputFile::getNotFound();
+ return InputFile();
+}
}
bool IsOutOfDate = false;
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -671,17 +671,17 @@
getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
}
-void SourceManager::disableFileContentsOverride(const FileEntry *File) {
- if (!isFileOverridden(File))
-return;
+const FileEntry *
+SourceManager::bypassFileContentsOverride(const FileEntry ) {
+ assert(isFileOverridden());
+ auto *BypassFile = FileMgr.getBypassFile(File);
- const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
- const_cast(IR)->replaceBuffer(nullptr);
- const_cast(IR)->ContentsEntry = IR->OrigEntry;
+ // If the file can't be found in the FS, give up.
+ if (!BypassFile)
+return nullptr;
- assert(OverriddenFilesInfo);
- OverriddenFilesInfo->OverriddenFiles.erase(File);
- OverriddenFilesInfo->OverriddenFilesWithBuffer.erase(File);
+ (void)getOrCreateContentCache(BypassFile);
+ return BypassFile;
}
void SourceManager::setFileIsTransient(const FileEntry *File) {
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -389,6 +389,24 @@
return UFE;
}
+const FileEntry *FileManager::getBypassFile(const FileEntry ) {
+ // Stat of the file and return nullptr if it doesn't exist.
+ llvm::vfs::Status Status;
+ if (getStatValue(VFE.Name, Status, /*isFile=*/true, /*F=*/nullptr))
+return nullptr;
+
+ // Fill it in from the stat.
+ BypassFileEntries.push_back(std::make_unique());
+ auto = *BypassFileEntries.back();
+ BFE.Name = VFE.Name;
+ BFE.Size = Status.getSize();
+ BFE.Dir = VFE.Dir;
+ BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
+ BFE.UID = NextFileUID++;
+ BFE.IsValid = true;
+ return
+}
+
bool FileManager::FixupRelativePath(SmallVectorImpl ) const {
StringRef pathRef(path.data(), path.size());
@@ -532,12 +550,6 @@
UIDToFiles[VFE->getUID()] = VFE.get();
}
-void FileManager::modifyFileEntry(FileEntry *File,
- off_t Size, time_t ModificationTime) {
- File->Size = Size;
- File->ModTime = ModificationTime;
-}
-
StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
// FIXME: use
