r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

Nico Weber via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 23 05:35:44 PST 2019


On Wed, Jan 23, 2019 at 5:44 AM Sam McCall <sammccall at 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 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/0d7476d3/attachment-0001.html>


More information about the cfe-commits mailing list