Hello Sam, This commit broke build test step for on couple of our builders today:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/21573 http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14095 . . . Failing Tests (1): Clang-Unit :: Basic/./BasicTests.exe/FileManagerTest.getFileDefersOpen Please have a look ASAP? Thanks Galina On Mon, Nov 19, 2018 at 5:40 AM Sam McCall via cfe-commits < cfe-commits@lists.llvm.org> wrote: > 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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
