This is an archive of the discontinued LLVM Phabricator instance.

[FileManager] Revert r347205 to avoid PCH file-descriptor leak.
ClosedPublic

Authored by sammccall on Jan 24 2019, 9:45 AM.

Details

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.

Diff Detail

Event Timeline

sammccall created this revision.Jan 24 2019, 9:45 AM
thakis added inline comments.Jan 24 2019, 10:11 AM
lib/Basic/FileManager.cpp
295

Should this call fillRealPathName() (which was added after your change)?

thakis accepted this revision.Jan 24 2019, 10:13 AM

lgtm, but lebtm with call to fillRealPathName()

This revision is now accepted and ready to land.Jan 24 2019, 10:13 AM
sammccall marked an inline comment as done.

avoid revert of fillRealPathName() (thanks Nico!)

This revision was automatically updated to reflect the committed changes.
thakis added a comment.Aug 8 2019, 8:21 AM

A few weeks later, I realize that since the test doesn't have a ".test" extension, it doesn't run on any bots. Could you rename it to leakfiles.test (or any other fitting extension listed in clang/test/lit.cfg.py's config.suffixes list)?

Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 8:21 AM