r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 19 05:37:46 PST 2018
Author: sammccall
Date: Mon Nov 19 05:37:46 2018
New Revision: 347205
URL: http://llvm.org/viewvc/llvm-project?rev=347205&view=rev
Log:
[FileManager] getFile(open=true) after getFile(open=false) should open the file.
Summary:
Old behavior is to just return the cached entry regardless of opened-ness.
That feels buggy (though I guess nobody ever actually needed this).
This came up in the context of clangd+clang-tidy integration: we're
going to getFile(open=false) to replay preprocessor actions obscured by
the preamble, but the compilation may subsequently getFile(open=true)
for non-preamble includes.
Reviewers: ilya-biryukov
Subscribers: ioeric, kadircet, cfe-commits
Differential Revision: https://reviews.llvm.org/D54691
Modified:
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp
Modified: cfe/trunk/include/clang/Basic/FileManager.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205&r1=347204&r2=347205&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/FileManager.h (original)
+++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018
@@ -70,14 +70,15 @@ class FileEntry {
bool IsNamedPipe;
bool InPCH;
bool IsValid; // Is this \c FileEntry initialized and valid?
+ bool DeferredOpen; // Created by getFile(OpenFile=0); may open later.
/// The open file, if it is owned by the \p FileEntry.
mutable std::unique_ptr<llvm::vfs::File> File;
public:
FileEntry()
- : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
- {}
+ : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
+ DeferredOpen(false) {}
FileEntry(const FileEntry &) = delete;
FileEntry &operator=(const FileEntry &) = delete;
Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205&r1=347204&r2=347205&view=diff
==============================================================================
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018
@@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St
*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
// See if there is already an entry in the map.
- if (NamedFileEnt.second)
- return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
- : NamedFileEnt.second;
+ if (NamedFileEnt.second) {
+ if (NamedFileEnt.second == NON_EXISTENT_FILE)
+ return nullptr;
+ // Entry exists: return it *unless* it wasn't opened and open is requested.
+ if (!(NamedFileEnt.second->DeferredOpen && openFile))
+ return NamedFileEnt.second;
+ // We previously stat()ed the file, but didn't open it: do that below.
+ // FIXME: the below does other redundant work too (stats the dir and file).
+ } else {
+ // By default, initialize it to invalid.
+ NamedFileEnt.second = NON_EXISTENT_FILE;
+ }
++NumFileCacheMisses;
- // By default, initialize it to invalid.
- NamedFileEnt.second = NON_EXISTENT_FILE;
-
// Get the null-terminated file name as stored as the key of the
// SeenFileEntries map.
StringRef InterndFileName = NamedFileEnt.first();
@@ -267,6 +273,7 @@ const FileEntry *FileManager::getFile(St
// It exists. See if we have already opened a file with the same inode.
// This occurs when one dir is symlinked to another, for example.
FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
+ UFE.DeferredOpen = !openFile;
NamedFileEnt.second = &UFE;
@@ -283,6 +290,23 @@ const FileEntry *FileManager::getFile(St
InterndFileName = NamedFileEnt.first().data();
}
+ // If we opened the file for the first time, record the resulting info.
+ // Do this even if the cache entry was valid, maybe we didn't previously open.
+ if (F && !UFE.File) {
+ if (auto PathName = F->getName()) {
+ llvm::SmallString<128> AbsPath(*PathName);
+ // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+ // but can be very expensive on real file systems.
+ // FIXME: the semantic of RealPathName is unclear, and the name might be
+ // misleading. We need to clean up the interface here.
+ makeAbsolutePath(AbsPath);
+ llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+ UFE.RealPathName = AbsPath.str();
+ }
+ UFE.File = std::move(F);
+ assert(!UFE.DeferredOpen && "we just opened it!");
+ }
+
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
@@ -313,21 +337,9 @@ const FileEntry *FileManager::getFile(St
UFE.UniqueID = Data.UniqueID;
UFE.IsNamedPipe = Data.IsNamedPipe;
UFE.InPCH = Data.InPCH;
- UFE.File = std::move(F);
UFE.IsValid = true;
+ // Note File and DeferredOpen were initialized above.
- if (UFE.File) {
- if (auto PathName = UFE.File->getName()) {
- llvm::SmallString<128> AbsPath(*PathName);
- // This is not the same as `VFS::getRealPath()`, which resolves symlinks
- // but can be very expensive on real file systems.
- // FIXME: the semantic of RealPathName is unclear, and the name might be
- // misleading. We need to clean up the interface here.
- makeAbsolutePath(AbsPath);
- llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
- UFE.RealPathName = AbsPath.str();
- }
- }
return &UFE;
}
@@ -398,6 +410,7 @@ FileManager::getVirtualFile(StringRef Fi
UFE->UID = NextFileUID++;
UFE->IsValid = true;
UFE->File.reset();
+ UFE->DeferredOpen = false;
return UFE;
}
Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=347205&r1=347204&r2=347205&view=diff
==============================================================================
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Mon Nov 19 05:37:46 2018
@@ -222,6 +222,33 @@ TEST_F(FileManagerTest, getFileReturnsNU
EXPECT_EQ(nullptr, file);
}
+// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is
+// opened for the second call.
+TEST_F(FileManagerTest, getFileDefersOpen) {
+ llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
+ new llvm::vfs::InMemoryFileSystem());
+ FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));
+ FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));
+ FileManager manager(options, FS);
+
+ const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);
+ ASSERT_TRUE(file != nullptr);
+ ASSERT_TRUE(file->isValid());
+ // "real path name" reveals whether the file was actually opened.
+ EXPECT_EQ("", file->tryGetRealPathName());
+
+ file = manager.getFile("/tmp/test", /*OpenFile=*/true);
+ ASSERT_TRUE(file != nullptr);
+ ASSERT_TRUE(file->isValid());
+ EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
+
+ // However we should never try to open a file previously opened as virtual.
+ ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
+ ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
+ file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
+ EXPECT_EQ("", file->tryGetRealPathName());
+}
+
// The following tests apply to Unix-like system only.
#ifndef _WIN32
More information about the cfe-commits
mailing list