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

[Sam McCall via cfe-commits]

Reverted as r352079, with a test derived from this thread.
https://bugs.llvm.org/show_bug.cgi?id=40448 to cherrypick to branch.
I'll work on relanding a fixed version soon.

Thanks again for investigating, and sorry for the trouble.

On Thu, Jan 24, 2019 at 5:05 PM Sam McCall  wrote:

> Sure. Op counts and linux benchmarks are easy, I could try to get access
> to mac/windows too...
>
> Building clang's HeaderSearch.cpp (without PCH) yields:
> master: fstat = 341, open = 1039, stat = 211
> openFile=false: fstat = 9 (-332), open = 379 (-660), stat = 1203 (+992)
> revert: fstat = 341 (+0), open = 1039 (+0), stat = 211 (+0)
> (this is on linux, but that shouldn't matter to the counts)
>
> Timings on my workstation (-fsyntax-only, disk caches flushed, warmed up
> by running clang with no args):
> master: 2.063
> openFile=false patch: 1.953
> revert: 1.946
> (stdev is around 0.07)
> So openFile=false a regression of 6% or so on linux, I don't think it's
> worth testing anything else :-\
>
> Best plan I have now is:
>  - revert r347205, disable tests that this breaks
>  - cherrypick the revert to 8.0 branch
>  - reapply r347205 + a version of this patch where the openFile value is
> plumbed through, with false for the PCH case.
> I'll send the revert out shortly. It should probably get reviewed since it
> won't be clean.
> The final patch is going to be ugly, but there's time to come up with a
> better idea.
>
> On Thu, Jan 24, 2019 at 3:26 PM Nico Weber  wrote:
>
>> Can you measure on macOS and Windows too, and maybe with a larger
>> program? The open+fstat (instead of stat+open) optimization was I think
>> done with benchmarking on macOS.
>>
>> I can see why you say you think the new behavior is more consistent, but
>> it's basically changing what information goes into the getFile() cache key.
>> Previously, openFile didn't go in there, and from that perspective saying
>> "the cache keeps the openFile passed the first time you call this for a
>> file" makes sense to me too.
>>
>> I'm happy with either before your patch or with getFileAndSuggestModule()
>> passing openFile=false if perf is comparable (which I expect it to be, to
>> be honest).
>>
>> Since the openFile=false change might be a bit risky, maybe we should
>> revert on 8.0 and do openFile=false only on trunk.
>>
>> On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:
>>
>>> So from my reading starting at getFileAndSuggestModule, changing to
>>> openFile=false is correct there unless some code is relying on that side
>>> effect.
>>> All the tests pass, too.
>>>
>>> The most likely regression is where we previously did open+fstat we'll
>>> now do stat+open+fstat.
>>>
>>> I tried running three commands using your repro, counting
>>> stat/open/fstat by instrumenting Unix/Path.inc.
>>> I used three versions of the code: old = before my patch, mid = with
>>> r347205, new = with r347205 and openFile=false.
>>>
>>> Build PCH:
>>>   stat: 331 -> 331 -> 635
>>>   open: 316 -> 314 -> 316
>>>   fstat: 311 -> 311 -> 8
>>>
>>> Use PCH:
>>>  stat: 322 -> 322 -> 924
>>>  open: 910 -> 609 -> 308
>>>  fstat: 607 -> 607 -> 5
>>>
>>> No PCH:
>>>  stat: 16 -> 16 -> 617
>>>  open: 606 -> 606 -> 306
>>>  fstat: 604 -> 604 -> 3
>>>
>>> Summary:
>>>  - r347205 was neutral for stats and fstats. It caused fewer files to be
>>> opened when building a PCH (though failed to close the ones it did). It
>>> caused file handles to leak when using a PCH.
>>>  - openFile=false reduces the number of files opened when compiling
>>> (with or without PCH). It also changes most fstats to stats, which is a
>>> loss.
>>>  - compared to the original state, having both patches seems like a
>>> small loss when building a PCH, and a small win when using one or compiling
>>> without one.
>>>
>>> So I'm tempted to check in the openFile=false.
>>> It seems more internally consistent (passing openFile=true leaves the
>>> file open), and avoids breaking clangd.
>>> It seems uninvasive, though as we've seen side-effects are hard to
>>> predict. Performance seems likely to be not-worse (vs reverting r347205).
>>>
>>> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall 
>>> wrote:
>>>
 Thanks for all the digging!
 I can reproduce locally with your last example and ulimit -n 1000.

 So it sounds like:
  - this *is* actually the interaction/behavior I expected from
 FileManager (we call first with openFile=false then with openFile=true, and
 the file is opened the second time)
  - the problem manifests because the file is opened but never closed,
 so we leak file descriptors, but closing the file would'nt be the best fix
 - we shouldn't open it if we don't need its content
  - I would call this a bug in the caller of file manager (in the stack
 we saw where getFileAndSuggestModule calls getFile), because it calls with
 openFile=true, but doesn't use the file contents nor ensure the file is
 closed. This was previously masked by the bug (fixed in my patch) 

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

[Sam McCall via cfe-commits]

Sure. Op counts and linux benchmarks are easy, I could try to get access to
mac/windows too...

Building clang's HeaderSearch.cpp (without PCH) yields:
master: fstat = 341, open = 1039, stat = 211
openFile=false: fstat = 9 (-332), open = 379 (-660), stat = 1203 (+992)
revert: fstat = 341 (+0), open = 1039 (+0), stat = 211 (+0)
(this is on linux, but that shouldn't matter to the counts)

Timings on my workstation (-fsyntax-only, disk caches flushed, warmed up by
running clang with no args):
master: 2.063
openFile=false patch: 1.953
revert: 1.946
(stdev is around 0.07)
So openFile=false a regression of 6% or so on linux, I don't think it's
worth testing anything else :-\

Best plan I have now is:
 - revert r347205, disable tests that this breaks
 - cherrypick the revert to 8.0 branch
 - reapply r347205 + a version of this patch where the openFile value is
plumbed through, with false for the PCH case.
I'll send the revert out shortly. It should probably get reviewed since it
won't be clean.
The final patch is going to be ugly, but there's time to come up with a
better idea.

On Thu, Jan 24, 2019 at 3:26 PM Nico Weber  wrote:

> Can you measure on macOS and Windows too, and maybe with a larger program?
> The open+fstat (instead of stat+open) optimization was I think done with
> benchmarking on macOS.
>
> I can see why you say you think the new behavior is more consistent, but
> it's basically changing what information goes into the getFile() cache key.
> Previously, openFile didn't go in there, and from that perspective saying
> "the cache keeps the openFile passed the first time you call this for a
> file" makes sense to me too.
>
> I'm happy with either before your patch or with getFileAndSuggestModule()
> passing openFile=false if perf is comparable (which I expect it to be, to
> be honest).
>
> Since the openFile=false change might be a bit risky, maybe we should
> revert on 8.0 and do openFile=false only on trunk.
>
> On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:
>
>> So from my reading starting at getFileAndSuggestModule, changing to
>> openFile=false is correct there unless some code is relying on that side
>> effect.
>> All the tests pass, too.
>>
>> The most likely regression is where we previously did open+fstat we'll
>> now do stat+open+fstat.
>>
>> I tried running three commands using your repro, counting stat/open/fstat
>> by instrumenting Unix/Path.inc.
>> I used three versions of the code: old = before my patch, mid = with
>> r347205, new = with r347205 and openFile=false.
>>
>> Build PCH:
>>   stat: 331 -> 331 -> 635
>>   open: 316 -> 314 -> 316
>>   fstat: 311 -> 311 -> 8
>>
>> Use PCH:
>>  stat: 322 -> 322 -> 924
>>  open: 910 -> 609 -> 308
>>  fstat: 607 -> 607 -> 5
>>
>> No PCH:
>>  stat: 16 -> 16 -> 617
>>  open: 606 -> 606 -> 306
>>  fstat: 604 -> 604 -> 3
>>
>> Summary:
>>  - r347205 was neutral for stats and fstats. It caused fewer files to be
>> opened when building a PCH (though failed to close the ones it did). It
>> caused file handles to leak when using a PCH.
>>  - openFile=false reduces the number of files opened when compiling (with
>> or without PCH). It also changes most fstats to stats, which is a loss.
>>  - compared to the original state, having both patches seems like a small
>> loss when building a PCH, and a small win when using one or compiling
>> without one.
>>
>> So I'm tempted to check in the openFile=false.
>> It seems more internally consistent (passing openFile=true leaves the
>> file open), and avoids breaking clangd.
>> It seems uninvasive, though as we've seen side-effects are hard to
>> predict. Performance seems likely to be not-worse (vs reverting r347205).
>>
>> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:
>>
>>> Thanks for all the digging!
>>> I can reproduce locally with your last example and ulimit -n 1000.
>>>
>>> So it sounds like:
>>>  - this *is* actually the interaction/behavior I expected from
>>> FileManager (we call first with openFile=false then with openFile=true, and
>>> the file is opened the second time)
>>>  - the problem manifests because the file is opened but never closed, so
>>> we leak file descriptors, but closing the file would'nt be the best fix -
>>> we shouldn't open it if we don't need its content
>>>  - I would call this a bug in the caller of file manager (in the stack
>>> we saw where getFileAndSuggestModule calls getFile), because it calls with
>>> openFile=true, but doesn't use the file contents nor ensure the file is
>>> closed. This was previously masked by the bug (fixed in my patch) where
>>> openFile=true could result in not opening the file.
>>>  - nevertheless if I can't find a fairly safe fix soon then we'll need
>>> to revert this patch.
>>>
>>> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>>>
 For completeness, in case the construction isn't clear, here's a full
 repro of the symptom.

 $ cat pch.cc
 $ cat pch.h
 #include "foo.h"
 $ rm bar/foo.h
 $ for i in 

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

[Nico Weber via cfe-commits]

Can you measure on macOS and Windows too, and maybe with a larger program?
The open+fstat (instead of stat+open) optimization was I think done with
benchmarking on macOS.

I can see why you say you think the new behavior is more consistent, but
it's basically changing what information goes into the getFile() cache key.
Previously, openFile didn't go in there, and from that perspective saying
"the cache keeps the openFile passed the first time you call this for a
file" makes sense to me too.

I'm happy with either before your patch or with getFileAndSuggestModule()
passing openFile=false if perf is comparable (which I expect it to be, to
be honest).

Since the openFile=false change might be a bit risky, maybe we should
revert on 8.0 and do openFile=false only on trunk.

On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:

> So from my reading starting at getFileAndSuggestModule, changing to
> openFile=false is correct there unless some code is relying on that side
> effect.
> All the tests pass, too.
>
> The most likely regression is where we previously did open+fstat we'll now
> do stat+open+fstat.
>
> I tried running three commands using your repro, counting stat/open/fstat
> by instrumenting Unix/Path.inc.
> I used three versions of the code: old = before my patch, mid = with
> r347205, new = with r347205 and openFile=false.
>
> Build PCH:
>   stat: 331 -> 331 -> 635
>   open: 316 -> 314 -> 316
>   fstat: 311 -> 311 -> 8
>
> Use PCH:
>  stat: 322 -> 322 -> 924
>  open: 910 -> 609 -> 308
>  fstat: 607 -> 607 -> 5
>
> No PCH:
>  stat: 16 -> 16 -> 617
>  open: 606 -> 606 -> 306
>  fstat: 604 -> 604 -> 3
>
> Summary:
>  - r347205 was neutral for stats and fstats. It caused fewer files to be
> opened when building a PCH (though failed to close the ones it did). It
> caused file handles to leak when using a PCH.
>  - openFile=false reduces the number of files opened when compiling (with
> or without PCH). It also changes most fstats to stats, which is a loss.
>  - compared to the original state, having both patches seems like a small
> loss when building a PCH, and a small win when using one or compiling
> without one.
>
> So I'm tempted to check in the openFile=false.
> It seems more internally consistent (passing openFile=true leaves the file
> open), and avoids breaking clangd.
> It seems uninvasive, though as we've seen side-effects are hard to
> predict. Performance seems likely to be not-worse (vs reverting r347205).
>
> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:
>
>> Thanks for all the digging!
>> I can reproduce locally with your last example and ulimit -n 1000.
>>
>> So it sounds like:
>>  - this *is* actually the interaction/behavior I expected from
>> FileManager (we call first with openFile=false then with openFile=true, and
>> the file is opened the second time)
>>  - the problem manifests because the file is opened but never closed, so
>> we leak file descriptors, but closing the file would'nt be the best fix -
>> we shouldn't open it if we don't need its content
>>  - I would call this a bug in the caller of file manager (in the stack we
>> saw where getFileAndSuggestModule calls getFile), because it calls with
>> openFile=true, but doesn't use the file contents nor ensure the file is
>> closed. This was previously masked by the bug (fixed in my patch) where
>> openFile=true could result in not opening the file.
>>  - nevertheless if I can't find a fairly safe fix soon then we'll need to
>> revert this patch.
>>
>> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>>
>>> For completeness, in case the construction isn't clear, here's a full
>>> repro of the symptom.
>>>
>>> $ cat pch.cc
>>> $ cat pch.h
>>> #include "foo.h"
>>> $ rm bar/foo.h
>>> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
>>> $ for i in {1..300}; do touch bar/foo$i.h; done
>>> $ cat client.cc
>>> #include "bar/foo.h"
>>> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc;
>>> done
>>>
>>> With today's trunk, on macOS:
>>>
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>>> /FIpch.h pch.cc  /Ibar
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>>> /FIpch.h client.cc /Ibar
>>> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
>>> #include "bar/foo252.h"
>>>  ^~
>>> 1 error generated.
>>>
>>> This works fine with this patch undone.
>>>
>>> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>>>
 This might finally be a repro!

 Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
 foo.h foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
 -bash: type: foo.h: not found
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
 foo.h foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
 #include "foo2.h"
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
 #include "foo.h"
 

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

[Sam McCall via cfe-commits]

So from my reading starting at getFileAndSuggestModule, changing to
openFile=false is correct there unless some code is relying on that side
effect.
All the tests pass, too.

The most likely regression is where we previously did open+fstat we'll now
do stat+open+fstat.

I tried running three commands using your repro, counting stat/open/fstat
by instrumenting Unix/Path.inc.
I used three versions of the code: old = before my patch, mid = with
r347205, new = with r347205 and openFile=false.

Build PCH:
  stat: 331 -> 331 -> 635
  open: 316 -> 314 -> 316
  fstat: 311 -> 311 -> 8

Use PCH:
 stat: 322 -> 322 -> 924
 open: 910 -> 609 -> 308
 fstat: 607 -> 607 -> 5

No PCH:
 stat: 16 -> 16 -> 617
 open: 606 -> 606 -> 306
 fstat: 604 -> 604 -> 3

Summary:
 - r347205 was neutral for stats and fstats. It caused fewer files to be
opened when building a PCH (though failed to close the ones it did). It
caused file handles to leak when using a PCH.
 - openFile=false reduces the number of files opened when compiling (with
or without PCH). It also changes most fstats to stats, which is a loss.
 - compared to the original state, having both patches seems like a small
loss when building a PCH, and a small win when using one or compiling
without one.

So I'm tempted to check in the openFile=false.
It seems more internally consistent (passing openFile=true leaves the file
open), and avoids breaking clangd.
It seems uninvasive, though as we've seen side-effects are hard to predict.
Performance seems likely to be not-worse (vs reverting r347205).

On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:

> Thanks for all the digging!
> I can reproduce locally with your last example and ulimit -n 1000.
>
> So it sounds like:
>  - this *is* actually the interaction/behavior I expected from FileManager
> (we call first with openFile=false then with openFile=true, and the file is
> opened the second time)
>  - the problem manifests because the file is opened but never closed, so
> we leak file descriptors, but closing the file would'nt be the best fix -
> we shouldn't open it if we don't need its content
>  - I would call this a bug in the caller of file manager (in the stack we
> saw where getFileAndSuggestModule calls getFile), because it calls with
> openFile=true, but doesn't use the file contents nor ensure the file is
> closed. This was previously masked by the bug (fixed in my patch) where
> openFile=true could result in not opening the file.
>  - nevertheless if I can't find a fairly safe fix soon then we'll need to
> revert this patch.
>
> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>
>> For completeness, in case the construction isn't clear, here's a full
>> repro of the symptom.
>>
>> $ cat pch.cc
>> $ cat pch.h
>> #include "foo.h"
>> $ rm bar/foo.h
>> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
>> $ for i in {1..300}; do touch bar/foo$i.h; done
>> $ cat client.cc
>> #include "bar/foo.h"
>> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc; done
>>
>> With today's trunk, on macOS:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc /Ibar
>> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
>> #include "bar/foo252.h"
>>  ^~
>> 1 error generated.
>>
>> This works fine with this patch undone.
>>
>> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>>
>>> This might finally be a repro!
>>>
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>>> foo.h foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
>>> -bash: type: foo.h: not found
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>>> foo.h foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
>>> #include "foo2.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>>> #include "foo.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>>> #include "bar/foo.h"
>>> #include "bar/foo2.h"
>>>
>>> #include "foo3.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>>> /FIpch.h pch.cc  /Ibar
>>> stat pch.cc 1
>>> opened pch.cc
>>> closing pch.cc
>>> stat ./pch.h 1
>>> opened ./pch.h
>>> closing ./pch.h
>>> stat ./foo.h 1
>>> stat bar/foo.h 1
>>> opened bar/foo.h
>>> closing bar/foo.h
>>> stat bar/foo2.h 1
>>> opened bar/foo2.h
>>> closing bar/foo2.h
>>> stat pch.cc 1
>>> opened pch.cc
>>> stat pch.pch 1
>>> opened pch.pch
>>> closing pch.pch
>>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>>> stat ./pch.h 1
>>> opened ./pch.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>>> 

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

[Sam McCall via cfe-commits]

Thanks for all the digging!
I can reproduce locally with your last example and ulimit -n 1000.

So it sounds like:
 - this *is* actually the interaction/behavior I expected from FileManager
(we call first with openFile=false then with openFile=true, and the file is
opened the second time)
 - the problem manifests because the file is opened but never closed, so we
leak file descriptors, but closing the file would'nt be the best fix - we
shouldn't open it if we don't need its content
 - I would call this a bug in the caller of file manager (in the stack we
saw where getFileAndSuggestModule calls getFile), because it calls with
openFile=true, but doesn't use the file contents nor ensure the file is
closed. This was previously masked by the bug (fixed in my patch) where
openFile=true could result in not opening the file.
 - nevertheless if I can't find a fairly safe fix soon then we'll need to
revert this patch.

On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:

> For completeness, in case the construction isn't clear, here's a full
> repro of the symptom.
>
> $ cat pch.cc
> $ cat pch.h
> #include "foo.h"
> $ rm bar/foo.h
> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
> $ for i in {1..300}; do touch bar/foo$i.h; done
> $ cat client.cc
> #include "bar/foo.h"
> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc; done
>
> With today's trunk, on macOS:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc  /Ibar
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc /Ibar
> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
> #include "bar/foo252.h"
>  ^~
> 1 error generated.
>
> This works fine with this patch undone.
>
> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>
>> This might finally be a repro!
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>> foo.h foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
>> -bash: type: foo.h: not found
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>> foo.h foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
>> #include "foo2.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>> #include "foo.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>> #include "bar/foo.h"
>> #include "bar/foo2.h"
>>
>> #include "foo3.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> stat bar/foo.h 1
>> opened bar/foo.h
>> closing bar/foo.h
>> stat bar/foo2.h 1
>> opened bar/foo2.h
>> closing bar/foo2.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 1
>> opened client.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing client.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> stat ./bar/foo.h 1
>> opened ./bar/foo.h
>> closing ./bar/foo.h
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 1
>> opened /Users/thakis/src/llvm-mono-2/bar/foo2.h
>> closing /Users/thakis/src/llvm-mono-2/bar/foo2.h
>> stat ./bar/foo2.h 1
>> opened ./bar/foo2.h
>> stat ./foo3.h 1
>> opened ./foo3.h
>> closing ./foo3.h
>>
>>
>> Note how ./bar/foo2.h is opened in the 4th-to-last line but never
>> closed.  For comparision, with your patch reverted:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> stat bar/foo.h 1
>> opened bar/foo.h
>> closing bar/foo.h
>> stat bar/foo2.h 1
>> opened bar/foo2.h
>> closing bar/foo2.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 1
>> opened client.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat 

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

[Nico Weber via cfe-commits]

For completeness, in case the construction isn't clear, here's a full repro
of the symptom.

$ cat pch.cc
$ cat pch.h
#include "foo.h"
$ rm bar/foo.h
$ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
$ for i in {1..300}; do touch bar/foo$i.h; done
$ cat client.cc
#include "bar/foo.h"
$ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc; done

With today's trunk, on macOS:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc  /Ibar
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc /Ibar
client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
#include "bar/foo252.h"
 ^~
1 error generated.

This works fine with this patch undone.

On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:

> This might finally be a repro!
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
> foo.h foo2.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
> -bash: type: foo.h: not found
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
> foo.h foo2.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
> #include "foo2.h"
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
> #include "foo.h"
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
> #include "bar/foo.h"
> #include "bar/foo2.h"
>
> #include "foo3.h"
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc  /Ibar
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> stat bar/foo.h 1
> opened bar/foo.h
> closing bar/foo.h
> stat bar/foo2.h 1
> opened bar/foo2.h
> closing bar/foo2.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> opened ./pch.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> opened ./pch.h
> stat ./bar/foo.h 1
> opened ./bar/foo.h
> closing ./bar/foo.h
> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 1
> opened /Users/thakis/src/llvm-mono-2/bar/foo2.h
> closing /Users/thakis/src/llvm-mono-2/bar/foo2.h
> stat ./bar/foo2.h 1
> opened ./bar/foo2.h
> stat ./foo3.h 1
> opened ./foo3.h
> closing ./foo3.h
>
>
> Note how ./bar/foo2.h is opened in the 4th-to-last line but never closed.
> For comparision, with your patch reverted:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc  /Ibar
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> stat bar/foo.h 1
> opened bar/foo.h
> closing bar/foo.h
> stat bar/foo2.h 1
> opened bar/foo2.h
> closing bar/foo2.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> stat ./bar/foo.h 1
> stat ./bar/foo2.h 1
> stat ./foo3.h 1
> opened ./foo3.h
> closing ./foo3.h
>
> On Wed, Jan 23, 2019 at 2:21 PM Nico Weber  wrote:
>
>> On Wed, Jan 23, 2019 at 2:17 PM Nico Weber  wrote:
>>
>>> Here's what I think is a repro:
>>>
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>>> #include "foo.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>>> #include "foo2.h"
>>> #include "foo.h"
>>>
>>> #include "foo2.h"
>>> #include "foo.h"
>>>
>>> Then just build pch.cc with a pch file, and then use it (you can do this
>>> on any OS; you could also use the gcc style driver if you prefer it).
>>>
>>> The following output is with your change reverted, I added
>>>
>>> fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), 

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

[Nico Weber via cfe-commits]

This might finally be a repro!

Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
foo.h foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
-bash: type: foo.h: not found
Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
foo.h foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
#include "foo2.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
#include "foo.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
#include "bar/foo.h"
#include "bar/foo2.h"

#include "foo3.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc  /Ibar
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
stat bar/foo.h 1
opened bar/foo.h
closing bar/foo.h
stat bar/foo2.h 1
opened bar/foo2.h
closing bar/foo2.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
opened ./pch.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
opened ./pch.h
stat ./bar/foo.h 1
opened ./bar/foo.h
closing ./bar/foo.h
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 1
opened /Users/thakis/src/llvm-mono-2/bar/foo2.h
closing /Users/thakis/src/llvm-mono-2/bar/foo2.h
stat ./bar/foo2.h 1
opened ./bar/foo2.h
stat ./foo3.h 1
opened ./foo3.h
closing ./foo3.h


Note how ./bar/foo2.h is opened in the 4th-to-last line but never closed.
For comparision, with your patch reverted:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc  /Ibar
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
stat bar/foo.h 1
opened bar/foo.h
closing bar/foo.h
stat bar/foo2.h 1
opened bar/foo2.h
closing bar/foo2.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
stat ./bar/foo.h 1
stat ./bar/foo2.h 1
stat ./foo3.h 1
opened ./foo3.h
closing ./foo3.h

On Wed, Jan 23, 2019 at 2:21 PM Nico Weber  wrote:

> On Wed, Jan 23, 2019 at 2:17 PM Nico Weber  wrote:
>
>> Here's what I think is a repro:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>> #include "foo.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>> #include "foo2.h"
>> #include "foo.h"
>>
>> #include "foo2.h"
>> #include "foo.h"
>>
>> Then just build pch.cc with a pch file, and then use it (you can do this
>> on any OS; you could also use the gcc style driver if you prefer it).
>>
>> The following output is with your change reverted, I added
>>
>> fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);
>>
>> right before the call to getStatValue() in getFile, and
>>
>> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
>>   UFE.File = std::move(F);
>>
>> at the very bottom of that function, and changed getBufferForFile() to
>> have
>>
>> if (ShouldCloseOpenFile) {
>> fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
>>   Entry->closeFile();
>> }
>>
>>
>> The output is:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> opened ./foo.h
>> closing ./foo.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 

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

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 2:17 PM Nico Weber  wrote:

> Here's what I think is a repro:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
> #include "foo.h"
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
> #include "foo2.h"
> #include "foo.h"
>
> #include "foo2.h"
> #include "foo.h"
>
> Then just build pch.cc with a pch file, and then use it (you can do this
> on any OS; you could also use the gcc style driver if you prefer it).
>
> The following output is with your change reverted, I added
>
> fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);
>
> right before the call to getStatValue() in getFile, and
>
> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
>   UFE.File = std::move(F);
>
> at the very bottom of that function, and changed getBufferForFile() to have
>
> if (ShouldCloseOpenFile) {
> fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
>   Entry->closeFile();
> }
>
>
> The output is:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> stat ./foo2.h 1
> opened ./foo2.h
> closing ./foo2.h
> stat ./foo.h 1
>
>
> Note how foo.h at the end is stat'd with openFile = 1, but we don't keep
> that file handle around.
>
> Now with your patch applied, where put the
>
> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
> UFE.File = std::move(F);
>
> print in the block you moved up.
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> opened ./pch.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> opened ./pch.h
> stat ./foo2.h 1
> opened ./foo2.h
> closing ./foo2.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
>
>
> Notice how pch.pch gets opened twice but never closed in the version with
> your patch.
>

Sorry, I mean pch.h. But that's probably just from the /FI flag. While this
is an example of a file that's opened and not closed with your patch, it'd
be nice to have a repro that shows this from an #include line, not from an
/FI flag. Let me look at this a bit more. Sorry for the noise.


> I think this is enough to show that your patch is introducing assumptions
> made by clang's PCH code. Even though the issue isn't understood in detail,
> this is imho enough to revert and fix and debug async, unless you're able
> to fix very quickly.
>
>
>
> On Wed, Jan 23, 2019 at 1:23 PM Nico Weber  wrote:
>
>> The way things worked before your patch is that getFile() calls for
>> headers that are loaded from a PCH file hit the early-ish return in
>>
>>   if (UFE.isValid()) { // Already have an entry with this inode, return
>> it.
>>
>> The first stack I posted (with ReadControlBlock() in it) populates
>> this UniqueRealFiles cache, and then getFile() for includes in a pch when
>> called from the preprocessor just return "early", at least before the file
>> is opened. (Stack for that:
>>
>> (lldb) bt
>> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
>>   * frame #0: 0x00010005a2cf
>> clang-cl`clang::FileManager::getFile(this=0x00011200a190,
>> Filename=(Data =
>> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
>> Length = 89), openFile=true, 

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

[Nico Weber via cfe-commits]

Here's what I think is a repro:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
#include "foo.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
#include "foo2.h"
#include "foo.h"

#include "foo2.h"
#include "foo.h"

Then just build pch.cc with a pch file, and then use it (you can do this on
any OS; you could also use the gcc style driver if you prefer it).

The following output is with your change reverted, I added

fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);

right before the call to getStatValue() in getFile, and

if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
  UFE.File = std::move(F);

at the very bottom of that function, and changed getBufferForFile() to have

if (ShouldCloseOpenFile) {
fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
  Entry->closeFile();
}


The output is:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
stat ./foo2.h 1
opened ./foo2.h
closing ./foo2.h
stat ./foo.h 1


Note how foo.h at the end is stat'd with openFile = 1, but we don't keep
that file handle around.

Now with your patch applied, where put the

if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
UFE.File = std::move(F);

print in the block you moved up.

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
opened ./pch.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
opened ./pch.h
stat ./foo2.h 1
opened ./foo2.h
closing ./foo2.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h


Notice how pch.pch gets opened twice but never closed in the version with
your patch. I think this is enough to show that your patch is introducing
assumptions made by clang's PCH code. Even though the issue isn't
understood in detail, this is imho enough to revert and fix and debug
async, unless you're able to fix very quickly.



On Wed, Jan 23, 2019 at 1:23 PM Nico Weber  wrote:

> The way things worked before your patch is that getFile() calls for
> headers that are loaded from a PCH file hit the early-ish return in
>
>   if (UFE.isValid()) { // Already have an entry with this inode, return it.
>
> The first stack I posted (with ReadControlBlock() in it) populates
> this UniqueRealFiles cache, and then getFile() for includes in a pch when
> called from the preprocessor just return "early", at least before the file
> is opened. (Stack for that:
>
> (lldb) bt
> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
>   * frame #0: 0x00010005a2cf
> clang-cl`clang::FileManager::getFile(this=0x00011200a190,
> Filename=(Data =
> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
> Length = 89), openFile=true, CacheFailure=true) at FileManager.cpp:198
> frame #1: 0x000102ca1cb2
> clang-cl`clang::HeaderSearch::getFileAndSuggestModule(this=0x00011282e600,
> FileName=(Data =
> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
> Length = 89), IncludeLoc=(ID = 4980551), Dir=0x00011071e500,
> IsSystemHeaderDir=false, RequestingModule=0x,
> SuggestedModule=0x7fff5fbf9020) at HeaderSearch.cpp:313
> frame #2: 0x000102ca37aa
> clang-cl`clang::HeaderSearch::LookupFile(this=0x00011282e600,
> 

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

[Nico Weber via cfe-commits]

The way things worked before your patch is that getFile() calls for headers
that are loaded from a PCH file hit the early-ish return in

  if (UFE.isValid()) { // Already have an entry with this inode, return it.

The first stack I posted (with ReadControlBlock() in it) populates
this UniqueRealFiles cache, and then getFile() for includes in a pch when
called from the preprocessor just return "early", at least before the file
is opened. (Stack for that:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
  * frame #0: 0x00010005a2cf
clang-cl`clang::FileManager::getFile(this=0x00011200a190,
Filename=(Data =
"../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
Length = 89), openFile=true, CacheFailure=true) at FileManager.cpp:198
frame #1: 0x000102ca1cb2
clang-cl`clang::HeaderSearch::getFileAndSuggestModule(this=0x00011282e600,
FileName=(Data =
"../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
Length = 89), IncludeLoc=(ID = 4980551), Dir=0x00011071e500,
IsSystemHeaderDir=false, RequestingModule=0x,
SuggestedModule=0x7fff5fbf9020) at HeaderSearch.cpp:313
frame #2: 0x000102ca37aa
clang-cl`clang::HeaderSearch::LookupFile(this=0x00011282e600,
Filename=(Data = "third_party/skia/include/core/SkPoint3.h", Length = 40),
IncludeLoc=(ID = 4980551), isAngled=false, FromDir=0x,
CurDir=0x7fff5fbf9848, Includers=ArrayRef > @ 0x7fff5fbf7c78,
SearchPath=0x7fff5fbf9438, RelativePath=0x7fff5fbf9028,
RequestingModule=0x, SuggestedModule=0x7fff5fbf9020,
IsMapped=0x7fff5fbf9857, SkipCache=false, BuildSystemModule=false) at
HeaderSearch.cpp:756
frame #3: 0x000102d01898
clang-cl`clang::Preprocessor::LookupFile(this=0x00011282f618,
FilenameLoc=(ID = 4980551), Filename=(Data =
"third_party/skia/include/core/SkPoint3.h", Length = 40), isAngled=false,
FromDir=0x, FromFile=0x,
CurDir=0x7fff5fbf9848, SearchPath=0x7fff5fbf9438,
RelativePath=0x7fff5fbf9028, SuggestedModule=0x7fff5fbf9020,
IsMapped=0x7fff5fbf9857, SkipCache=false) at PPDirectives.cpp:740
frame #4: 0x000102d033c4
clang-cl`clang::Preprocessor::HandleIncludeDirective(this=0x00011282f618,
HashLoc=(ID = 4980542), IncludeTok=0x00011180a810,
LookupFrom=0x, LookupFromFile=0x,
isImport=false) at PPDirectives.cpp:1773
frame #5: 0x000102d05e3a
clang-cl`clang::Preprocessor::HandleDirective(this=0x00011282f618,
Result=0x00011180a810) at PPDirectives.cpp:942
frame #6: 0x000102cbff1f
clang-cl`clang::Lexer::LexTokenInternal(this=0x00011bc567a0,
Result=0x00011180a810, TokAtPhysicalStartOfLine=true) at Lexer.cpp:3931
frame #7: 0x000102cbc1b8
clang-cl`clang::Lexer::Lex(this=0x00011bc567a0,
Result=0x00011180a810) at Lexer.cpp:3152
frame #8: 0x000102d54dcb
clang-cl`clang::Preprocessor::Lex(this=0x00011282f618,
Result=0x00011180a810) at Preprocessor.cpp:868
frame #9: 0x000102f235f9
clang-cl`clang::Parser::ConsumeBrace(this=0x00011180a800) at
Parser.h:563
frame #10: 0x000102f2a21a
clang-cl`clang::BalancedDelimiterTracker::consumeClose(this=0x7fff5fbfa560)
at RAIIObjectsForParser.h:429
frame #11: 0x000102e788e5
clang-cl`clang::Parser::ParseInnerNamespace(this=0x00011180a800,
InnerNSs=0x7fff5fbfa6e0, index=0, InlineLoc=0x7fff5fbfa7a0,
attrs=0x7fff5fbfa6b8, Tracker=0x7fff5fbfa560) at
ParseDeclCXX.cpp:250
frame #12: 0x000102e77dd6
clang-cl`clang::Parser::ParseNamespace(this=0x00011180a800,
Context=FileContext, DeclEnd=0x7fff5fbfaa70, InlineLoc=(ID = 0)) at
ParseDeclCXX.cpp:223
frame #13: 0x000102e5af29
clang-cl`clang::Parser::ParseDeclaration(this=0x00011180a800,
Context=FileContext, DeclEnd=0x7fff5fbfaa70, attrs=0x7fff5fbfac40)
at ParseDecl.cpp:1714
frame #14: 0x000102f25dc3
clang-cl`clang::Parser::ParseExternalDeclaration(this=0x00011180a800,
attrs=0x7fff5fbfac40, DS=0x) at Parser.cpp:788
frame #15: 0x000102f24f06
clang-cl`clang::Parser::ParseTopLevelDecl(this=0x00011180a800,
Result=0x7fff5fbfad78) at Parser.cpp:609
frame #16: 0x000102e4a076
clang-cl`clang::ParseAST(S=0x0001118fb600, PrintStats=true,
SkipFunctionBodies=false) at ParseAST.cpp:156
frame #17: 0x000100add312
clang-cl`clang::ASTFrontendAction::ExecuteAction(this=0x000112007eb0)
at FrontendAction.cpp:1035
frame #18: 0x0001004f6cbd
clang-cl`clang::CodeGenAction::ExecuteAction(this=0x000112007eb0) at
CodeGenAction.cpp:1047
frame #19: 0x000100adc920
clang-cl`clang::FrontendAction::Execute(this=0x000112007eb0) at
FrontendAction.cpp:934
frame #20: 

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

[Nico Weber via cfe-commits]

Stacks for getFile() from PCH files look like so:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00010005a1fb
clang-cl`clang::FileManager::getFile(this=0x0001106111f0,
Filename=(Data =
"/Users/thakis/src/chrome/src/out/gnwin/../../third_party/skia/include/core/SkPoint3.h",
Length = 85), openFile=false, CacheFailure=true) at FileManager.cpp:187
frame #1: 0x000103c44b2e
clang-cl`clang::ASTReader::getInputFile(this=0x00011104c200,
F=0x00011104fc00, ID=3, Complain=true) at ASTReader.cpp:2120
frame #2: 0x000103c5173b
clang-cl`clang::ASTReader::ReadControlBlock(this=0x00011104c200,
F=0x00011104fc00, Loaded=0x7fff5fbfa130,
ImportedBy=0x, ClientLoadCapabilities=0) at
ASTReader.cpp:2398
frame #3: 0x000103c5381b
clang-cl`clang::ASTReader::ReadASTCore(this=0x00011104c200,
FileName=(Data =
"obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Type=MK_PCH, ImportLoc=(ID = 0), ImportedBy=0x,
Loaded=0x7fff5fbfa130, ExpectedSize=0, ExpectedModTime=0,
ExpectedSignature=ASTFileSignature @ 0x7fff5fbf9d58,
ClientLoadCapabilities=0) at ASTReader.cpp:4208
frame #4: 0x000103c5fe95
clang-cl`clang::ASTReader::ReadAST(this=0x00011104c200, FileName=(Data
= "obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Type=MK_PCH, ImportLoc=(ID = 0), ClientLoadCapabilities=0,
Imported=0x) at ASTReader.cpp:3883
frame #5: 0x000100a49542
clang-cl`clang::CompilerInstance::createPCHExternalASTSource(Path=(Data =
"obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Sysroot=(Data = "/", Length = 1), DisablePCHValidation=false,
AllowPCHWithCompilerErrors=false, PP=0x000111037e18,
Context=0x000111042200, PCHContainerRdr=0x00011060d2b0,
Extensions=ArrayRef > @
0x7fff5fbfa430, DependencyFile=0x,
DependencyCollectors=ArrayRef
> @ 0x7fff5fbfa448, DeserializationListener=0x,
OwnDeserializationListener=false, Preamble=false,
UseGlobalModuleIndex=true) at CompilerInstance.cpp:532
frame #6: 0x000100a4909b
clang-cl`clang::CompilerInstance::createPCHExternalASTSource(this=0x00011060d000,
Path=(Data = "obj/third_party/blink/renderer/core/layout/layout_cc.pch",
Length = 56), DisablePCHValidation=false, AllowPCHWithCompilerErrors=false,
DeserializationListener=0x,
OwnDeserializationListener=false) at CompilerInstance.cpp:490
frame #7: 0x000100ada301
clang-cl`clang::FrontendAction::BeginSourceFile(this=0x00011060eee0,
CI=0x00011060d000, RealInput=0x000110614a60) at
FrontendAction.cpp:859
frame #8: 0x000100a4cdf8
clang-cl`clang::CompilerInstance::ExecuteAction(this=0x00011060d000,
Act=0x00011060eee0) at CompilerInstance.cpp:953
frame #9: 0x000100b5c21f
clang-cl`clang::ExecuteCompilerInvocation(Clang=0x00011060d000) at
ExecuteCompilerInvocation.cpp:267
frame #10: 0x00011ac9 clang-cl`cc1_main(Argv=ArrayRef @ 0x7fff5fbfb4c8,
Argv0="../../../../llvm-mono-2/out/gn/bin/clang-cl",
MainAddr=0x000100028110) at cc1_main.cpp:218
frame #11: 0x00010002999f
clang-cl`ExecuteCC1Tool(argv=ArrayRef @ 0x7fff5fbfba78,
Tool=(Data = "", Length = 0)) at driver.cpp:309
frame #12: 0x000100028a81 clang-cl`main(argc_=409,
argv_=0x7fff5fbfd170) at driver.cpp:381
frame #13: 0x7fffb015c235 libdyld.dylib`start + 1

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00010005a1fb
clang-cl`clang::FileManager::getFile(this=0x0001106111f0,
Filename=(Data =
"/Users/thakis/src/chrome/src/out/gnwin/../../third_party/skia/include/core/SkPoint3.h",
Length = 85), openFile=false, CacheFailure=true) at FileManager.cpp:187
frame #1: 0x000103c476e3
clang-cl`clang::serialization::reader::HeaderFileInfoTrait::EqualKey(this=0x7fff5fbf5f80,
Key=0x7fff5fbf6068)::$_1::operator()(clang::serialization::reader::HeaderFileInfoTrait::internal_key_type
const&) const at ASTReader.cpp:1723
frame #2: 0x000103c4759f
clang-cl`clang::serialization::reader::HeaderFileInfoTrait::EqualKey(this=0x000113a16c88,
a=0x7fff5fbf6068, b=0x7fff5fbf6138) at ASTReader.cpp:1726
frame #3: 0x000103cd5c1e
clang-cl`llvm::OnDiskChainedHashTable::find_hashed(this=0x000113a16c70,
IKey=0x7fff5fbf6138, KeyHash=986574196, InfoPtr=0x000113a16c88) at
OnDiskHashTable.h:390
frame #4: 0x000103cd59b8
clang-cl`llvm::OnDiskChainedHashTable::find(this=0x000113a16c70,
EKey=0x7fff5fbf6538, InfoPtr=0x) at
OnDiskHashTable.h:345
frame #5: 0x000103cd58b9 clang-cl`(anonymous
namespace)::HeaderFileInfoVisitor::operator(this=0x7fff5fbf6538,
M=0x00011104fc00)(clang::serialization::ModuleFile&) at
ASTReader.cpp:5683
frame #6: 0x000103cd5850 clang-cl`bool 

Re: 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  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  wrote:
>
>> On Wed, Jan 23, 2019 at 9:54 AM Sam McCall  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
>> , 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> 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

Re: 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  wrote:

> On Wed, Jan 23, 2019 at 9:54 AM Sam McCall  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
> , 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> 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

Re: 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  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
, 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> 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

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

[Sam McCall via cfe-commits]

(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? 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

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

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

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 5:44 AM Sam McCall  wrote:

> Ugh, sorry about this :-\
>
> Some random observations:
>  - I didn't think clang itself could actually hit that codepath
> (open=false then open=true for the same file). Though not surprising it's
> precompiled-headers related, that's similar to how clangd hit it.
>  - The obvious failure mode (file state actually changed on disk) isn't
> the case. It's possible that the VFS or filemanager state might be
> different at the two relevant points in time.
>  - this was a bugfix itself, and it seems plausible that we're uncovering
> another subtle latent bug. So I'd really like to get a bit more info before
> reverting.
>  - that said, this seems like an issue that needs to be fixed on the llvm8
> branch somehow.
>
> If it's easy enough to do so, it'd be useful to add logging to
> FileManager::getStatValue...
> if Path ends in "content_security_policy_parsers.h" then log:
>  - Path (the full path we're opening)
>  - FS->getCurrentWorkingDirectory() (in case Path is relative)
>  - whether the file pointer F is null (if non-null, this is an open)
>  - and the return value of FileSystemStatCache::get() (was the file found
> or not)
> I expect to see a call with F != null followed by a call with F == null.
>

I can repro locally, happy to try everything you want me to try. I had
reduced the repro a bit so now the error looks like:

../../third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h(10,10):
fatal error:
'third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h'
file not found

I changed FileManager.cpp like this:

$ git diff
diff --git a/clang/lib/Basic/FileManager.cpp
b/clang/lib/Basic/FileManager.cpp
index 01acfd5dd61..5388c7f879c 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -460,16 +460,28 @@ FileManager::getBufferForFile(StringRef Filename,
bool isVolatile) {
 /// do directory look-up instead of file look-up.
 bool FileManager::getStatValue(StringRef Path, FileData , bool isFile,
std::unique_ptr *F) {
+bool b = false;
+  if (Path.endswith("scheduling_lifecycle_state.h")) {
+fprintf(stderr, "%s: %s %p\n", Path.str().c_str(),
FS->getCurrentWorkingDirectory().get().c_str(), F);
+b = true;
+  }
+
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
-  if (FileSystemOpts.WorkingDir.empty())
-return FileSystemStatCache::get(Path, Data, isFile, F,StatCache.get(),
*FS);
+  if (FileSystemOpts.WorkingDir.empty()) {
+bool R =
+FileSystemStatCache::get(Path, Data, isFile, F, StatCache.get(),
*FS);
+if (b) fprintf(stderr, "early get() %d\n", R);
+return R;
+  }

   SmallString<128> FilePath(Path);
   FixupRelativePath(FilePath);

-  return FileSystemStatCache::get(FilePath.c_str(), Data, isFile, F,
-  StatCache.get(), *FS);
+  bool R = FileSystemStatCache::get(FilePath.c_str(), Data, isFile, F,
+StatCache.get(), *FS);
+if (b) fprintf(stderr, "get() %d\n", R);
+  return R;
 }



The output with that patch is:

$ time ./run.sh
pch
main
../../third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h:
/Users/thakis/src/chrome/src/out/gnwin 0x7fff5e677150
early get() 1
In file included from
../../third_party/blink/renderer/core/layout/layout_quote.cc:40:
../../third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h(10,10):
fatal error:
'third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h'
file not found
#include
"third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h"



It looks like FS->getCurrentWorkingDirectory() is set
yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected? But
the output doesn't look like you expect, at least.


>
> I can also try to repro locally, though I don't have a mac or experience
> building chromium, and it sounds likely this is at least somewhat
> platform-dependent.
>
> On Wed, Jan 23, 2019 at 4:17 AM Nico Weber via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> I don't have a reduced test case yet, but this seems to cause clang to
>> sometimes claim that an included file isn't found even if it's there, at
>> least on macOS:
>> https://bugs.chromium.org/p/chromium/issues/detail?id=924225
>>
>> On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: sammccall
>>> Date: Mon Nov 19 05:37:46 2018
>>> New Revision: 347205
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=347205=rev
>>> Log:
>>> [FileManager] getFile(open=true) after getFile(open=false) should open
>>> the file.
>>>
>>> Summary:
>>> Old behavior is to just return the cached entry regardless of
>>> opened-ness.
>>> That feels buggy (though I guess nobody ever actually needed this).
>>>
>>> This came up in the context of 

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

[Sam McCall via cfe-commits]

Ugh, sorry about this :-\

Some random observations:
 - I didn't think clang itself could actually hit that codepath (open=false
then open=true for the same file). Though not surprising it's
precompiled-headers related, that's similar to how clangd hit it.
 - The obvious failure mode (file state actually changed on disk) isn't the
case. It's possible that the VFS or filemanager state might be different at
the two relevant points in time.
 - this was a bugfix itself, and it seems plausible that we're uncovering
another subtle latent bug. So I'd really like to get a bit more info before
reverting.
 - that said, this seems like an issue that needs to be fixed on the llvm8
branch somehow.

If it's easy enough to do so, it'd be useful to add logging to
FileManager::getStatValue...
if Path ends in "content_security_policy_parsers.h" then log:
 - Path (the full path we're opening)
 - FS->getCurrentWorkingDirectory() (in case Path is relative)
 - whether the file pointer F is null (if non-null, this is an open)
 - and the return value of FileSystemStatCache::get() (was the file found
or not)
I expect to see a call with F != null followed by a call with F == null.

I can also try to repro locally, though I don't have a mac or experience
building chromium, and it sounds likely this is at least somewhat
platform-dependent.

On Wed, Jan 23, 2019 at 4:17 AM Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> I don't have a reduced test case yet, but this seems to cause clang to
> sometimes claim that an included file isn't found even if it's there, at
> least on macOS:
> https://bugs.chromium.org/p/chromium/issues/detail?id=924225
>
> On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: sammccall
>> Date: Mon Nov 19 05:37:46 2018
>> New Revision: 347205
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=347205=rev
>> Log:
>> [FileManager] getFile(open=true) after getFile(open=false) should open
>> the file.
>>
>> Summary:
>> Old behavior is to just return the cached entry regardless of opened-ness.
>> That feels buggy (though I guess nobody ever actually needed this).
>>
>> This came up in the context of clangd+clang-tidy integration: we're
>> going to getFile(open=false) to replay preprocessor actions obscured by
>> the preamble, but the compilation may subsequently getFile(open=true)
>> for non-preamble includes.
>>
>> Reviewers: ilya-biryukov
>>
>> Subscribers: ioeric, kadircet, cfe-commits
>>
>> Differential Revision: https://reviews.llvm.org/D54691
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/FileManager.h
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/FileManager.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205=347204=347205=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/FileManager.h (original)
>> +++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018
>> @@ -70,14 +70,15 @@ class FileEntry {
>>bool IsNamedPipe;
>>bool InPCH;
>>bool IsValid;   // Is this \c FileEntry initialized and
>> valid?
>> +  bool DeferredOpen;  // Created by getFile(OpenFile=0); may
>> open later.
>>
>>/// The open file, if it is owned by the \p FileEntry.
>>mutable std::unique_ptr File;
>>
>>  public:
>>FileEntry()
>> -  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
>> -  {}
>> +  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
>> +DeferredOpen(false) {}
>>
>>FileEntry(const FileEntry &) = delete;
>>FileEntry =(const FileEntry &) = delete;
>>
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205=347204=347205=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018
>> @@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St
>>*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
>>
>>// See if there is already an entry in the map.
>> -  if (NamedFileEnt.second)
>> -return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
>> -:
>> NamedFileEnt.second;
>> +  if (NamedFileEnt.second) {
>> +if (NamedFileEnt.second == NON_EXISTENT_FILE)
>> +  return nullptr;
>> +// Entry exists: return it *unless* it wasn't opened and open is
>> requested.
>> +if (!(NamedFileEnt.second->DeferredOpen && openFile))
>> +  return NamedFileEnt.second;
>> +// We previously stat()ed the file, but didn't open it: do that
>> below.
>> +// FIXME: the below does other redundant work too 

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

[Nico Weber via cfe-commits]

I don't have a reduced test case yet, but this seems to cause clang to
sometimes claim that an included file isn't found even if it's there, at
least on macOS: https://bugs.chromium.org/p/chromium/issues/detail?id=924225

On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: sammccall
> Date: Mon Nov 19 05:37:46 2018
> New Revision: 347205
>
> URL: http://llvm.org/viewvc/llvm-project?rev=347205=rev
> Log:
> [FileManager] getFile(open=true) after getFile(open=false) should open the
> file.
>
> Summary:
> Old behavior is to just return the cached entry regardless of opened-ness.
> That feels buggy (though I guess nobody ever actually needed this).
>
> This came up in the context of clangd+clang-tidy integration: we're
> going to getFile(open=false) to replay preprocessor actions obscured by
> the preamble, but the compilation may subsequently getFile(open=true)
> for non-preamble includes.
>
> Reviewers: ilya-biryukov
>
> Subscribers: ioeric, kadircet, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D54691
>
> Modified:
> cfe/trunk/include/clang/Basic/FileManager.h
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/include/clang/Basic/FileManager.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205=347204=347205=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/FileManager.h (original)
> +++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018
> @@ -70,14 +70,15 @@ class FileEntry {
>bool IsNamedPipe;
>bool InPCH;
>bool IsValid;   // Is this \c FileEntry initialized and
> valid?
> +  bool DeferredOpen;  // Created by getFile(OpenFile=0); may open
> later.
>
>/// The open file, if it is owned by the \p FileEntry.
>mutable std::unique_ptr File;
>
>  public:
>FileEntry()
> -  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
> -  {}
> +  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
> +DeferredOpen(false) {}
>
>FileEntry(const FileEntry &) = delete;
>FileEntry =(const FileEntry &) = delete;
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205=347204=347205=diff
>
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018
> @@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St
>*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
>
>// See if there is already an entry in the map.
> -  if (NamedFileEnt.second)
> -return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
> -: NamedFileEnt.second;
> +  if (NamedFileEnt.second) {
> +if (NamedFileEnt.second == NON_EXISTENT_FILE)
> +  return nullptr;
> +// Entry exists: return it *unless* it wasn't opened and open is
> requested.
> +if (!(NamedFileEnt.second->DeferredOpen && openFile))
> +  return NamedFileEnt.second;
> +// We previously stat()ed the file, but didn't open it: do that below.
> +// FIXME: the below does other redundant work too (stats the dir and
> file).
> +  } else {
> +// By default, initialize it to invalid.
> +NamedFileEnt.second = NON_EXISTENT_FILE;
> +  }
>
>++NumFileCacheMisses;
>
> -  // By default, initialize it to invalid.
> -  NamedFileEnt.second = NON_EXISTENT_FILE;
> -
>// Get the null-terminated file name as stored as the key of the
>// SeenFileEntries map.
>StringRef InterndFileName = NamedFileEnt.first();
> @@ -267,6 +273,7 @@ const FileEntry *FileManager::getFile(St
>// It exists.  See if we have already opened a file with the same inode.
>// This occurs when one dir is symlinked to another, for example.
>FileEntry  = UniqueRealFiles[Data.UniqueID];
> +  UFE.DeferredOpen = !openFile;
>
>NamedFileEnt.second = 
>
> @@ -283,6 +290,23 @@ const FileEntry *FileManager::getFile(St
>  InterndFileName = NamedFileEnt.first().data();
>}
>
> +  // If we opened the file for the first time, record the resulting info.
> +  // Do this even if the cache entry was valid, maybe we didn't
> previously open.
> +  if (F && !UFE.File) {
> +if (auto PathName = F->getName()) {
> +  llvm::SmallString<128> AbsPath(*PathName);
> +  // This is not the same as `VFS::getRealPath()`, which resolves
> symlinks
> +  // but can be very expensive on real file systems.
> +  // FIXME: the semantic of RealPathName is unclear, and the name
> might be
> +  // misleading. We need to clean up the interface here.
> +  makeAbsolutePath(AbsPath);
> +  

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

[Galina Kistanova via cfe-commits]

Hello Sam,

This commit broke build test step for on couple of our builders today:

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/21573
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14095

. . .
Failing Tests (1):
Clang-Unit :: Basic/./BasicTests.exe/FileManagerTest.getFileDefersOpen

Please have a look ASAP?

Thanks

Galina

On Mon, Nov 19, 2018 at 5:40 AM Sam McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: sammccall
> Date: Mon Nov 19 05:37:46 2018
> New Revision: 347205
>
> URL: http://llvm.org/viewvc/llvm-project?rev=347205=rev
> Log:
> [FileManager] getFile(open=true) after getFile(open=false) should open the
> file.
>
> Summary:
> Old behavior is to just return the cached entry regardless of opened-ness.
> That feels buggy (though I guess nobody ever actually needed this).
>
> This came up in the context of clangd+clang-tidy integration: we're
> going to getFile(open=false) to replay preprocessor actions obscured by
> the preamble, but the compilation may subsequently getFile(open=true)
> for non-preamble includes.
>
> Reviewers: ilya-biryukov
>
> Subscribers: ioeric, kadircet, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D54691
>
> Modified:
> cfe/trunk/include/clang/Basic/FileManager.h
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/include/clang/Basic/FileManager.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205=347204=347205=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/FileManager.h (original)
> +++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018
> @@ -70,14 +70,15 @@ class FileEntry {
>bool IsNamedPipe;
>bool InPCH;
>bool IsValid;   // Is this \c FileEntry initialized and
> valid?
> +  bool DeferredOpen;  // Created by getFile(OpenFile=0); may open
> later.
>
>/// The open file, if it is owned by the \p FileEntry.
>mutable std::unique_ptr File;
>
>  public:
>FileEntry()
> -  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
> -  {}
> +  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
> +DeferredOpen(false) {}
>
>FileEntry(const FileEntry &) = delete;
>FileEntry =(const FileEntry &) = delete;
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205=347204=347205=diff
>
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018
> @@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St
>*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
>
>// See if there is already an entry in the map.
> -  if (NamedFileEnt.second)
> -return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
> -: NamedFileEnt.second;
> +  if (NamedFileEnt.second) {
> +if (NamedFileEnt.second == NON_EXISTENT_FILE)
> +  return nullptr;
> +// Entry exists: return it *unless* it wasn't opened and open is
> requested.
> +if (!(NamedFileEnt.second->DeferredOpen && openFile))
> +  return NamedFileEnt.second;
> +// We previously stat()ed the file, but didn't open it: do that below.
> +// FIXME: the below does other redundant work too (stats the dir and
> file).
> +  } else {
> +// By default, initialize it to invalid.
> +NamedFileEnt.second = NON_EXISTENT_FILE;
> +  }
>
>++NumFileCacheMisses;
>
> -  // By default, initialize it to invalid.
> -  NamedFileEnt.second = NON_EXISTENT_FILE;
> -
>// Get the null-terminated file name as stored as the key of the
>// SeenFileEntries map.
>StringRef InterndFileName = NamedFileEnt.first();
> @@ -267,6 +273,7 @@ const FileEntry *FileManager::getFile(St
>// It exists.  See if we have already opened a file with the same inode.
>// This occurs when one dir is symlinked to another, for example.
>FileEntry  = UniqueRealFiles[Data.UniqueID];
> +  UFE.DeferredOpen = !openFile;
>
>NamedFileEnt.second = 
>
> @@ -283,6 +290,23 @@ const FileEntry *FileManager::getFile(St
>  InterndFileName = NamedFileEnt.first().data();
>}
>
> +  // If we opened the file for the first time, record the resulting info.
> +  // Do this even if the cache entry was valid, maybe we didn't
> previously open.
> +  if (F && !UFE.File) {
> +if (auto PathName = F->getName()) {
> +  llvm::SmallString<128> AbsPath(*PathName);
> +  // This is not the same as `VFS::getRealPath()`, which resolves
> symlinks
> +  // but can be very expensive on real file systems.
> +  // FIXME: the semantic of