From 88b00c0761dc588d14d37517a71d96be9690119a Mon Sep 17 00:00:00 2001
From: David Blaikie <dblaikie@gmail.com>
Date: Mon, 11 Aug 2014 21:29:24 +0000
Subject: [PATCH] unique_ptr-ify FileSystemStatCache::setNextStatCache
And in the process, discover that FileManager::removeStatCache had a
double-delete when removing an element from the middle of the list (at
the beginning or the end of the list, there was no problem) and add a
unit test to exercise the code path (which successfully crashed when run
(with modifications to match the old API) without this patch applied)
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@215388 91177308-0d34-0410-b5e6-96231b3b80d8
---
include/clang/Basic/FileManager.h | 3 +-
include/clang/Basic/FileSystemStatCache.h | 4 +--
include/clang/Lex/PTHManager.h | 2 +-
lib/Basic/FileManager.cpp | 12 ++++----
lib/Frontend/CacheTokens.cpp | 6 ++--
lib/Lex/PTHLexer.cpp | 4 +--
lib/Lex/Preprocessor.cpp | 1 +
unittests/Basic/FileManagerTest.cpp | 35 ++++++++++++++---------
8 files changed, 40 insertions(+), 27 deletions(-)
diff --git a/include/clang/Basic/FileManager.h b/include/clang/Basic/FileManager.h
index 0ad53c4df5a..fbfbb49c1a5 100644
--- a/include/clang/Basic/FileManager.h
+++ b/include/clang/Basic/FileManager.h
@@ -194,7 +194,8 @@ public:
/// \param AtBeginning whether this new stat cache must be installed at the
/// beginning of the chain of stat caches. Otherwise, it will be added to
/// the end of the chain.
- void addStatCache(FileSystemStatCache *statCache, bool AtBeginning = false);
+ void addStatCache(std::unique_ptr<FileSystemStatCache> statCache,
+ bool AtBeginning = false);
/// \brief Removes the specified FileSystemStatCache object from the manager.
void removeStatCache(FileSystemStatCache *statCache);
diff --git a/include/clang/Basic/FileSystemStatCache.h b/include/clang/Basic/FileSystemStatCache.h
index f30d3fcba30..a685c960c7f 100644
--- a/include/clang/Basic/FileSystemStatCache.h
+++ b/include/clang/Basic/FileSystemStatCache.h
@@ -74,8 +74,8 @@ public:
/// \brief Sets the next stat call cache in the chain of stat caches.
/// Takes ownership of the given stat cache.
- void setNextStatCache(FileSystemStatCache *Cache) {
- NextStatCache.reset(Cache);
+ void setNextStatCache(std::unique_ptr<FileSystemStatCache> Cache) {
+ NextStatCache = std::move(Cache);
}
/// \brief Retrieve the next stat call cache in the chain.
diff --git a/include/clang/Lex/PTHManager.h b/include/clang/Lex/PTHManager.h
index 11b5ceaad1d..41fefdb321a 100644
--- a/include/clang/Lex/PTHManager.h
+++ b/include/clang/Lex/PTHManager.h
@@ -131,7 +131,7 @@ public:
/// FileManager objects. These objects use the PTH data to speed up
/// calls to stat by memoizing their results from when the PTH file
/// was generated.
- FileSystemStatCache *createStatCache();
+ std::unique_ptr<FileSystemStatCache> createStatCache();
};
} // end namespace clang
diff --git a/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp
index 94bd3f8ccfd..e4a7f1e2575 100644
--- a/lib/Basic/FileManager.cpp
+++ b/lib/Basic/FileManager.cpp
@@ -64,20 +64,20 @@ FileManager::~FileManager() {
delete VirtualDirectoryEntries[i];
}
-void FileManager::addStatCache(FileSystemStatCache *statCache,
+void FileManager::addStatCache(std::unique_ptr<FileSystemStatCache> statCache,
bool AtBeginning) {
assert(statCache && "No stat cache provided?");
if (AtBeginning || !StatCache.get()) {
- statCache->setNextStatCache(StatCache.release());
- StatCache.reset(statCache);
+ statCache->setNextStatCache(std::move(StatCache));
+ StatCache = std::move(statCache);
return;
}
FileSystemStatCache *LastCache = StatCache.get();
while (LastCache->getNextStatCache())
LastCache = LastCache->getNextStatCache();
-
- LastCache->setNextStatCache(statCache);
+
+ LastCache->setNextStatCache(std::move(statCache));
}
void FileManager::removeStatCache(FileSystemStatCache *statCache) {
@@ -96,7 +96,7 @@ void FileManager::removeStatCache(FileSystemStatCache *statCache) {
PrevCache = PrevCache->getNextStatCache();
assert(PrevCache && "Stat cache not found for removal");
- PrevCache->setNextStatCache(statCache->getNextStatCache());
+ PrevCache->setNextStatCache(statCache->takeNextStatCache());
}
void FileManager::clearStatCaches() {
diff --git a/lib/Frontend/CacheTokens.cpp b/lib/Frontend/CacheTokens.cpp
index 14f7027e468..23f22ada7fc 100644
--- a/lib/Frontend/CacheTokens.cpp
+++ b/lib/Frontend/CacheTokens.cpp
@@ -572,8 +572,10 @@ void clang::CacheTokens(Preprocessor &PP, llvm::raw_fd_ostream* OS) {
PTHWriter PW(*OS, PP);
// Install the 'stat' system call listener in the FileManager.
- StatListener *StatCache = new StatListener(PW.getPM());
- PP.getFileManager().addStatCache(StatCache, /*AtBeginning=*/true);
+ auto StatCacheOwner = llvm::make_unique<StatListener>(PW.getPM());
+ StatListener *StatCache = StatCacheOwner.get();
+ PP.getFileManager().addStatCache(std::move(StatCacheOwner),
+ /*AtBeginning=*/true);
// Lex through the entire file. This will populate SourceManager with
// all of the header information.
diff --git a/lib/Lex/PTHLexer.cpp b/lib/Lex/PTHLexer.cpp
index fce31c4be29..4e2a755f8c6 100644
--- a/lib/Lex/PTHLexer.cpp
+++ b/lib/Lex/PTHLexer.cpp
@@ -732,6 +732,6 @@ public:
};
} // end anonymous namespace
-FileSystemStatCache *PTHManager::createStatCache() {
- return new PTHStatCache(*((PTHFileLookup*) FileLookup));
+std::unique_ptr<FileSystemStatCache> PTHManager::createStatCache() {
+ return llvm::make_unique<PTHStatCache>(*((PTHFileLookup *)FileLookup));
}
diff --git a/lib/Lex/Preprocessor.cpp b/lib/Lex/Preprocessor.cpp
index ab11fabbbce..fd0e48c66c8 100644
--- a/lib/Lex/Preprocessor.cpp
+++ b/lib/Lex/Preprocessor.cpp
@@ -27,6 +27,7 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemStatCache.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/Lex/CodeCompletionHandler.h"
diff --git a/unittests/Basic/FileManagerTest.cpp b/unittests/Basic/FileManagerTest.cpp
index b3bc767949e..e53213b4bcc 100644
--- a/unittests/Basic/FileManagerTest.cpp
+++ b/unittests/Basic/FileManagerTest.cpp
@@ -97,7 +97,7 @@ TEST_F(FileManagerTest, NoVirtualDirectoryExistsBeforeAVirtualFileIsAdded) {
// FileManager to report "file/directory doesn't exist". This
// avoids the possibility of the result of this test being affected
// by what's in the real file system.
- manager.addStatCache(new FakeStatCache);
+ manager.addStatCache(llvm::make_unique<FakeStatCache>());
EXPECT_EQ(nullptr, manager.getDirectory("virtual/dir/foo"));
EXPECT_EQ(nullptr, manager.getDirectory("virtual/dir"));
@@ -107,7 +107,7 @@ TEST_F(FileManagerTest, NoVirtualDirectoryExistsBeforeAVirtualFileIsAdded) {
// When a virtual file is added, all of its ancestors should be created.
TEST_F(FileManagerTest, getVirtualFileCreatesDirectoryEntriesForAncestors) {
// Fake an empty real file system.
- manager.addStatCache(new FakeStatCache);
+ manager.addStatCache(llvm::make_unique<FakeStatCache>());
manager.getVirtualFile("virtual/dir/bar.h", 100, 0);
EXPECT_EQ(nullptr, manager.getDirectory("virtual/dir/foo"));
@@ -124,7 +124,7 @@ TEST_F(FileManagerTest, getVirtualFileCreatesDirectoryEntriesForAncestors) {
// getFile() returns non-NULL if a real file exists at the given path.
TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingRealFile) {
// Inject fake files into the file system.
- FakeStatCache *statCache = new FakeStatCache;
+ auto statCache = llvm::make_unique<FakeStatCache>();
statCache->InjectDirectory("/tmp", 42);
statCache->InjectFile("/tmp/test", 43);
@@ -135,7 +135,7 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingRealFile) {
statCache->InjectFile(FileName, 45);
#endif
- manager.addStatCache(statCache);
+ manager.addStatCache(std::move(statCache));
const FileEntry *file = manager.getFile("/tmp/test");
ASSERT_TRUE(file != nullptr);
@@ -158,7 +158,7 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingRealFile) {
// 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(new FakeStatCache);
+ manager.addStatCache(llvm::make_unique<FakeStatCache>());
manager.getVirtualFile("virtual/dir/bar.h", 100, 0);
const FileEntry *file = manager.getFile("virtual/dir/bar.h");
@@ -175,11 +175,11 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingVirtualFile) {
TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) {
// Inject two fake files into the file system. Different inodes
// mean the files are not symlinked together.
- FakeStatCache *statCache = new FakeStatCache;
+ auto statCache = llvm::make_unique<FakeStatCache>();
statCache->InjectDirectory(".", 41);
statCache->InjectFile("foo.cpp", 42);
statCache->InjectFile("bar.cpp", 43);
- manager.addStatCache(statCache);
+ manager.addStatCache(std::move(statCache));
const FileEntry *fileFoo = manager.getFile("foo.cpp");
const FileEntry *fileBar = manager.getFile("bar.cpp");
@@ -192,10 +192,10 @@ TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) {
// exists at the given path.
TEST_F(FileManagerTest, getFileReturnsNULLForNonexistentFile) {
// Inject a fake foo.cpp into the file system.
- FakeStatCache *statCache = new FakeStatCache;
+ auto statCache = llvm::make_unique<FakeStatCache>();
statCache->InjectDirectory(".", 41);
statCache->InjectFile("foo.cpp", 42);
- manager.addStatCache(statCache);
+ manager.addStatCache(std::move(statCache));
// Create a virtual bar.cpp file.
manager.getVirtualFile("bar.cpp", 200, 0);
@@ -211,11 +211,11 @@ TEST_F(FileManagerTest, getFileReturnsNULLForNonexistentFile) {
// getFile() returns the same FileEntry for real files that are aliases.
TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedRealFiles) {
// Inject two real files with the same inode.
- FakeStatCache *statCache = new FakeStatCache;
+ auto statCache = llvm::make_unique<FakeStatCache>();
statCache->InjectDirectory("abc", 41);
statCache->InjectFile("abc/foo.cpp", 42);
statCache->InjectFile("abc/bar.cpp", 42);
- manager.addStatCache(statCache);
+ manager.addStatCache(std::move(statCache));
EXPECT_EQ(manager.getFile("abc/foo.cpp"), manager.getFile("abc/bar.cpp"));
}
@@ -224,11 +224,11 @@ TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedRealFiles) {
// corresponding real files that are aliases.
TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
// Inject two real files with the same inode.
- FakeStatCache *statCache = new FakeStatCache;
+ auto statCache = llvm::make_unique<FakeStatCache>();
statCache->InjectDirectory("abc", 41);
statCache->InjectFile("abc/foo.cpp", 42);
statCache->InjectFile("abc/bar.cpp", 42);
- manager.addStatCache(statCache);
+ manager.addStatCache(std::move(statCache));
manager.getVirtualFile("abc/foo.cpp", 100, 0);
manager.getVirtualFile("abc/bar.cpp", 200, 0);
@@ -236,6 +236,15 @@ TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
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);
+}
+
#endif // !LLVM_ON_WIN32
} // anonymous namespace
--
GitLab