<div dir="auto">Thanks! given that we don't see an earlier stat, I guess these files were being treated as virtual (file metadata deserialized from PCH). Previously despite the open=true these never actually got opened, and that worked fine.<div dir="auto"><br></div><div dir="auto">I'm away from my computer but will verify later tonight or in the morning (CET) and try to find a fix. If it's not obvious, we should revert the patch at least on the release branch.</div><div dir="auto"><br></div><div dir="auto">A stack trace at the relevant breakpoint might well be useful - can't remember if there are lots of entry points here.</div><div dir="auto"><br></div><div dir="auto">Cheers, Sam</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 23, 2019, 16:38 Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr">With your patch reverted locally, at the same breakpoint I instead get<div><br></div><div><div>$ lsof -p 95842 | wc -l</div><div> 94</div></div><div><br></div><div>So your patch seems to increase number of open file handles by ~260%.</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="m_3334640080678745290gmail_attr">On Wed, Jan 23, 2019 at 10:27 AM Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank" rel="noreferrer">thakis@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"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Wed, Jan 23, 2019 at 9:54 AM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank" rel="noreferrer">sammccall@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>(Email is better than IRC if that's OK - I don't know this code that well so it takes me a while).</div><div><br></div><div>Thanks, that's definitely interesting and not what I expected. I thought every call sequence r347205 changed the behavior of would have resulted in two calls to getStatValue().</div><div>I guess the "pch"/"main" output is printed before the corresponding lines in run.sh?</div></div></div></div></blockquote><div><br></div><div>Correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Weird that we don't get any output from building the PCH, but I don't know well how PCH builds work.</div><div><br></div><div>> It looks like FS->getCurrentWorkingDirectory() is set yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?</div><div>I think so. The FileManager and the VFS each have their own concept of working directory, I guess for historical reasons.</div><div>Making use of the VFS one but not the FileManager one seems reasonable.</div><div><br></div><div>So the weirdness is that FileSystemStatCache::get() is returning true (i.e. file doesn't exist), when the file does exist.</div><div>Possibilities:</div><div>1) we're serving this result from the FileSystemStatCache (and maybe it's being poisoned in a PCH-related way somehow). Except as far as I can tell, FileSystemStatCache is always null (FileManager::setStateCache has no callers).</div><div>2) the FS.openFileForRead call failed (ultimately ::open, if FS is the real FS)</div><div>3) the OwnedFile->status() call failed (ultimately ::fstat)</div><div>4) I'm misreading the code somehow</div></div></div></div></blockquote><div><br></div><div>::open() fails with errno == 24, EMFILE. </div><div><br></div><div>This log statement here:</div><div><br></div><div><div>diff --git a/clang/lib/Basic/FileSystemStatCache.cpp b/clang/lib/Basic/FileSystemStatCache.cpp</div><div>index d29ebb750fc..63fc4780237 100644</div><div>--- a/clang/lib/Basic/FileSystemStatCache.cpp</div><div>+++ b/clang/lib/Basic/FileSystemStatCache.cpp</div><div>@@ -70,9 +70,13 @@ bool FileSystemStatCache::get(StringRef Path, FileData &Data, bool isFile,</div><div> //</div><div> // Because of this, check to see if the file exists with 'open'. If the</div><div> // open succeeds, use fstat to get the stat info.</div><div>- auto OwnedFile = FS.openFileForRead(Path);</div><div>+ llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> OwnedFile =</div><div>+ FS.openFileForRead(Path);</div><div> </div><div> if (!OwnedFile) {</div><div>+if (Path.endswith("scheduling_lifecycle_state.h")) {</div><div>+fprintf(stderr, "hmm failed %s\n", OwnedFile.getError().message().c_str());</div><div>+}</div><div> // If the open fails, our "stat" fails.</div><div> R = CacheMissing;</div><div> } else {</div></div><div><br></div><div><br></div><div>causes clang to print "hmm failed Too many open files". That path should maybe check if `OwnedFile.getError().value() == EMFILE && OwnedFile.getError().category() == std::generic_category()` on mac and abort or diag or something more visible.</div><div><br></div><div>`ulimit -n` on macOS is pretty small -- do you see how your patch could cause clang to keep more file handles open?</div><div><br></div><div>Here's how many files clang had open when I had a breakpoint in that error path:</div><div><br></div><div><div>$ lsof -p 91890 | wc -l</div><div> 343</div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>Could you find out which of these is going on? Either running in a debugger or adding some similar printfs to FileSystemStatCache::get() should be doable.</div><div>I'm also going to try to work out how the patch could have affected this, but new vs correct much easier for me to compare than new vs old...</div></div></div></div>
</blockquote></div></div></div></div></div></div></div></div>
</blockquote></div>
</blockquote></div>