r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.
Galina Kistanova via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 19 18:52:58 PST 2018
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 at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181119/e128b1d7/attachment.html>
More information about the cfe-commits
mailing list