r212541 - Improve memory ownership of vfs::Files in the FileSystemStatCache by using std::unique_ptr
David Blaikie
dblaikie at gmail.com
Tue Jul 8 08:46:02 PDT 2014
Author: dblaikie
Date: Tue Jul 8 10:46:02 2014
New Revision: 212541
URL: http://llvm.org/viewvc/llvm-project?rev=212541&view=rev
Log:
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.
Modified:
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/include/clang/Basic/FileSystemStatCache.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Basic/FileSystemStatCache.cpp
cfe/trunk/lib/Frontend/CacheTokens.cpp
cfe/trunk/lib/Lex/PTHLexer.cpp
Modified: cfe/trunk/include/clang/Basic/FileManager.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/FileManager.h (original)
+++ cfe/trunk/include/clang/Basic/FileManager.h Tue Jul 8 10:46:02 2014
@@ -172,7 +172,7 @@ class FileManager : public RefCountedBas
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.
Modified: cfe/trunk/include/clang/Basic/FileSystemStatCache.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileSystemStatCache.h?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/FileSystemStatCache.h (original)
+++ cfe/trunk/include/clang/Basic/FileSystemStatCache.h Tue Jul 8 10:46:02 2014
@@ -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
Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Tue Jul 8 10:46:02 2014
@@ -253,7 +253,7 @@ const FileEntry *FileManager::getFile(St
// 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(St
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(St
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
/// 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())
Modified: cfe/trunk/lib/Basic/FileSystemStatCache.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileSystemStatCache.cpp?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/lib/Basic/FileSystemStatCache.cpp (original)
+++ cfe/trunk/lib/Basic/FileSystemStatCache.cpp Tue Jul 8 10:46:02 2014
@@ -52,8 +52,8 @@ static void copyStatusToFileData(const v
/// 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
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
// 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
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
Modified: cfe/trunk/lib/Frontend/CacheTokens.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CacheTokens.cpp?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/CacheTokens.cpp (original)
+++ cfe/trunk/lib/Frontend/CacheTokens.cpp Tue Jul 8 10:46:02 2014
@@ -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'.
Modified: cfe/trunk/lib/Lex/PTHLexer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PTHLexer.cpp?rev=212541&r1=212540&r2=212541&view=diff
==============================================================================
--- cfe/trunk/lib/Lex/PTHLexer.cpp (original)
+++ cfe/trunk/lib/Lex/PTHLexer.cpp Tue Jul 8 10:46:02 2014
@@ -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);
More information about the cfe-commits
mailing list