From d0f2c4fcf79222d5c6f3c17188a811b0a47833c6 Mon Sep 17 00:00:00 2001 From: Tannin Date: Wed, 17 Jul 2013 20:58:58 +0200 Subject: - now avoids a few unnecessary copy operations during generation of the directory structure - bugfix: circular dependency caused a memory leak - bugfix: removing a single mod lead to the wrong mod being deleted --- src/shared/directoryentry.cpp | 134 +++++++++++++++++++++++++++++++----------- src/shared/directoryentry.h | 53 ++++++++++------- src/shared/leaktrace.cpp | 109 ++++++++++++++++++++++++++++++++++ src/shared/leaktrace.h | 24 ++++++++ src/shared/shared.pro | 8 ++- src/shared/util.cpp | 35 +++++++---- 6 files changed, 296 insertions(+), 67 deletions(-) create mode 100644 src/shared/leaktrace.cpp create mode 100644 src/shared/leaktrace.h (limited to 'src/shared') diff --git a/src/shared/directoryentry.cpp b/src/shared/directoryentry.cpp index 165af798..ceba113d 100644 --- a/src/shared/directoryentry.cpp +++ b/src/shared/directoryentry.cpp @@ -27,10 +27,17 @@ along with Mod Organizer. If not, see . #include "util.h" #include "windows_error.h" #include +#include "util.h" +#include "leaktrace.h" + +#include + namespace MOShared { +static int s_Count = 0; + class OriginConnection { @@ -43,7 +50,9 @@ public: OriginConnection() : m_NextID(0) - {} + { + LEAK_TRACE; + } FilesOrigin& createOrigin(const std::wstring &originName, const std::wstring &directory, int priority, boost::shared_ptr fileRegister, boost::shared_ptr originConnection) { @@ -123,7 +132,7 @@ void FilesOrigin::enable(bool enabled) if (!enabled) { std::set copy = m_Files; for (auto iter = copy.begin(); iter != copy.end(); ++iter) { - m_FileRegister->removeOrigin(*iter, m_ID); + m_FileRegister.lock()->removeOrigin(*iter, m_ID); } m_Files.clear(); } @@ -151,9 +160,41 @@ std::wstring tail(const std::wstring &source, const size_t count) } +FilesOrigin::FilesOrigin() + : m_ID(0), m_Disabled(false), m_Name(), m_Path(), m_Priority(0) +{ + LEAK_TRACE; +} + +FilesOrigin::FilesOrigin(const FilesOrigin &reference) + : m_ID(reference.m_ID) + , m_Disabled(reference.m_Disabled) + , m_Name(reference.m_Name) + , m_Path(reference.m_Path) + , m_Priority(reference.m_Priority) + , m_FileRegister(reference.m_FileRegister) + , m_OriginConnection(reference.m_OriginConnection) +{ + LEAK_TRACE; +} + + +FilesOrigin::FilesOrigin(int ID, const std::wstring &name, const std::wstring &path, int priority, boost::shared_ptr fileRegister, boost::shared_ptr originConnection) + : m_ID(ID), m_Disabled(false), m_Name(name), m_Path(path), m_Priority(priority), + m_FileRegister(fileRegister), m_OriginConnection(originConnection) +{ + LEAK_TRACE; +} + +FilesOrigin::~FilesOrigin() +{ + LEAK_UNTRACE; +} + + void FilesOrigin::setPriority(int priority) { - m_OriginConnection->changePriorityLookup(m_Priority, priority); + m_OriginConnection.lock()->changePriorityLookup(m_Priority, priority); m_Priority = priority; } @@ -161,7 +202,7 @@ void FilesOrigin::setPriority(int priority) void FilesOrigin::setName(const std::wstring &name) { - m_OriginConnection->changeNameLookup(m_Name, name); + m_OriginConnection.lock()->changeNameLookup(m_Name, name); // change path too if (tail(m_Path, m_Name.length()) == m_Name) { m_Path = m_Path.substr(0, m_Path.length() - m_Name.length()).append(name); @@ -169,12 +210,12 @@ void FilesOrigin::setName(const std::wstring &name) m_Name = name; } -std::vector FilesOrigin::getFiles() const +std::vector FilesOrigin::getFiles() const { - std::vector result; + std::vector result; for (auto iter = m_Files.begin(); iter != m_Files.end(); ++iter) { - result.push_back(m_FileRegister->getFile(*iter)); + result.push_back(m_FileRegister.lock()->getFile(*iter)); } return result; @@ -275,11 +316,22 @@ static bool ByOriginPriority(DirectoryEntry *entry, int LHS, int RHS) } +FileEntry::FileEntry() + : m_Index(UINT_MAX), m_Name(), m_Parent(NULL) +{ + LEAK_TRACE; +} + FileEntry::FileEntry(Index index, const std::wstring &name, DirectoryEntry *parent) : m_Index(index), m_Name(name), m_Parent(parent), m_Origin(-1), m_Archive(L"") { + LEAK_TRACE; } +FileEntry::~FileEntry() +{ + LEAK_UNTRACE; +} void FileEntry::sortOrigins() { @@ -322,28 +374,35 @@ std::wstring FileEntry::getRelativePath() const } - - - // // DirectoryEntry // DirectoryEntry::DirectoryEntry(const std::wstring &name, DirectoryEntry *parent, int originID) : m_OriginConnection(new OriginConnection), - m_Name(name), m_Parent(parent), m_Populated(false), m_Origin(originID) + m_Name(name), m_Parent(parent), m_Populated(false), m_Origin(originID), m_TopLevel(true) { m_FileRegister.reset(new FileRegister(m_OriginConnection)); + LEAK_TRACE; } DirectoryEntry::DirectoryEntry(const std::wstring &name, DirectoryEntry *parent, int originID, boost::shared_ptr fileRegister, boost::shared_ptr originConnection) : m_FileRegister(fileRegister), m_OriginConnection(originConnection), - m_Name(name), m_Parent(parent), m_Populated(false), m_Origin(originID) -{} + m_Name(name), m_Parent(parent), m_Populated(false), m_Origin(originID), m_TopLevel(false) +{ + LEAK_TRACE; +} DirectoryEntry::~DirectoryEntry() { +/* if (m_TopLevel) { + if (m_FileRegister.use_count() > 1) { +log("this should not happen"); + delete m_FileRegister.get(); + } + }*/ + LEAK_UNTRACE; clear(); } @@ -573,7 +632,7 @@ int DirectoryEntry::anyOrigin() const { bool ignore; for (auto iter = m_Files.begin(); iter != m_Files.end(); ++iter) { - FileEntry *entry = m_FileRegister->getFile(iter->second); + FileEntry::Ptr entry = m_FileRegister->getFile(iter->second); if (!entry->isFromArchive()) { return entry->getOrigin(ignore); } @@ -612,8 +671,8 @@ FilesOrigin &DirectoryEntry::getOriginByName(const std::wstring &name) const int DirectoryEntry::getOrigin(const std::wstring &path, bool &archive) { const DirectoryEntry *directory = NULL; - const FileEntry *file = searchFile(path, &directory); - if (file != NULL) { + const FileEntry::Ptr file = searchFile(path, &directory); + if (file.get() != NULL) { return file->getOrigin(archive); } else { if (directory != NULL) { @@ -624,9 +683,9 @@ int DirectoryEntry::getOrigin(const std::wstring &path, bool &archive) } } -std::vector DirectoryEntry::getFiles() const +std::vector DirectoryEntry::getFiles() const { - std::vector result; + std::vector result; for (auto iter = m_Files.begin(); iter != m_Files.end(); ++iter) { result.push_back(m_FileRegister->getFile(iter->second)); } @@ -634,7 +693,7 @@ std::vector DirectoryEntry::getFiles() const } -const FileEntry *DirectoryEntry::searchFile(const std::wstring &path, const DirectoryEntry **directory) const +const FileEntry::Ptr DirectoryEntry::searchFile(const std::wstring &path, const DirectoryEntry **directory) const { if (directory != NULL) { *directory = NULL; @@ -682,7 +741,7 @@ DirectoryEntry *DirectoryEntry::findSubDirectory(const std::wstring &name) const } -const FileEntry *DirectoryEntry::findFile(const std::wstring &name) +const FileEntry::Ptr DirectoryEntry::findFile(const std::wstring &name) { auto iter = m_Files.find(name); if (iter != m_Files.end()) { @@ -734,8 +793,16 @@ DirectoryEntry *DirectoryEntry::getSubDirectoryRecursive(const std::wstring &pat FileRegister::FileRegister(boost::shared_ptr originConnection) : m_OriginConnection(originConnection) { + LEAK_TRACE; } +FileRegister::~FileRegister() +{ + LEAK_UNTRACE; + m_Files.clear(); +} + + FileEntry::Index FileRegister::generateIndex() { static FileEntry::Index sIndex = 0; @@ -747,38 +814,38 @@ bool FileRegister::indexValid(FileEntry::Index index) const return m_Files.find(index) != m_Files.end(); } -FileEntry &FileRegister::createFile(const std::wstring &name, DirectoryEntry *parent) +FileEntry::Ptr FileRegister::createFile(const std::wstring &name, DirectoryEntry *parent) { FileEntry::Index index = generateIndex(); - m_Files[index] = FileEntry(index, name, parent); + m_Files[index] = FileEntry::Ptr(new FileEntry(index, name, parent)); return m_Files[index]; } -FileEntry *FileRegister::getFile(FileEntry::Index index) +FileEntry::Ptr FileRegister::getFile(FileEntry::Index index) { auto iter = m_Files.find(index); if (iter != m_Files.end()) { - return &iter->second; + return iter->second; } return NULL; } -void FileRegister::unregisterFile(FileEntry &file) +void FileRegister::unregisterFile(FileEntry::Ptr file) { bool ignore; // unregister from origin - int originID = file.getOrigin(ignore); - m_OriginConnection->getByID(originID).removeFile(file.getIndex()); - const std::vector &alternatives = file.getAlternatives(); + int originID = file->getOrigin(ignore); + m_OriginConnection->getByID(originID).removeFile(file->getIndex()); + const std::vector &alternatives = file->getAlternatives(); for (auto iter = alternatives.begin(); iter != alternatives.end(); ++iter) { - m_OriginConnection->getByID(*iter).removeFile(file.getIndex()); + m_OriginConnection->getByID(*iter).removeFile(file->getIndex()); } // unregister from directory - if (file.getParent() != NULL) { - file.getParent()->removeFile(file.getIndex()); + if (file->getParent() != NULL) { + file->getParent()->removeFile(file->getIndex()); } } @@ -796,7 +863,7 @@ void FileRegister::removeOrigin(FileEntry::Index index, int originID) { auto iter = m_Files.find(index); if (iter != m_Files.end()) { - if (iter->second.removeOrigin(originID)) { + if (iter->second->removeOrigin(originID)) { unregisterFile(iter->second); } } @@ -805,7 +872,8 @@ void FileRegister::removeOrigin(FileEntry::Index index, int originID) void FileRegister::sortOrigins() { for (auto iter = m_Files.begin(); iter != m_Files.end(); ++iter) { - iter->second.sortOrigins(); + iter->second->sortOrigins(); } } + } // namespace MOShared diff --git a/src/shared/directoryentry.h b/src/shared/directoryentry.h index c6d155de..498ebfe1 100644 --- a/src/shared/directoryentry.h +++ b/src/shared/directoryentry.h @@ -20,6 +20,7 @@ along with Mod Organizer. If not, see . #ifndef DIRECTORYENTRY_H #define DIRECTORYENTRY_H + #include #include #include @@ -29,6 +30,7 @@ along with Mod Organizer. If not, see . #include #include #include +#include #include "util.h" @@ -46,12 +48,16 @@ public: typedef unsigned int Index; + typedef boost::shared_ptr Ptr; + public: - FileEntry() : m_Index(UINT_MAX), m_Name(), m_Parent(NULL) {} + FileEntry(); FileEntry(Index index, const std::wstring &name, DirectoryEntry *parent); + ~FileEntry(); + Index getIndex() const { return m_Index; } void addOrigin(int origin, FILETIME fileTime, const std::wstring &archive); @@ -105,8 +111,9 @@ class FilesOrigin { friend class OriginConnection; public: - FilesOrigin() - : m_ID(0), m_Disabled(false), m_Name(), m_Path(), m_Priority(0) { } + FilesOrigin(); + FilesOrigin(const FilesOrigin &reference); + ~FilesOrigin(); // sets priority for this origin, but it will overwrite the exisiting mapping for this priority, // the previous origin will no longer be referenced @@ -120,7 +127,7 @@ public: int getID() const { return m_ID; } const std::wstring &getPath() const { return m_Path; } - std::vector getFiles() const; + std::vector getFiles() const; void enable(bool enabled); bool isDisabled() const { return m_Disabled; } @@ -131,9 +138,8 @@ public: private: FilesOrigin(int ID, const std::wstring &name, const std::wstring &path, int priority, - boost::shared_ptr fileRegister, boost::shared_ptr originConnection) - : m_ID(ID), m_Disabled(false), m_Name(name), m_Path(path), m_Priority(priority), - m_FileRegister(fileRegister), m_OriginConnection(originConnection) {} + boost::shared_ptr fileRegister, boost::shared_ptr originConnection); + private: @@ -145,8 +151,8 @@ private: std::wstring m_Name; std::wstring m_Path; int m_Priority; - boost::shared_ptr m_FileRegister; - boost::shared_ptr m_OriginConnection; + boost::weak_ptr m_FileRegister; + boost::weak_ptr m_OriginConnection; }; @@ -157,11 +163,12 @@ class FileRegister public: FileRegister(boost::shared_ptr originConnection); + ~FileRegister(); bool indexValid(FileEntry::Index index) const; - FileEntry &createFile(const std::wstring &name, DirectoryEntry *parent); - FileEntry *getFile(FileEntry::Index index); + FileEntry::Ptr createFile(const std::wstring &name, DirectoryEntry *parent); + FileEntry::Ptr getFile(FileEntry::Index index); void removeFile(FileEntry::Index index); void removeOrigin(FileEntry::Index index, int originID); @@ -172,11 +179,11 @@ private: FileEntry::Index generateIndex(); - void unregisterFile(FileEntry &file); + void unregisterFile(FileEntry::Ptr file); private: - std::map m_Files; + std::map m_Files; boost::shared_ptr m_OriginConnection; @@ -197,7 +204,6 @@ public: void clear(); bool isPopulated() const { return m_Populated; } - boost::shared_ptr getRegister() { return m_FileRegister; } const DirectoryEntry *getParent() const { return m_Parent; } // add files to this directory (and subdirectories) from the specified origin. That origin may exist or not @@ -214,7 +220,7 @@ public: int getOrigin(const std::wstring &path, bool &archive); - std::vector getFiles() const; + std::vector getFiles() const; void getSubDirectories(std::vector::const_iterator &begin, std::vector::const_iterator &end) const { begin = m_SubDirectories.begin(); end = m_SubDirectories.end(); @@ -226,12 +232,12 @@ public: * @param name name of the file * @return fileentry object for the file or NULL if no file matches */ - const FileEntry *findFile(const std::wstring &name); + const FileEntry::Ptr findFile(const std::wstring &name); /** search through this directory and all subdirectories for a file by the specified name. if directory is not NULL, the referenced variable will be set to true if the path refers to a directory. the returned pointer is NULL in that case */ - const FileEntry *searchFile(const std::wstring &path, const DirectoryEntry **directory) const; + const FileEntry::Ptr searchFile(const std::wstring &path, const DirectoryEntry **directory) const; void insertFile(const std::wstring &filePath, FilesOrigin &origin, FILETIME fileTime); @@ -250,8 +256,8 @@ public: auto iter = m_Files.find(fileName); if (iter != m_Files.end()) { if (origin != NULL) { - FileEntry *entry = m_FileRegister->getFile(iter->second); - if (entry != NULL) { + FileEntry::Ptr entry = m_FileRegister->getFile(iter->second); + if (entry.get() != NULL) { bool ignore; *origin = entry->getOrigin(ignore); } @@ -262,13 +268,16 @@ public: private: + DirectoryEntry(const DirectoryEntry &reference); + DirectoryEntry &operator=(const DirectoryEntry &reference); + void insert(const std::wstring &fileName, FilesOrigin &origin, FILETIME fileTime, const std::wstring &archive) { auto iter = m_Files.find(fileName); - FileEntry *file = NULL; + FileEntry::Ptr file; if (iter != m_Files.end()) { file = m_FileRegister->getFile(iter->second); } else { - file = &m_FileRegister->createFile(fileName, this); + file = m_FileRegister->createFile(fileName, this); m_Files[fileName] = file->getIndex(); } file->addOrigin(origin.getID(), fileTime, archive); @@ -310,6 +319,8 @@ private: bool m_Populated; + bool m_TopLevel; + }; diff --git a/src/shared/leaktrace.cpp b/src/shared/leaktrace.cpp new file mode 100644 index 00000000..0c618b68 --- /dev/null +++ b/src/shared/leaktrace.cpp @@ -0,0 +1,109 @@ +#include "leaktrace.h" +#include +#include +#include +#include +#include + + +static const int FRAMES_TO_SKIP = 3; // StackData::StackData(), __TraceData::regTrace(), TraceAlloc() +static const int FRAMES_TO_CAPTURE = 10; + + +void initDbgIfNecessary() +{ + HANDLE process = ::GetCurrentProcess(); + static std::set initialized; + if (initialized.find(::GetCurrentProcessId()) == initialized.end()) { + static bool firstCall = true; + if (firstCall) { + ::SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS); + firstCall = false; + } + if (!::SymInitialize(process, NULL, TRUE)) { + printf("failed to initialize symbols: %d", ::GetLastError()); + } + initialized.insert(::GetCurrentProcessId()); + } +} + + +class StackData { + friend bool operator==(const StackData &LHS, const StackData &RHS); + friend bool operator<(const StackData &LHS, const StackData &RHS); +public: + StackData() { + m_Count = ::CaptureStackBackTrace(FRAMES_TO_SKIP, FRAMES_TO_CAPTURE, m_Stack, &m_Hash); + } + std::string toString() const { + initDbgIfNecessary(); + + char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)]; + PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer; + symbol->SizeOfStruct = sizeof(SYMBOL_INFO); + symbol->MaxNameLen = MAX_SYM_NAME; + + std::ostringstream stackStream; + + for(unsigned int i = 0; i < m_Count; ++i) { + DWORD64 displacement = 0; + if (!::SymFromAddr(::GetCurrentProcess(), (DWORD64)m_Stack[i], &displacement, symbol)) { + stackStream << m_Count - i - 1 << ": [" << m_Stack[i] << "]\n"; + } else { + stackStream << m_Count - i - 1 << ": " << symbol->Name << "\n"; + } + } + return stackStream.str(); + } +private: + LPVOID m_Stack[FRAMES_TO_CAPTURE]; + USHORT m_Count; + ULONG m_Hash; +}; + +bool operator==(const StackData &LHS, const StackData &RHS) { + return LHS.m_Hash == RHS.m_Hash; +} + +bool operator<(const StackData &LHS, const StackData &RHS) { + return LHS.m_Hash < RHS.m_Hash; +} + + + +static struct __TraceData { + void regTrace(void *pointer) { + m_Traces[reinterpret_cast(pointer)] = StackData(); + } + void deregTrace(void *pointer) { + auto iter = m_Traces.find(reinterpret_cast(pointer)); + if (iter != m_Traces.end()) { + m_Traces.erase(iter); + } + } + + ~__TraceData() { + std::map result; + for (auto iter = m_Traces.begin(); iter != m_Traces.end(); ++iter) { + result[iter->second] += 1; + } + for (auto iter = result.begin(); iter != result.end(); ++iter) { + printf("-----------------------------------\n" + "%d objects not freed, allocated at:\n%s", + iter->second, iter->first.toString().c_str()); + } + } + + std::map m_Traces; +} __trace; + + +void LeakTrace::TraceAlloc(void *ptr) +{ + __trace.regTrace(ptr); +} + +void LeakTrace::TraceDealloc(void *ptr) +{ + __trace.deregTrace(ptr); +} diff --git a/src/shared/leaktrace.h b/src/shared/leaktrace.h new file mode 100644 index 00000000..78764260 --- /dev/null +++ b/src/shared/leaktrace.h @@ -0,0 +1,24 @@ +#ifndef LEAKTRACE_H +#define LEAKTRACE_H + + +namespace LeakTrace { + +void TraceAlloc(void *ptr); +void TraceDealloc(void *ptr); + +}; + +#ifdef TRACE_LEAKS + +#define LEAK_TRACE LeakTrace::TraceAlloc(this) +#define LEAK_UNTRACE LeakTrace::TraceDealloc(this) + +#else // TRACE_LEAKS + +#define LEAK_TRACE +#define LEAK_UNTRACE + +#endif // TRACE_LEAKS + +#endif // LEAKTRACE_H diff --git a/src/shared/shared.pro b/src/shared/shared.pro index 9489958a..ab0bd8a0 100644 --- a/src/shared/shared.pro +++ b/src/shared/shared.pro @@ -15,7 +15,7 @@ INCLUDEPATH += ../bsatk "$(BOOSTPATH)" CONFIG(debug, debug|release) { LIBS += -L$$OUT_PWD/../bsatk/debug - + LIBS += -lDbgHelp } else { LIBS += -L$$OUT_PWD/../bsatk/release } @@ -39,7 +39,8 @@ SOURCES += \ falloutnvinfo.cpp \ util.cpp \ skyriminfo.cpp \ - appconfig.cpp + appconfig.cpp \ + leaktrace.cpp HEADERS += \ inject.h \ @@ -53,4 +54,5 @@ HEADERS += \ util.h \ skyriminfo.h \ appconfig.h \ - appconfig.inc + appconfig.inc \ + leaktrace.h diff --git a/src/shared/util.cpp b/src/shared/util.cpp index 1c4bf5b6..4378e03c 100644 --- a/src/shared/util.cpp +++ b/src/shared/util.cpp @@ -24,6 +24,7 @@ along with Mod Organizer. If not, see . #include #include #include +#include namespace MOShared { @@ -147,24 +148,38 @@ VS_FIXEDFILEINFO GetFileVersion(const std::wstring &fileName) std::string GetStack() { -#ifdef DEBUG +#ifdef _DEBUG HANDLE process = ::GetCurrentProcess(); - static bool firstCall = true; - if (firstCall) { - ::SymInitialize(process, NULL, TRUE); - firstCall = false; + static std::set initialized; + if (initialized.find(::GetCurrentProcessId()) == initialized.end()) { + static bool firstCall = true; + if (firstCall) { + ::SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS); + firstCall = false; + } + if (!::SymInitialize(process, NULL, TRUE)) { + log("failed to initialize symbols: %d", ::GetLastError()); + } + initialized.insert(::GetCurrentProcessId()); } LPVOID stack[32]; WORD frames = ::CaptureStackBackTrace(0, 100, stack, NULL); - SYMBOL_INFO_PACKAGE symbol; - symbol.si.SizeOfStruct = sizeof(SYMBOL_INFO); - symbol.si.MaxNameLen = MAX_SYM_NAME; + + char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(TCHAR)]; + PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer; + symbol->SizeOfStruct = sizeof(SYMBOL_INFO); + symbol->MaxNameLen = MAX_SYM_NAME; std::ostringstream stackStream; for(unsigned int i = 0; i < frames; ++i) { - ::SymFromAddr(process, (DWORD64)(stack[i]), 0, &symbol.si); - stackStream << frames - i - 1 << ": " << symbol.si.Name << "\n"; + DWORD64 addr = (DWORD64)stack[i]; + DWORD64 displacement = 0; + if (!::SymFromAddr(::GetCurrentProcess(), addr, &displacement, symbol)) { + stackStream << frames - i - 1 << ": " << stack[i] << " - " << ::GetLastError() << " (error)\n"; + } else { + stackStream << frames - i - 1 << ": " << symbol->Name << "\n"; + } } return stackStream.str(); #else -- cgit v1.3.1