FileManager: mark virtual file entries as valid entries

[erikjv]

Summary

The getVirtualFile method would create entries for e.g. libclang's
CXUnsavedFile but not mark them as valid. The effect is that a lookup
through getFile where the file name is not exactly matching the virtual
file (e.g. through mixing slashes and backslashes on Windows) would
result in a normal file "lookup", and re-using the file entry found
by using the UniqueID, and overwrite the file entry fields. Because the
lookup involves opening the file, and moving it into the file entry, the
file is now open. The SourceManager keys its buffers on the UniqueID
(which is still the same), so it will find an already loaded buffer.
Because only the loading a buffer from disk will close the file, the
FileEntry will hold on to an open file for as long as the FileManager
is around. As the FileManager will only get destroyed at a reparse,
you can't safe to the "leaked" and locked file on Windows.

[besc]

I found a crash when applying this patch to 3.9.1 final (source archives from http://llvm.org/releases/download.html#3.9.1) and compiling LLVM/Clang with MinGW-w64 GCC 6.2.0 or MSVC 2015. Clang-format sometimes crashes with input from stdin, output to stdout and a command line like this:

clang-format.exe -style=file -assume-filename=C:/path/to/file.hpp

I’m not familiar with the code base, so these are just my observations from a user’s point of view. From what I found the problem could be linked to the first slash/backslash in an absolute path:

Crash

• C:/path/to/file.hpp
• C:/path\to\file.hpp
• /path/to/file.hpp
• /path\to\file.hpp

Works

• Backslashes only always work.
• C:\path/to/file.hpp
• \path/to/file.hpp
• path/to/file.hpp
• path/to\file.hpp

Header file (.hpp) or source file (.cpp) does not make a difference.

A crash produces this error message:

Wrote crash dump file "C:\Users\besc\AppData\Local\Temp\clang-format.exe-e00fa3.dmp"
0x0000000000459504 (0x0000000000000000 0x0000000000000000 0x0000000000000000 0x000000000022E300)
0x000000000047A45F (0x000000000022E3F0 0x0000000000000015 0x000000000022E5AC 0x0000000004000004)
0x000000000047174F (0x000000000022EE20 0x000000000022ED30 0x0000000000000000 0x000000000094846C)
0x0000000000403744 (0x0000000000A77F30 0x0000000077CC0A7A 0x0000000000857120 0x000000000022FDB0)
0x00000000006E9CBF (0x0000000000000002 0x000000000000004D 0x000000000085ABD0 0x0000000000000000)
0x00000000004013E8 (0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000)
0x000000000040151B (0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000)
0x0000000077B859CD (0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000), BaseThreadInitThunk() + 0xD bytes(s)
0x0000000077CBA561 (0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000), RtlUserThreadStart() + 0x21 bytes(s)

A corresponding dump file is attached:

clang-format.exe-e00fa3.dmp27 KBDownload

I also tested a vanilla 3.9.1 final, built with the same compilers: works fine with all the above slash combinations.

[yaron.keren]

lib/Basic/FileManager.cpp

218

I'd use a larger SmallString<256>, with large projects 128 bytes are frequently not enough.

[erikjv]

besc: I can't reproduce any crash. I tried trunk and the release_39 branch with msvc2015.

[erikjv]

lib/Basic/FileManager.cpp

218

Well, the calling sides often use 128, so that's why I stuck with it.

[erikjv]

Fixed all failing tests on Windows.

[klimek]

lib/Basic/FileManager.cpp

167–168

I'd add a canonicalizePath function that:
-> on unix just returns the path
-> on windows, does the ::native dance and returns that path

Then we can call that when we access the cache or write to the cache instead of sprinkling #ifdefs around.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1716–1717 ↗

(On Diff #87376)

I'd do changes like this in a separate patch.

test/CodeGen/debug-prefix-map.c

21 ↗

(On Diff #87376)

That is super unfortunate.
If we want tests to be able to run on windows, I think we might just want to give clang a flag to always show paths in unix format. That also should be something we could just make the file manager do (I think whatever the filemanager stores will be what is visible to the user).

[kfunk]

@erikjv Ready for review? Does this reliably fix https://bugreports.qt.io/browse/QTCREATORBUG-15449?

[erikjv]

@kfunk yes and yes, so @bkramer or @klimek : ping?

[bkramer]

I assume this is fine but I don't really understand what's going on. A test case would be great.

[klimek]

Is the diff view on phab broken, or am I missing something? I only see a single line of diff now, and don't see a way to change the diff.

[erikjv]

@klimek no, it's a 1 line fix. The rest was the previous version of the patch.

[bruno]

Please attach a testcase!

[erikjv]

Added a test for the specific scenario, and added asserts for validity of UFEs returned by getVirtualFile.

[bkramer]

lg

[erikjv]

Committed as r298905.

[cameron314]

The IsValid = true line coincidentally fixes a tangentially related bug -- see D20338 (in which I tried to introduce the same fix almost a year earlier, but nobody accepted the review). I guess I have to maintain the test for that case out of tree, though I could try to upstream it again (however, given that it's now passing with your change, I don't have much hope of it being accepted).

lib/Basic/FileManager.cpp

if (DirName.size() > 1 &&
     DirName = DirName.substr(0, DirName.size()-1);
 #ifdef LLVM_ON_WIN32
   // Fixing a problem with "clang C:test.c" on Windows.
   // Stat("C:") does not recognize "C:" as a valid directory
   std::string DirNameStr;
   if (DirName.size() > 1 && DirName.back() == ':' &&
       DirName.equals_lower(llvm::sys::path::root_name(DirName))) {
     DirNameStr = DirName.str() + '.';
     DirName = DirNameStr;
   }
 #endif
 
   ++NumDirLookups;
   auto &NamedDirEnt =
       *SeenDirEntries.insert(std::make_pair(DirName, nullptr)).first;
 
   // See if there was already an entry in the map.  Note that the map
   // contains both virtual and real directories.
#endif
   }
 
   return &UDE;
 }
 
 const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
                                       bool CacheFailure) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map.
   auto &NamedFileEnt =
       *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 (!UFE) {
     NamedFileEnt.second = UFE;
   }
 
   UFE->Name    = InterndFileName;
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = DirInfo;
   UFE->UID     = NextFileUID++;
+  UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty() 

unittests/Basic/FileManagerTest.cpp

 //===- unittests/Basic/FileMangerTest.cpp ------------ FileManger tests ---===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/FileSystemStatCache.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace clang;
 
 namespace {
 
 // Used to create a fake file system for running the tests with such
 // that the tests are not affected by the structure/contents of the
 // file system on the machine running the tests.
 class FakeStatCache : public FileSystemStatCache {
 private:
   // Maps a file/directory path to its desired stat result.  Anything
   // not in this map is considered to not exist in the file system.
   llvm::StringMap<FileData, llvm::BumpPtrAllocator> StatCalls;
 
   void InjectFileOrDirectory(const char *Path, ino_t INode, bool IsFile) {
+#ifndef LLVM_ON_WIN32
+    SmallString<128> NormalizedPath(Path);
+    llvm::sys::path::native(NormalizedPath);
+    Path = NormalizedPath.c_str();
+#endif
+
     FileData Data;
     Data.Name = Path;
     Data.Size = 0;
     Data.ModTime = 0;
     Data.UniqueID = llvm::sys::fs::UniqueID(1, INode);
     Data.IsDirectory = !IsFile;
     Data.IsNamedPipe = false;
     Data.InPCH = false;
public:
   void InjectDirectory(const char *Path, ino_t INode) {
     InjectFileOrDirectory(Path, INode, /*IsFile=*/false);
   }
 
   // Implement FileSystemStatCache::getStat().
   LookupResult getStat(StringRef Path, FileData &Data, bool isFile,
                        std::unique_ptr<vfs::File> *F,
                        vfs::FileSystem &FS) override {
+#ifndef LLVM_ON_WIN32
+    SmallString<128> NormalizedPath(Path);
+    llvm::sys::path::native(NormalizedPath);
+    Path = NormalizedPath.c_str();
+#endif
+
     if (StatCalls.count(Path) != 0) {
       Data = StatCalls[Path];
       return CacheExists;
     }
 
     return CacheMissing;  // This means the file/directory doesn't exist.
   }
 };
#ifdef LLVM_ON_WIN32
   statCache->InjectDirectory(DirName, 44);
   statCache->InjectFile(FileName, 45);
 #endif
 
   manager.addStatCache(std::move(statCache));
 
   const FileEntry *file = manager.getFile("/tmp/test");
   ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
   EXPECT_EQ("/tmp/test", file->getName());
 
   const DirectoryEntry *dir = file->getDir();
   ASSERT_TRUE(dir != nullptr);
   EXPECT_EQ("/tmp", dir->getName());
 
 #ifdef LLVM_ON_WIN32
   file = manager.getFile(FileName);
   ASSERT_TRUE(file != NULL);
 
   dir = file->getDir();
   ASSERT_TRUE(dir != NULL);
   EXPECT_EQ(DirName, dir->getName());
 #endif
 }
 
 // getFile() returns non-NULL if a virtual file exists at the given path.
 TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingVirtualFile) {
   // Fake an empty real file system.
   manager.addStatCache(llvm::make_unique<FakeStatCache>());
 
   manager.getVirtualFile("virtual/dir/bar.h", 100, 0);
   const FileEntry *file = manager.getFile("virtual/dir/bar.h");
   ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
   EXPECT_EQ("virtual/dir/bar.h", file->getName());
 
   const DirectoryEntry *dir = file->getDir();
   ASSERT_TRUE(dir != nullptr);
   EXPECT_EQ("virtual/dir", dir->getName());
 }
 
 // getFile() returns different FileEntries for different paths when
 // there's no aliasing.
 TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) {
   // Inject two fake files into the file system.  Different inodes
   // mean the files are not symlinked together.
   auto statCache = llvm::make_unique<FakeStatCache>();
   statCache->InjectDirectory(".", 41);
   statCache->InjectFile("foo.cpp", 42);
   statCache->InjectFile("bar.cpp", 43);
   manager.addStatCache(std::move(statCache));
 
   const FileEntry *fileFoo = manager.getFile("foo.cpp");
   const FileEntry *fileBar = manager.getFile("bar.cpp");
   ASSERT_TRUE(fileFoo != nullptr);
+  ASSERT_TRUE(fileFoo->isValid());
   ASSERT_TRUE(fileBar != nullptr);
+  ASSERT_TRUE(fileBar->isValid());
   EXPECT_NE(fileFoo, fileBar);
 }
 
 // getFile() returns NULL if neither a real file nor a virtual file
 // exists at the given path.
 TEST_F(FileManagerTest, getFileReturnsNULLForNonexistentFile) {
   // Inject a fake foo.cpp into the file system.
   auto statCache = llvm::make_unique<FakeStatCache>();

 TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
   // Inject two real files with the same inode.
   auto statCache = llvm::make_unique<FakeStatCache>();
   statCache->InjectDirectory("abc", 41);
   statCache->InjectFile("abc/foo.cpp", 42);
   statCache->InjectFile("abc/bar.cpp", 42);
   manager.addStatCache(std::move(statCache));
 
-  manager.getVirtualFile("abc/foo.cpp", 100, 0);
-  manager.getVirtualFile("abc/bar.cpp", 200, 0);
+  ASSERT_TRUE(manager.getVirtualFile("abc/foo.cpp", 100, 0)->isValid());
+  ASSERT_TRUE(manager.getVirtualFile("abc/bar.cpp", 200, 0)->isValid());
 
   EXPECT_EQ(manager.getFile("abc/foo.cpp"), manager.getFile("abc/bar.cpp"));
 }
 
 TEST_F(FileManagerTest, addRemoveStatCache) {
   manager.addStatCache(llvm::make_unique<FakeStatCache>());
   auto statCacheOwner = llvm::make_unique<FakeStatCache>();
   auto *statCache = statCacheOwner.get();
   manager.addStatCache(std::move(statCacheOwner));
   manager.addStatCache(llvm::make_unique<FakeStatCache>());
   manager.removeStatCache(statCache);
 }
 
+// getFile() Should return the same entry as getVirtualFile if the file actually
+// is a virtual file, even if the name is not exactly the same (but is after
+// normalisation done by the file system, like on Windows). This can be checked
+// here by checkng the size.
+TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique<FakeStatCache>();
+  statCache->InjectDirectory("c:\\tmp", 42);
+  statCache->InjectFile("c:\\tmp\\test", 43);
+
+  manager.addStatCache(std::move(statCache));
+
+  // Inject the virtual file:
+  const FileEntry *file1 = manager.getVirtualFile("c:\\tmp\\test", 123, 1);
+  ASSERT_TRUE(file1 != nullptr);
+  ASSERT_TRUE(file1->isValid());
+  EXPECT_EQ(43U, file1->getUniqueID().getFile());
+  EXPECT_EQ(123, file1->getSize());
+
+  // Lookup the virtual file with a different name:
+  const FileEntry *file2 = manager.getFile("c:/tmp/test", 100, 1);
+  ASSERT_TRUE(file2 != nullptr);
+  ASSERT_TRUE(file2->isValid());
+  // Check that it's the same UFE:
+  EXPECT_EQ(file1, file2);
+  EXPECT_EQ(43U, file2->getUniqueID().getFile());
+  // Check that the contents of the UFE are not overwritten by the entry in the
+  // filesystem:
+  EXPECT_EQ(123, file2->getSize());
+}
+
 #endif  // !LLVM_ON_WIN32
 
 } // anonymous namespace