Index: include/clang/Basic/FileManager.h =================================================================== --- include/clang/Basic/FileManager.h +++ include/clang/Basic/FileManager.h @@ -69,15 +69,14 @@ 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 File; public: FileEntry() - : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false), - DeferredOpen(false) {} + : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false) + {} FileEntry(const FileEntry &) = delete; FileEntry &operator=(const FileEntry &) = delete; Index: lib/Basic/FileManager.cpp =================================================================== --- lib/Basic/FileManager.cpp +++ lib/Basic/FileManager.cpp @@ -188,21 +188,15 @@ *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first; // See if there is already an entry in the map. - 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; - } + if (NamedFileEnt.second) + return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr + : NamedFileEnt.second; ++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(); @@ -240,7 +234,6 @@ // 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; @@ -257,15 +250,6 @@ 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()) - fillRealPathName(&UFE, *PathName); - 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 @@ -296,9 +280,13 @@ 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()) + fillRealPathName(&UFE, *PathName); + } return &UFE; } @@ -370,7 +358,6 @@ UFE->UID = NextFileUID++; UFE->IsValid = true; UFE->File.reset(); - UFE->DeferredOpen = false; return UFE; } Index: test/PCH/leakfiles =================================================================== --- test/PCH/leakfiles +++ test/PCH/leakfiles @@ -0,0 +1,29 @@ +// Test that compiling using a PCH doesn't leak file descriptors. +// https://bugs.chromium.org/p/chromium/issues/detail?id=924225 +// +// This test requires bash loops and ulimit. +// REQUIRES: shell +// UNSUPPORTED: win32 +// +// Set up source files. lib/lib.h includes lots of lib*.h files in that dir. +// client.c includes lib/lib.h, and also the individual files directly. +// +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: cd %t +// RUN: mkdir lib +// RUN: for i in {1..300}; do touch lib/lib$i.h; done +// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done +// RUN: echo "#include \"lib/lib.h\"" > client.c +// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done +// +// We want to verify that we don't hold all the files open at the same time. +// This is important e.g. on mac, which has a low default FD limit. +// RUN: ulimit -n 100 +// +// Test without PCH. +// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c +// +// Test with PCH. +// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c +// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only Index: unittests/Basic/FileManagerTest.cpp =================================================================== --- unittests/Basic/FileManagerTest.cpp +++ unittests/Basic/FileManagerTest.cpp @@ -221,33 +221,6 @@ 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 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_FALSE(file->isOpenForTests()); - - file = manager.getFile("/tmp/test", /*OpenFile=*/true); - ASSERT_TRUE(file != nullptr); - ASSERT_TRUE(file->isValid()); - EXPECT_TRUE(file->isOpenForTests()); - - // 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_FALSE(file->isOpenForTests()); -} - // The following tests apply to Unix-like system only. #ifndef _WIN32