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

[Sam McCall via cfe-commits]

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.

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.

A stack trace at the relevant breakpoint might well be useful - can't
remember if there are lots of entry points here.

Cheers, Sam

On Wed, Jan 23, 2019, 16:38 Nico Weber <thakis at chromium.org wrote:

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