<div dir="ltr"><div dir="ltr"><div>Ugh, sorry about this :-\</div><div><br></div><div>Some random observations:<br><div> - 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.</div></div><div> - 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.</div><div> - 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.</div><div> - that said, this seems like an issue that needs to be fixed on the llvm8 branch somehow.</div><div><br></div><div>If it's easy enough to do so, it'd be useful to add logging to FileManager::getStatValue...</div><div>if Path ends in "content_security_policy_parsers.h" then log:</div><div> - Path (the full path we're opening)</div><div> - FS->getCurrentWorkingDirectory() (in case Path is relative)</div><div> - whether the file pointer F is null (if non-null, this is an open)</div><div> - and the return value of FileSystemStatCache::get() (was the file found or not)</div><div>I expect to see a call with F != null followed by a call with F == null.</div><div><br></div><div>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.</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1576035286175550366gmail_attr">On Wed, Jan 23, 2019 at 4:17 AM Nico Weber via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">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: <a href="https://bugs.chromium.org/p/chromium/issues/detail?id=924225" target="_blank">https://bugs.chromium.org/p/chromium/issues/detail?id=924225</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_1576035286175550366gmail-m_8476460814898750731gmail_attr">On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: sammccall<br>
Date: Mon Nov 19 05:37:46 2018<br>
New Revision: 347205<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=347205&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=347205&view=rev</a><br>
Log:<br>
[FileManager] getFile(open=true) after getFile(open=false) should open the file.<br>
<br>
Summary:<br>
Old behavior is to just return the cached entry regardless of opened-ness.<br>
That feels buggy (though I guess nobody ever actually needed this).<br>
<br>
This came up in the context of clangd+clang-tidy integration: we're<br>
going to getFile(open=false) to replay preprocessor actions obscured by<br>
the preamble, but the compilation may subsequently getFile(open=true)<br>
for non-preamble includes.<br>
<br>
Reviewers: ilya-biryukov<br>
<br>
Subscribers: ioeric, kadircet, cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D54691" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54691</a><br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/FileManager.h<br>
cfe/trunk/lib/Basic/FileManager.cpp<br>
cfe/trunk/unittests/Basic/FileManagerTest.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/FileManager.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205&r1=347204&r2=347205&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205&r1=347204&r2=347205&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/FileManager.h (original)<br>
+++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018<br>
@@ -70,14 +70,15 @@ class FileEntry {<br>
bool IsNamedPipe;<br>
bool InPCH;<br>
bool IsValid; // Is this \c FileEntry initialized and valid?<br>
+ bool DeferredOpen; // Created by getFile(OpenFile=0); may open later.<br>
<br>
/// The open file, if it is owned by the \p FileEntry.<br>
mutable std::unique_ptr<llvm::vfs::File> File;<br>
<br>
public:<br>
FileEntry()<br>
- : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)<br>
- {}<br>
+ : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),<br>
+ DeferredOpen(false) {}<br>
<br>
FileEntry(const FileEntry &) = delete;<br>
FileEntry &operator=(const FileEntry &) = delete;<br>
<br>
Modified: cfe/trunk/lib/Basic/FileManager.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205&r1=347204&r2=347205&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205&r1=347204&r2=347205&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Basic/FileManager.cpp (original)<br>
+++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018<br>
@@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St<br>
*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;<br>
<br>
// See if there is already an entry in the map.<br>
- if (NamedFileEnt.second)<br>
- return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr<br>
- : NamedFileEnt.second;<br>
+ if (NamedFileEnt.second) {<br>
+ if (NamedFileEnt.second == NON_EXISTENT_FILE)<br>
+ return nullptr;<br>
+ // Entry exists: return it *unless* it wasn't opened and open is requested.<br>
+ if (!(NamedFileEnt.second->DeferredOpen && openFile))<br>
+ return NamedFileEnt.second;<br>
+ // We previously stat()ed the file, but didn't open it: do that below.<br>
+ // FIXME: the below does other redundant work too (stats the dir and file).<br>
+ } else {<br>
+ // By default, initialize it to invalid.<br>
+ NamedFileEnt.second = NON_EXISTENT_FILE;<br>
+ }<br>
<br>
++NumFileCacheMisses;<br>
<br>
- // By default, initialize it to invalid.<br>
- NamedFileEnt.second = NON_EXISTENT_FILE;<br>
-<br>
// Get the null-terminated file name as stored as the key of the<br>
// SeenFileEntries map.<br>
StringRef InterndFileName = NamedFileEnt.first();<br>
@@ -267,6 +273,7 @@ const FileEntry *FileManager::getFile(St<br>
// It exists. See if we have already opened a file with the same inode.<br>
// This occurs when one dir is symlinked to another, for example.<br>
FileEntry &UFE = UniqueRealFiles[Data.UniqueID];<br>
+ UFE.DeferredOpen = !openFile;<br>
<br>
NamedFileEnt.second = &UFE;<br>
<br>
@@ -283,6 +290,23 @@ const FileEntry *FileManager::getFile(St<br>
InterndFileName = NamedFileEnt.first().data();<br>
}<br>
<br>
+ // If we opened the file for the first time, record the resulting info.<br>
+ // Do this even if the cache entry was valid, maybe we didn't previously open.<br>
+ if (F && !UFE.File) {<br>
+ if (auto PathName = F->getName()) {<br>
+ llvm::SmallString<128> AbsPath(*PathName);<br>
+ // This is not the same as `VFS::getRealPath()`, which resolves symlinks<br>
+ // but can be very expensive on real file systems.<br>
+ // FIXME: the semantic of RealPathName is unclear, and the name might be<br>
+ // misleading. We need to clean up the interface here.<br>
+ makeAbsolutePath(AbsPath);<br>
+ llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);<br>
+ UFE.RealPathName = AbsPath.str();<br>
+ }<br>
+ UFE.File = std::move(F);<br>
+ assert(!UFE.DeferredOpen && "we just opened it!");<br>
+ }<br>
+<br>
if (UFE.isValid()) { // Already have an entry with this inode, return it.<br>
<br>
// FIXME: this hack ensures that if we look up a file by a virtual path in<br>
@@ -313,21 +337,9 @@ const FileEntry *FileManager::getFile(St<br>
UFE.UniqueID = Data.UniqueID;<br>
UFE.IsNamedPipe = Data.IsNamedPipe;<br>
UFE.InPCH = Data.InPCH;<br>
- UFE.File = std::move(F);<br>
UFE.IsValid = true;<br>
+ // Note File and DeferredOpen were initialized above.<br>
<br>
- if (UFE.File) {<br>
- if (auto PathName = UFE.File->getName()) {<br>
- llvm::SmallString<128> AbsPath(*PathName);<br>
- // This is not the same as `VFS::getRealPath()`, which resolves symlinks<br>
- // but can be very expensive on real file systems.<br>
- // FIXME: the semantic of RealPathName is unclear, and the name might be<br>
- // misleading. We need to clean up the interface here.<br>
- makeAbsolutePath(AbsPath);<br>
- llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);<br>
- UFE.RealPathName = AbsPath.str();<br>
- }<br>
- }<br>
return &UFE;<br>
}<br>
<br>
@@ -398,6 +410,7 @@ FileManager::getVirtualFile(StringRef Fi<br>
UFE->UID = NextFileUID++;<br>
UFE->IsValid = true;<br>
UFE->File.reset();<br>
+ UFE->DeferredOpen = false;<br>
return UFE;<br>
}<br>
<br>
<br>
Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=347205&r1=347204&r2=347205&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=347205&r1=347204&r2=347205&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)<br>
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Mon Nov 19 05:37:46 2018<br>
@@ -222,6 +222,33 @@ TEST_F(FileManagerTest, getFileReturnsNU<br>
EXPECT_EQ(nullptr, file);<br>
}<br>
<br>
+// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is<br>
+// opened for the second call.<br>
+TEST_F(FileManagerTest, getFileDefersOpen) {<br>
+ llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(<br>
+ new llvm::vfs::InMemoryFileSystem());<br>
+ FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));<br>
+ FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));<br>
+ FileManager manager(options, FS);<br>
+<br>
+ const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);<br>
+ ASSERT_TRUE(file != nullptr);<br>
+ ASSERT_TRUE(file->isValid());<br>
+ // "real path name" reveals whether the file was actually opened.<br>
+ EXPECT_EQ("", file->tryGetRealPathName());<br>
+<br>
+ file = manager.getFile("/tmp/test", /*OpenFile=*/true);<br>
+ ASSERT_TRUE(file != nullptr);<br>
+ ASSERT_TRUE(file->isValid());<br>
+ EXPECT_EQ("/tmp/test", file->tryGetRealPathName());<br>
+<br>
+ // However we should never try to open a file previously opened as virtual.<br>
+ ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));<br>
+ ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));<br>
+ file = manager.getFile("/tmp/testv", /*OpenFile=*/true);<br>
+ EXPECT_EQ("", file->tryGetRealPathName());<br>
+}<br>
+<br>
// The following tests apply to Unix-like system only.<br>
<br>
#ifndef _WIN32<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>