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

@@ -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);

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.

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

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() {

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.

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));
 }

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"

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