[clangd-dev] r352079 - [FileManager] Revert r347205 to avoid PCH file-descriptor leak.
Sam McCall via clangd-dev
clangd-dev at lists.llvm.org
Mon Jan 28 01:14:14 PST 2019
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 <hans at 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 <hans at 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 at 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 at lists.llvm.org
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/clangd-dev/attachments/20190128/bf92850a/attachment-0001.html>
More information about the clangd-dev
mailing list