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

[Nico Weber via cfe-commits]

With your patch reverted locally, at the same breakpoint I instead get

$ lsof -p 95842 | wc -l
      94

So your patch seems to increase number of open file handles by ~260%.

On Wed, Jan 23, 2019 at 10:27 AM Nico Weber <thakis at chromium.org> wrote:

> 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/67dd746c/attachment.html>