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
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),
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
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
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
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
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"
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$
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
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
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 =
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
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
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
(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
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
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
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 <
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):
19 matches
Mail list logo
