On Wed, Jan 23, 2019 at 5:44 AM Sam McCall <sammcc...@google.com> 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 &Data, bool isFile,
std::unique_ptr<llvm::vfs::File> *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&view=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&r1=347204&r2=347205&view=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<llvm::vfs::File> 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 &operator=(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&r1=347204&r2=347205&view=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 &UFE = UniqueRealFiles[Data.UniqueID];
>>> + UFE.DeferredOpen = !openFile;
>>>
>>> NamedFileEnt.second = &UFE;
>>>
>>> @@ -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);
>>> + llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
>>> + UFE.RealPathName = AbsPath.str();
>>> + }
>>> + UFE.File = std::move(F);
>>> + assert(!UFE.DeferredOpen && "we just opened it!");
>>> + }
>>> +
>>> if (UFE.isValid()) { // Already have an entry with this inode, return
>>> it.
>>>
>>> // FIXME: this hack ensures that if we look up a file by a virtual
>>> path in
>>> @@ -313,21 +337,9 @@ const FileEntry *FileManager::getFile(St
>>> UFE.UniqueID = Data.UniqueID;
>>> UFE.IsNamedPipe = Data.IsNamedPipe;
>>> UFE.InPCH = Data.InPCH;
>>> - UFE.File = std::move(F);
>>> UFE.IsValid = true;
>>> + // Note File and DeferredOpen were initialized above.
>>>
>>> - if (UFE.File) {
>>> - if (auto PathName = UFE.File->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);
>>> - llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
>>> - UFE.RealPathName = AbsPath.str();
>>> - }
>>> - }
>>> return &UFE;
>>> }
>>>
>>> @@ -398,6 +410,7 @@ FileManager::getVirtualFile(StringRef Fi
>>> UFE->UID = NextFileUID++;
>>> UFE->IsValid = true;
>>> UFE->File.reset();
>>> + UFE->DeferredOpen = false;
>>> return UFE;
>>> }
>>>
>>>
>>> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=347205&r1=347204&r2=347205&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
>>> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Mon Nov 19 05:37:46
>>> 2018
>>> @@ -222,6 +222,33 @@ TEST_F(FileManagerTest, getFileReturnsNU
>>> EXPECT_EQ(nullptr, file);
>>> }
>>>
>>> +// When calling getFile(OpenFile=false); getFile(OpenFile=true) the
>>> file is
>>> +// opened for the second call.
>>> +TEST_F(FileManagerTest, getFileDefersOpen) {
>>> + llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
>>> + new llvm::vfs::InMemoryFileSystem());
>>> + FS->addFile("/tmp/test", 0,
>>> llvm::MemoryBuffer::getMemBufferCopy("test"));
>>> + FS->addFile("/tmp/testv", 0,
>>> llvm::MemoryBuffer::getMemBufferCopy("testv"));
>>> + FileManager manager(options, FS);
>>> +
>>> + const FileEntry *file = manager.getFile("/tmp/test",
>>> /*OpenFile=*/false);
>>> + ASSERT_TRUE(file != nullptr);
>>> + ASSERT_TRUE(file->isValid());
>>> + // "real path name" reveals whether the file was actually opened.
>>> + EXPECT_EQ("", file->tryGetRealPathName());
>>> +
>>> + file = manager.getFile("/tmp/test", /*OpenFile=*/true);
>>> + ASSERT_TRUE(file != nullptr);
>>> + ASSERT_TRUE(file->isValid());
>>> + EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
>>> +
>>> + // However we should never try to open a file previously opened as
>>> virtual.
>>> + ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
>>> + ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
>>> + file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
>>> + EXPECT_EQ("", file->tryGetRealPathName());
>>> +}
>>> +
>>> // The following tests apply to Unix-like system only.
>>>
>>> #ifndef _WIN32
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
