r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 23 02:44:14 PST 2019
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 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 at 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 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
>>
> _______________________________________________
> 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/20190123/bb858290/attachment-0001.html>
More information about the cfe-commits
mailing list