sammccall created this revision. sammccall added reviewers: thakis, ilya-biryukov, bkramer. Herald added a subscriber: cfe-commits.
We don't immediately need the file to be open, and this code does not ensure that it is closed. On at least some code paths (using headers from PCHs) the file is never closed and we accumulate open file descriptors. Prior to r347205, the file was not actually opened (despite shouldOpen=true) if the FileEntry already existed. In the PCH case, the entry existed (due to a previous call with shouldOpen=false) and so we got away with passing true. r347205 "fixed" that behavior of FileManager: if the file was previously statted but not opened, and then FileManager::get(shouldOpen=true) is called, then the file is opened. Subsequently we observe spurious "file not found" errors in projects that use large PCHs on platforms with low open-file limits: https://bugs.chromium.org/p/chromium/issues/detail?id=924225 The fix here has non-obvious effects on the sequence of FS operations, due to the subtle pattern of caches and side-effects in FileManager's usage in clang. The main observed effects (from instrumenting FileSystem) are: - It converts most open()+fstat() pairs for headers into a stat()+open() pair. This should be a performance loss: stat() is more expensive. - It avoids open()s of some files where we turn out not to need the contents. This should be a performance win: stat() is cheaper, and open() usually requires an additional close(). At least in simple tests without PCHs, I observe the same total for fstat+stat+open, e.g. fstat -51, stat +73, open -22. Repository: rC Clang https://reviews.llvm.org/D57150 Files: test/PCH/leakfiles Index: test/PCH/leakfiles =================================================================== --- /dev/null +++ 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: test/PCH/leakfiles =================================================================== --- /dev/null +++ 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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
