<div dir="ltr"><div dir="ltr">This is kind of expected: r347205 was a bugfix.</div><div dir="ltr">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).</div><div><br></div><div>One mystery: why are the tests failing on the branch but not on trunk?</div><div>I do want to dig into this today/tomorrow, but I don't think it affects what we do on the branch.</div><div><br></div><div>The right fix for the branch is just to delete the three failing assertions (for locations 5, 6, 7 in that test).</div><div>Is it possible to do this directly on the branch (without deleting them on trunk)?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jan 26, 2019 at 1:30 AM Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Sorry to bring more bad news, but merging this seems to have regressed<br>
a clangd test on the branch (I didn't notice at the time, because I<br>
ran the wrong set of tests, d'oh).<br>
<br>
I've searched my inbox but couldn't find any recent commits touching<br>
the test. Do you have any idea what might be happening?<br>
<br>
******************** TEST 'Extra Tools Unit Tests ::<br>
clangd/./ClangdTests/GoToInclude.All' FAILED ********************<br>
Note: Google Test filter = GoToInclude.All<br>
[==========] Running 1 test from 1 test case.<br>
[----------] Global test environment set-up.<br>
[----------] 1 test from GoToInclude<br>
[ RUN ] GoToInclude.All<br>
/work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1086:<br>
Failure<br>
Value of: *Locations<br>
Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0)<br>
Actual: {}<br>
/work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1096:<br>
Failure<br>
Value of: *Locations<br>
Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0)<br>
Actual: {}<br>
/work/llvm-8/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:1101:<br>
Failure<br>
Value of: *Locations<br>
Expected: has 1 element that file range ("/clangd-test/foo.h", 0:0-0:0)<br>
Actual: {}<br>
[ FAILED ] GoToInclude.All (14 ms)<br>
[----------] 1 test from GoToInclude (14 ms total)<br>
<br>
[----------] Global test environment tear-down<br>
[==========] 1 test from 1 test case ran. (14 ms total)<br>
[ PASSED ] 0 tests.<br>
[ FAILED ] 1 test, listed below:<br>
[ FAILED ] GoToInclude.All<br>
<br>
1 FAILED TEST<br>
Updating file /clangd-test/foo.h with command [/clangd-test] clang<br>
-ffreestanding /clangd-test/foo.h<br>
Updating file /clangd-test/foo.cpp with command [/clangd-test] clang<br>
-ffreestanding /clangd-test/foo.cpp<br>
Preamble for file /clangd-test/foo.h cannot be reused. Attempting to rebuild it.<br>
Preamble for file /clangd-test/foo.cpp cannot be reused. Attempting to<br>
rebuild it.<br>
Built preamble of size 169108 for file /clangd-test/foo.h<br>
Built preamble of size 173056 for file /clangd-test/foo.cpp<br>
<br>
********************<br>
Testing Time: 5.30s<br>
********************<br>
Failing Tests (1):<br>
Extra Tools Unit Tests :: clangd/./ClangdTests/GoToInclude.All<br>
<br>
Expected Passes : 1123<br>
Expected Failures : 1<br>
Unsupported Tests : 3<br>
Unexpected Failures: 1<br>
<br>
On Fri, Jan 25, 2019 at 10:18 AM Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
><br>
> Merged to 8.0 in r352225.<br>
><br>
> On Thu, Jan 24, 2019 at 10:55 AM Sam McCall via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
> ><br>
> > Author: sammccall<br>
> > Date: Thu Jan 24 10:55:24 2019<br>
> > New Revision: 352079<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=352079&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=352079&view=rev</a><br>
> > Log:<br>
> > [FileManager] Revert r347205 to avoid PCH file-descriptor leak.<br>
> ><br>
> > Summary:<br>
> > r347205 fixed a bug in FileManager: first calling<br>
> > getFile(shouldOpen=false) and then getFile(shouldOpen=true) results in<br>
> > the file not being open.<br>
> ><br>
> > Unfortunately, some code was (inadvertently?) relying on this bug: when<br>
> > building with a PCH, the file entries are obtained first by passing<br>
> > shouldOpen=false, and then later shouldOpen=true, without any intention<br>
> > of reading them. After r347205, they do get unneccesarily opened.<br>
> > Aside from extra operations, this means they need to be closed. Normally<br>
> > files are closed when their contents are read. As these files are never<br>
> > read, they stay open until clang exits. On platforms with a low<br>
> > open-files limit (e.g. Mac), this can lead to spurious file-not-found<br>
> > errors when building large projects with PCH enabled, e.g.<br>
> > <a href="https://bugs.chromium.org/p/chromium/issues/detail?id=924225" rel="noreferrer" target="_blank">https://bugs.chromium.org/p/chromium/issues/detail?id=924225</a><br>
> ><br>
> > Fixing the callsites to pass shouldOpen=false when the file won't be<br>
> > read is not quite trivial (that info isn't available at the direct<br>
> > callsite), and passing shouldOpen=false is a performance regression (it<br>
> > results in open+fstat pairs being replaced by stat+open).<br>
> ><br>
> > So an ideal fix is going to be a little risky and we need some fix soon<br>
> > (especially for the llvm 8 branch).<br>
> > The problem addressed by r347205 is rare and has only been observed in<br>
> > clangd. It was present in llvm-7, so we can live with it for now.<br>
> ><br>
> > Reviewers: bkramer, thakis<br>
> ><br>
> > Subscribers: ilya-biryukov, ioeric, kadircet, cfe-commits<br>
> ><br>
> > Differential Revision: <a href="https://reviews.llvm.org/D57165" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57165</a><br>
> ><br>
> > Added:<br>
> > cfe/trunk/test/PCH/leakfiles<br>
> > Modified:<br>
> > cfe/trunk/include/clang/Basic/FileManager.h<br>
> > cfe/trunk/lib/Basic/FileManager.cpp<br>
> > cfe/trunk/unittests/Basic/FileManagerTest.cpp<br>
> ><br>
> > Modified: cfe/trunk/include/clang/Basic/FileManager.h<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=352079&r1=352078&r2=352079&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=352079&r1=352078&r2=352079&view=diff</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/include/clang/Basic/FileManager.h (original)<br>
> > +++ cfe/trunk/include/clang/Basic/FileManager.h Thu Jan 24 10:55:24 2019<br>
> > @@ -69,15 +69,14 @@ class FileEntry {<br>
> > bool IsNamedPipe;<br>
> > bool InPCH;<br>
> > bool IsValid; // Is this \c FileEntry initialized and valid?<br>
> > - bool DeferredOpen; // Created by getFile(OpenFile=0); may open later.<br>
> ><br>
> > /// The open file, if it is owned by the \p FileEntry.<br>
> > mutable std::unique_ptr<llvm::vfs::File> File;<br>
> ><br>
> > public:<br>
> > FileEntry()<br>
> > - : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),<br>
> > - DeferredOpen(false) {}<br>
> > + : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)<br>
> > + {}<br>
> ><br>
> > FileEntry(const FileEntry &) = delete;<br>
> > FileEntry &operator=(const FileEntry &) = delete;<br>
> ><br>
> > Modified: cfe/trunk/lib/Basic/FileManager.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=352079&r1=352078&r2=352079&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=352079&r1=352078&r2=352079&view=diff</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/lib/Basic/FileManager.cpp (original)<br>
> > +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Jan 24 10:55:24 2019<br>
> > @@ -188,21 +188,15 @@ const FileEntry *FileManager::getFile(St<br>
> > *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;<br>
> ><br>
> > // See if there is already an entry in the map.<br>
> > - if (NamedFileEnt.second) {<br>
> > - if (NamedFileEnt.second == NON_EXISTENT_FILE)<br>
> > - return nullptr;<br>
> > - // Entry exists: return it *unless* it wasn't opened and open is requested.<br>
> > - if (!(NamedFileEnt.second->DeferredOpen && openFile))<br>
> > - return NamedFileEnt.second;<br>
> > - // We previously stat()ed the file, but didn't open it: do that below.<br>
> > - // FIXME: the below does other redundant work too (stats the dir and file).<br>
> > - } else {<br>
> > - // By default, initialize it to invalid.<br>
> > - NamedFileEnt.second = NON_EXISTENT_FILE;<br>
> > - }<br>
> > + if (NamedFileEnt.second)<br>
> > + return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr<br>
> > + : NamedFileEnt.second;<br>
> ><br>
> > ++NumFileCacheMisses;<br>
> ><br>
> > + // By default, initialize it to invalid.<br>
> > + NamedFileEnt.second = NON_EXISTENT_FILE;<br>
> > +<br>
> > // Get the null-terminated file name as stored as the key of the<br>
> > // SeenFileEntries map.<br>
> > StringRef InterndFileName = NamedFileEnt.first();<br>
> > @@ -240,7 +234,6 @@ const FileEntry *FileManager::getFile(St<br>
> > // It exists. See if we have already opened a file with the same inode.<br>
> > // This occurs when one dir is symlinked to another, for example.<br>
> > FileEntry &UFE = UniqueRealFiles[Data.UniqueID];<br>
> > - UFE.DeferredOpen = !openFile;<br>
> ><br>
> > NamedFileEnt.second = &UFE;<br>
> ><br>
> > @@ -257,15 +250,6 @@ const FileEntry *FileManager::getFile(St<br>
> > InterndFileName = NamedFileEnt.first().data();<br>
> > }<br>
> ><br>
> > - // If we opened the file for the first time, record the resulting info.<br>
> > - // Do this even if the cache entry was valid, maybe we didn't previously open.<br>
> > - if (F && !UFE.File) {<br>
> > - if (auto PathName = F->getName())<br>
> > - fillRealPathName(&UFE, *PathName);<br>
> > - UFE.File = std::move(F);<br>
> > - assert(!UFE.DeferredOpen && "we just opened it!");<br>
> > - }<br>
> > -<br>
> > if (UFE.isValid()) { // Already have an entry with this inode, return it.<br>
> ><br>
> > // FIXME: this hack ensures that if we look up a file by a virtual path in<br>
> > @@ -296,9 +280,13 @@ const FileEntry *FileManager::getFile(St<br>
> > UFE.UniqueID = Data.UniqueID;<br>
> > UFE.IsNamedPipe = Data.IsNamedPipe;<br>
> > UFE.InPCH = Data.InPCH;<br>
> > + UFE.File = std::move(F);<br>
> > UFE.IsValid = true;<br>
> > - // Note File and DeferredOpen were initialized above.<br>
> ><br>
> > + if (UFE.File) {<br>
> > + if (auto PathName = UFE.File->getName())<br>
> > + fillRealPathName(&UFE, *PathName);<br>
> > + }<br>
> > return &UFE;<br>
> > }<br>
> ><br>
> > @@ -370,7 +358,6 @@ FileManager::getVirtualFile(StringRef Fi<br>
> > UFE->UID = NextFileUID++;<br>
> > UFE->IsValid = true;<br>
> > UFE->File.reset();<br>
> > - UFE->DeferredOpen = false;<br>
> > return UFE;<br>
> > }<br>
> ><br>
> ><br>
> > Added: cfe/trunk/test/PCH/leakfiles<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/PCH/leakfiles?rev=352079&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/PCH/leakfiles?rev=352079&view=auto</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/test/PCH/leakfiles (added)<br>
> > +++ cfe/trunk/test/PCH/leakfiles Thu Jan 24 10:55:24 2019<br>
> > @@ -0,0 +1,29 @@<br>
> > +// Test that compiling using a PCH doesn't leak file descriptors.<br>
> > +// <a href="https://bugs.chromium.org/p/chromium/issues/detail?id=924225" rel="noreferrer" target="_blank">https://bugs.chromium.org/p/chromium/issues/detail?id=924225</a><br>
> > +//<br>
> > +// This test requires bash loops and ulimit.<br>
> > +// REQUIRES: shell<br>
> > +// UNSUPPORTED: win32<br>
> > +//<br>
> > +// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.<br>
> > +// client.c includes lib/lib.h, and also the individual files directly.<br>
> > +//<br>
> > +// RUN: rm -rf %t<br>
> > +// RUN: mkdir %t<br>
> > +// RUN: cd %t<br>
> > +// RUN: mkdir lib<br>
> > +// RUN: for i in {1..300}; do touch lib/lib$i.h; done<br>
> > +// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done<br>
> > +// RUN: echo "#include \"lib/lib.h\"" > client.c<br>
> > +// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done<br>
> > +//<br>
> > +// We want to verify that we don't hold all the files open at the same time.<br>
> > +// This is important e.g. on mac, which has a low default FD limit.<br>
> > +// RUN: ulimit -n 100<br>
> > +//<br>
> > +// Test without PCH.<br>
> > +// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c<br>
> > +//<br>
> > +// Test with PCH.<br>
> > +// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c<br>
> > +// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only<br>
> ><br>
> > Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=352079&r1=352078&r2=352079&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=352079&r1=352078&r2=352079&view=diff</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)<br>
> > +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Jan 24 10:55:24 2019<br>
> > @@ -221,33 +221,6 @@ TEST_F(FileManagerTest, getFileReturnsNU<br>
> > EXPECT_EQ(nullptr, file);<br>
> > }<br>
> ><br>
> > -// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is<br>
> > -// opened for the second call.<br>
> > -TEST_F(FileManagerTest, getFileDefersOpen) {<br>
> > - llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(<br>
> > - new llvm::vfs::InMemoryFileSystem());<br>
> > - FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));<br>
> > - FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));<br>
> > - FileManager manager(options, FS);<br>
> > -<br>
> > - const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);<br>
> > - ASSERT_TRUE(file != nullptr);<br>
> > - ASSERT_TRUE(file->isValid());<br>
> > - // "real path name" reveals whether the file was actually opened.<br>
> > - EXPECT_FALSE(file->isOpenForTests());<br>
> > -<br>
> > - file = manager.getFile("/tmp/test", /*OpenFile=*/true);<br>
> > - ASSERT_TRUE(file != nullptr);<br>
> > - ASSERT_TRUE(file->isValid());<br>
> > - EXPECT_TRUE(file->isOpenForTests());<br>
> > -<br>
> > - // However we should never try to open a file previously opened as virtual.<br>
> > - ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));<br>
> > - ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));<br>
> > - file = manager.getFile("/tmp/testv", /*OpenFile=*/true);<br>
> > - EXPECT_FALSE(file->isOpenForTests());<br>
> > -}<br>
> > -<br>
> > // The following tests apply to Unix-like system only.<br>
> ><br>
> > #ifndef _WIN32<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> > <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>