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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
