From 87e7bf9da1bac263ba7645ebf64447afae4db9d1 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 8 Jul 2014 15:46:02 +0000 Subject: [PATCH] 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 | 2 +- include/clang/Basic/FileSystemStatCache.h | 13 +++++++++---- lib/Basic/FileManager.cpp | 10 +++------- lib/Basic/FileSystemStatCache.cpp | 13 +++++-------- lib/Frontend/CacheTokens.cpp | 3 ++- lib/Lex/PTHLexer.cpp | 5 ++--- 6 files changed, 22 insertions(+), 24 deletions(-) diff --git a/include/clang/Basic/FileManager.h b/include/clang/Basic/FileManager.h index f0f0d2f2e1..dd1ad0da1d 100644 --- a/include/clang/Basic/FileManager.h +++ b/include/clang/Basic/FileManager.h @@ -172,7 +172,7 @@ class FileManager : public RefCountedBase { std::unique_ptr StatCache; bool getStatValue(const char *Path, FileData &Data, bool isFile, - vfs::File **F); + std::unique_ptr *F); /// Add all ancestors of the given path (pointing to either a file /// or a directory) as virtual directories. diff --git a/include/clang/Basic/FileSystemStatCache.h b/include/clang/Basic/FileSystemStatCache.h index e916cefba1..9be8b1074b 100644 --- a/include/clang/Basic/FileSystemStatCache.h +++ b/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 *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&> 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 *F, + vfs::FileSystem &FS) = 0; LookupResult statChained(const char *Path, FileData &Data, bool isFile, - vfs::File **F, vfs::FileSystem &FS) { + std::unique_ptr *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 *F, + vfs::FileSystem &FS) override; }; } // end namespace clang diff --git a/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp index 1faa370e82..22beed7dcc 100644 --- a/lib/Basic/FileManager.cpp +++ b/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 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 *F) { // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be // absolute! if (FileSystemOpts.WorkingDir.empty()) diff --git a/lib/Basic/FileSystemStatCache.cpp b/lib/Basic/FileSystemStatCache.cpp index 4952ef49ef..7515cfb440 100644 --- a/lib/Basic/FileSystemStatCache.cpp +++ b/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 *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 *F, vfs::FileSystem &FS) { LookupResult Result = statChained(Path, Data, isFile, F, FS); // Do not cache failed stats, it is easy to construct common inconsistent diff --git a/lib/Frontend/CacheTokens.cpp b/lib/Frontend/CacheTokens.cpp index d30196d6df..14f7027e46 100644 --- a/lib/Frontend/CacheTokens.cpp +++ b/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 *F, + vfs::FileSystem &FS) override { LookupResult Result = statChained(Path, Data, isFile, F, FS); if (Result == CacheMissing) // Failed 'stat'. diff --git a/lib/Lex/PTHLexer.cpp b/lib/Lex/PTHLexer.cpp index 53805f6034..fce31c4be2 100644 --- a/lib/Lex/PTHLexer.cpp +++ b/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 *F, + vfs::FileSystem &FS) override { // Do the lookup for the file's data in the PTH file. CacheTy::iterator I = Cache.find(Path); -- 2.50.1