Thanks for looking into this. I've reverted the test assertions on the branch in r352354.
On Mon, Jan 28, 2019 at 1:14 AM Sam McCall <sam.mcc...@gmail.com> wrote: > > This is kind of expected: r347205 was a bugfix. > Reverting it causes a real bug, and the tests are failing. The bug is > acceptable for 8.0 - it affects only a marginal case (go-to-definition on a > #include *outside* the preamble section of a file that was also transitively > #included in the preamble). > > One mystery: why are the tests failing on the branch but not on trunk? > I do want to dig into this today/tomorrow, but I don't think it affects what > we do on the branch. > > The right fix for the branch is just to delete the three failing assertions > (for locations 5, 6, 7 in that test). > Is it possible to do this directly on the branch (without deleting them on > trunk)? > > On Sat, Jan 26, 2019 at 1:30 AM Hans Wennborg <h...@chromium.org> wrote: >> >> Sorry to bring more bad news, but merging this seems to have regressed >> a clangd test on the branch (I didn't notice at the time, because I >> ran the wrong set of tests, d'oh). >> >> I've searched my inbox but couldn't find any recent commits touching >> the test. Do you have any idea what might be happening? >> >> ******************** TEST 'Extra Tools Unit Tests :: >> clangd/./ClangdTests/GoToInclude.All' FAILED ******************** >> Note: Google Test filter = GoToInclude.All >> [==========] Running 1 test from 1 test case. >> [----------] Global test environment set-up. >> [----------] 1 test from GoToInclude >> [ RUN ] GoToInclude.All >> /work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1086: >> Failure >> Value of: *Locations >> Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0) >> Actual: {} >> /work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1096: >> Failure >> Value of: *Locations >> Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0) >> Actual: {} >> /work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1101: >> Failure >> Value of: *Locations >> Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0) >> Actual: {} >> [ FAILED ] GoToInclude.All (14 ms) >> [----------] 1 test from GoToInclude (14 ms total) >> >> [----------] Global test environment tear-down >> [==========] 1 test from 1 test case ran. (14 ms total) >> [ PASSED ] 0 tests. >> [ FAILED ] 1 test, listed below: >> [ FAILED ] GoToInclude.All >> >> 1 FAILED TEST >> Updating file /clangd-test/foo.h with command [/clangd-test] clang >> -ffreestanding /clangd-test/foo.h >> Updating file /clangd-test/foo.cpp with command [/clangd-test] clang >> -ffreestanding /clangd-test/foo.cpp >> Preamble for file /clangd-test/foo.h cannot be reused. Attempting to rebuild >> it. >> Preamble for file /clangd-test/foo.cpp cannot be reused. Attempting to >> rebuild it. >> Built preamble of size 169108 for file /clangd-test/foo.h >> Built preamble of size 173056 for file /clangd-test/foo.cpp >> >> ******************** >> Testing Time: 5.30s >> ******************** >> Failing Tests (1): >> Extra Tools Unit Tests :: clangd/./ClangdTests/GoToInclude.All >> >> Expected Passes : 1123 >> Expected Failures : 1 >> Unsupported Tests : 3 >> Unexpected Failures: 1 >> >> On Fri, Jan 25, 2019 at 10:18 AM Hans Wennborg <h...@chromium.org> wrote: >> > >> > Merged to 8.0 in r352225. >> > >> > On Thu, Jan 24, 2019 at 10:55 AM Sam McCall via cfe-commits >> > <cfe-commits@lists.llvm.org> wrote: >> > > >> > > Author: sammccall >> > > Date: Thu Jan 24 10:55:24 2019 >> > > New Revision: 352079 >> > > >> > > URL: http://llvm.org/viewvc/llvm-project?rev=352079&view=rev >> > > Log: >> > > [FileManager] Revert r347205 to avoid PCH file-descriptor leak. >> > > >> > > Summary: >> > > r347205 fixed a bug in FileManager: first calling >> > > getFile(shouldOpen=false) and then getFile(shouldOpen=true) results in >> > > the file not being open. >> > > >> > > Unfortunately, some code was (inadvertently?) relying on this bug: when >> > > building with a PCH, the file entries are obtained first by passing >> > > shouldOpen=false, and then later shouldOpen=true, without any intention >> > > of reading them. After r347205, they do get unneccesarily opened. >> > > Aside from extra operations, this means they need to be closed. Normally >> > > files are closed when their contents are read. As these files are never >> > > read, they stay open until clang exits. On platforms with a low >> > > open-files limit (e.g. Mac), this can lead to spurious file-not-found >> > > errors when building large projects with PCH enabled, e.g. >> > > https://bugs.chromium.org/p/chromium/issues/detail?id=924225 >> > > >> > > Fixing the callsites to pass shouldOpen=false when the file won't be >> > > read is not quite trivial (that info isn't available at the direct >> > > callsite), and passing shouldOpen=false is a performance regression (it >> > > results in open+fstat pairs being replaced by stat+open). >> > > >> > > So an ideal fix is going to be a little risky and we need some fix soon >> > > (especially for the llvm 8 branch). >> > > The problem addressed by r347205 is rare and has only been observed in >> > > clangd. It was present in llvm-7, so we can live with it for now. >> > > >> > > Reviewers: bkramer, thakis >> > > >> > > Subscribers: ilya-biryukov, ioeric, kadircet, cfe-commits >> > > >> > > Differential Revision: https://reviews.llvm.org/D57165 >> > > >> > > Added: >> > > cfe/trunk/test/PCH/leakfiles >> > > 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=352079&r1=352078&r2=352079&view=diff >> > > ============================================================================== >> > > --- cfe/trunk/include/clang/Basic/FileManager.h (original) >> > > +++ cfe/trunk/include/clang/Basic/FileManager.h Thu Jan 24 10:55:24 2019 >> > > @@ -69,15 +69,14 @@ 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), >> > > - DeferredOpen(false) {} >> > > + : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(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=352079&r1=352078&r2=352079&view=diff >> > > ============================================================================== >> > > --- cfe/trunk/lib/Basic/FileManager.cpp (original) >> > > +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Jan 24 10:55:24 2019 >> > > @@ -188,21 +188,15 @@ 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) { >> > > - 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 @@ 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; >> > > >> > > @@ -257,15 +250,6 @@ 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()) >> > > - 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 @@ 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()) >> > > + fillRealPathName(&UFE, *PathName); >> > > + } >> > > return &UFE; >> > > } >> > > >> > > @@ -370,7 +358,6 @@ FileManager::getVirtualFile(StringRef Fi >> > > UFE->UID = NextFileUID++; >> > > UFE->IsValid = true; >> > > UFE->File.reset(); >> > > - UFE->DeferredOpen = false; >> > > return UFE; >> > > } >> > > >> > > >> > > Added: cfe/trunk/test/PCH/leakfiles >> > > URL: >> > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/PCH/leakfiles?rev=352079&view=auto >> > > ============================================================================== >> > > --- cfe/trunk/test/PCH/leakfiles (added) >> > > +++ cfe/trunk/test/PCH/leakfiles Thu Jan 24 10:55:24 2019 >> > > @@ -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 >> > > >> > > Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp >> > > URL: >> > > http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=352079&r1=352078&r2=352079&view=diff >> > > ============================================================================== >> > > --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original) >> > > +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Jan 24 10:55:24 >> > > 2019 >> > > @@ -221,33 +221,6 @@ 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_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 >> > > >> > > >> > > _______________________________________________ >> > > cfe-commits mailing list >> > > cfe-commits@lists.llvm.org >> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits