Improve memory ownership of vfs::Files in the FileSystemStatCache by using std::unique_ptr

Spotted after a memory leak (due to the complexities of manual memory
management) was fixed in 212466.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@212541 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/FileManager.h

@@ -172,7 +172,7 @@ class FileManager : public RefCountedBase<FileManager> {
   std::unique_ptr<FileSystemStatCache> StatCache;
 
   bool getStatValue(const char *Path, FileData &Data, bool isFile,
-                    vfs::File **F);
+                    std::unique_ptr<vfs::File> *F);
 
   /// Add all ancestors of the given path (pointing to either a file
   /// or a directory) as virtual directories.

include/clang/Basic/FileSystemStatCache.h

@@ -69,7 +69,7 @@ public:
   /// implementation can optionally fill in \p F with a valid \p File object and
   /// the client guarantees that it will close it.
   static bool get(const char *Path, FileData &Data, bool isFile,
-                  vfs::File **F, FileSystemStatCache *Cache,
+                  std::unique_ptr<vfs::File> *F, FileSystemStatCache *Cache,
                   vfs::FileSystem &FS);
 
   /// \brief Sets the next stat call cache in the chain of stat caches.
@@ -87,11 +87,15 @@ public:
   FileSystemStatCache *takeNextStatCache() { return NextStatCache.release(); }
 
 protected:
+  // FIXME: The pointer here is a non-owning/optional reference to the
+  // unique_ptr. Optional<unique_ptr<vfs::File>&> might be nicer, but
+  // Optional needs some work to support references so this isn't possible yet.
   virtual LookupResult getStat(const char *Path, FileData &Data, bool isFile,
-                               vfs::File **F, vfs::FileSystem &FS) = 0;
+                               std::unique_ptr<vfs::File> *F,
+                               vfs::FileSystem &FS) = 0;
 
   LookupResult statChained(const char *Path, FileData &Data, bool isFile,
-                           vfs::File **F, vfs::FileSystem &FS) {
+                           std::unique_ptr<vfs::File> *F, vfs::FileSystem &FS) {
     if (FileSystemStatCache *Next = getNextStatCache())
       return Next->getStat(Path, Data, isFile, F, FS);
 
@@ -116,7 +120,8 @@ public:
   iterator end() const { return StatCalls.end(); }
 
   LookupResult getStat(const char *Path, FileData &Data, bool isFile,
-                       vfs::File **F, vfs::FileSystem &FS) override;
+                       std::unique_ptr<vfs::File> *F,
+                       vfs::FileSystem &FS) override;
 };
 
 } // end namespace clang

lib/Basic/FileManager.cpp

@@ -253,7 +253,7 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   // FIXME: This will reduce the # syscalls.
 
   // Nope, there isn't.  Check to see if the file exists.
-  vfs::File *F = nullptr;
+  std::unique_ptr<vfs::File> F;
   FileData Data;
   if (getStatValue(InterndFileName, Data, true, openFile ? &F : nullptr)) {
     // There's no real file at the given path.
@@ -281,10 +281,6 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
     if (DirInfo != UFE.Dir && Data.IsVFSMapped)
       UFE.Dir = DirInfo;
 
-    // If the stat process opened the file, close it to avoid a FD leak.
-    if (F)
-      delete F;
-
     return &UFE;
   }
 
@@ -297,7 +293,7 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
-  UFE.File.reset(F);
+  UFE.File = std::move(F);
   UFE.IsValid = true;
   return &UFE;
 }
@@ -453,7 +449,7 @@ getBufferForFile(StringRef Filename, std::string *ErrorStr) {
 /// false if it's an existent real file.  If FileDescriptor is NULL,
 /// do directory look-up instead of file look-up.
 bool FileManager::getStatValue(const char *Path, FileData &Data, bool isFile,
-                               vfs::File **F) {
+                               std::unique_ptr<vfs::File> *F) {
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
   if (FileSystemOpts.WorkingDir.empty())

lib/Basic/FileSystemStatCache.cpp

@@ -52,8 +52,8 @@ static void copyStatusToFileData(const vfs::Status &Status,
 /// implementation can optionally fill in FileDescriptor with a valid
 /// descriptor and the client guarantees that it will close it.
 bool FileSystemStatCache::get(const char *Path, FileData &Data, bool isFile,
-                              vfs::File **F, FileSystemStatCache *Cache,
-                              vfs::FileSystem &FS) {
+                              std::unique_ptr<vfs::File> *F,
+                              FileSystemStatCache *Cache, vfs::FileSystem &FS) {
   LookupResult R;
   bool isForDir = !isFile;
 
@@ -92,7 +92,7 @@ bool FileSystemStatCache::get(const char *Path, FileData &Data, bool isFile,
       if (Status) {
         R = CacheExists;
         copyStatusToFileData(*Status, Data);
-        *F = OwnedFile.release();
+        *F = std::move(OwnedFile);
       } else {
         // fstat rarely fails.  If it does, claim the initial open didn't
         // succeed.
@@ -109,11 +109,8 @@ bool FileSystemStatCache::get(const char *Path, FileData &Data, bool isFile,
   // demands.
   if (Data.IsDirectory != isForDir) {
     // If not, close the file if opened.
-    if (F && *F) {
-      (*F)->close();
-      delete *F;
+    if (F)
       *F = nullptr;
-    }
     
     return true;
   }
@@ -123,7 +120,7 @@ bool FileSystemStatCache::get(const char *Path, FileData &Data, bool isFile,
 
 MemorizeStatCalls::LookupResult
 MemorizeStatCalls::getStat(const char *Path, FileData &Data, bool isFile,
-                           vfs::File **F, vfs::FileSystem &FS) {
+                           std::unique_ptr<vfs::File> *F, vfs::FileSystem &FS) {
   LookupResult Result = statChained(Path, Data, isFile, F, FS);
 
   // Do not cache failed stats, it is easy to construct common inconsistent

lib/Frontend/CacheTokens.cpp

@@ -540,7 +540,8 @@ public:
   ~StatListener() {}
 
   LookupResult getStat(const char *Path, FileData &Data, bool isFile,
-                       vfs::File **F, vfs::FileSystem &FS) override {
+                       std::unique_ptr<vfs::File> *F,
+                       vfs::FileSystem &FS) override {
     LookupResult Result = statChained(Path, Data, isFile, F, FS);
 
     if (Result == CacheMissing) // Failed 'stat'.

lib/Lex/PTHLexer.cpp

@@ -704,10 +704,9 @@ public:
     Cache(FL.getNumBuckets(), FL.getNumEntries(), FL.getBuckets(),
           FL.getBase()) {}
 
-  ~PTHStatCache() {}
-
   LookupResult getStat(const char *Path, FileData &Data, bool isFile,
-                       vfs::File **F, vfs::FileSystem &FS) override {
+                       std::unique_ptr<vfs::File> *F,
+                       vfs::FileSystem &FS) override {
     // Do the lookup for the file's data in the PTH file.
     CacheTy::iterator I = Cache.find(Path);