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 <tha...@chromium.org> wrote:
> On Wed, Jan 23, 2019 at 9:54 AM Sam McCall <sammcc...@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...
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
