<div dir="ltr"><div dir="ltr">Hello Sam,<br><br>This commit broke build test step for on couple of our builders today:<br><br><a href="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-lld-x86_64-scei-ps4-windows10pro-fast/builds/21573</a><br><a href="http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14095">http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14095</a><br><br>. . . <br>Failing Tests (1):<br> Clang-Unit :: Basic/./BasicTests.exe/FileManagerTest.getFileDefersOpen<br><br>Please have a look ASAP?<br><br>Thanks<br><br>Galina<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 19, 2018 at 5:40 AM Sam McCall via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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>