From: Erik Verbruggen Date: Tue, 28 Mar 2017 09:18:05 +0000 (+0000) Subject: FileManager: mark virtual file entries as valid entries X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=121956bde3695cafabb07a61866ad2432fb31ae9;p=clang FileManager: mark virtual file entries as valid entries 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@298905 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp index 646c1910bf..0c10b5f4d1 100644 --- a/lib/Basic/FileManager.cpp +++ b/lib/Basic/FileManager.cpp @@ -386,6 +386,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size, UFE->ModTime = ModificationTime; UFE->Dir = DirInfo; UFE->UID = NextFileUID++; + UFE->IsValid = true; UFE->File.reset(); return UFE; } diff --git a/unittests/Basic/FileManagerTest.cpp b/unittests/Basic/FileManagerTest.cpp index 4cf21cd845..a19535e104 100644 --- a/unittests/Basic/FileManagerTest.cpp +++ b/unittests/Basic/FileManagerTest.cpp @@ -12,6 +12,7 @@ #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; @@ -29,6 +30,12 @@ private: llvm::StringMap 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; @@ -55,6 +62,12 @@ public: LookupResult getStat(StringRef Path, FileData &Data, bool isFile, std::unique_ptr *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; @@ -140,6 +153,7 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingRealFile) { 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(); @@ -164,6 +178,7 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingVirtualFile) { 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(); @@ -185,7 +200,9 @@ TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) { 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); } @@ -231,8 +248,8 @@ TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) { 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")); } @@ -246,6 +263,37 @@ TEST_F(FileManagerTest, addRemoveStatCache) { 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(); + 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