r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 9:54 AM Sam McCall <sammccall at google.com> wrote:

> (Email is better than IRC if that's OK - I don't know this code that well
> so it takes me a while).
>
> 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().
> I guess the "pch"/"main" output is printed before the corresponding lines
> in run.sh?
>

Correct.


> Weird that we don't get any output from building the PCH, but I don't know
> well how PCH builds work.
>
> > It looks like FS->getCurrentWorkingDirectory() is set
> yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?
> I think so. The FileManager and the VFS each have their own concept of
> working directory, I guess for historical reasons.
> Making use of the VFS one but not the FileManager one seems reasonable.
>
> So the weirdness is that FileSystemStatCache::get() is returning true
> (i.e. file doesn't exist), when the file does exist.
> Possibilities:
> 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).
> 2) the FS.openFileForRead call failed (ultimately ::open, if FS is the
> real FS)
> 3) the OwnedFile->status() call failed (ultimately ::fstat)
> 4) I'm misreading the code somehow
>

::open() fails with errno == 24, EMFILE.

This log statement here:

diff --git a/clang/lib/Basic/FileSystemStatCache.cpp
b/clang/lib/Basic/FileSystemStatCache.cpp
index d29ebb750fc..63fc4780237 100644
--- a/clang/lib/Basic/FileSystemStatCache.cpp
+++ b/clang/lib/Basic/FileSystemStatCache.cpp
@@ -70,9 +70,13 @@ bool FileSystemStatCache::get(StringRef Path, FileData
&Data, bool isFile,
     //
     // Because of this, check to see if the file exists with 'open'.  If
the
     // open succeeds, use fstat to get the stat info.
-    auto OwnedFile = FS.openFileForRead(Path);
+    llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> OwnedFile =
+        FS.openFileForRead(Path);

     if (!OwnedFile) {
+if (Path.endswith("scheduling_lifecycle_state.h")) {
+fprintf(stderr, "hmm failed %s\n", OwnedFile.getError().message().c_str());
+}
       // If the open fails, our "stat" fails.
       R = CacheMissing;
     } else {


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.

`ulimit -n` on macOS is pretty small -- do you see how your patch could
cause clang to keep more file handles open?

Here's how many files clang had open when I had a breakpoint in that error
path:

$ lsof -p 91890 | wc -l
     343



>
> 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.
> 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...
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190123/efdcfb7a/attachment.html>