diff --git a/Core/Debugger/SymbolMap.cpp b/Core/Debugger/SymbolMap.cpp index 4c6948106c..057ade51b5 100644 --- a/Core/Debugger/SymbolMap.cpp +++ b/Core/Debugger/SymbolMap.cpp @@ -50,13 +50,10 @@ SymbolMap *g_symbolMap; void SymbolMap::SortSymbols() { - std::lock_guard guard(lock_); - AssignFunctionIndices(); } void SymbolMap::Clear() { - std::lock_guard guard(lock_); functions.clear(); labels.clear(); data.clear(); @@ -69,9 +66,8 @@ void SymbolMap::Clear() { } bool SymbolMap::LoadSymbolMap(const Path &filename) { - Clear(); // let's not recurse the lock + Clear(); - std::lock_guard guard(lock_); // TODO(scoped): Use gzdopen instead. @@ -203,8 +199,6 @@ bool SymbolMap::LoadSymbolMap(const Path &filename) { } bool SymbolMap::SaveSymbolMap(const Path &filename) const { - std::lock_guard guard(lock_); - // Don't bother writing a blank file. if (!File::Exists(filename) && functions.empty() && data.empty()) { return true; @@ -307,7 +301,6 @@ bool SymbolMap::SaveSymbolMap(const Path &filename) const { } bool SymbolMap::LoadNocashSym(const Path &filename) { - std::lock_guard guard(lock_); FILE *f = File::OpenCFile(filename, "r"); if (!f) return false; @@ -364,8 +357,6 @@ bool SymbolMap::LoadNocashSym(const Path &filename) { } bool SymbolMap::SaveNocashSym(const Path &filename) const { - std::lock_guard guard(lock_); - // Don't bother writing a blank file. if (!File::Exists(filename) && functions.empty() && data.empty()) { return false; @@ -389,7 +380,6 @@ SymbolType SymbolMap::GetSymbolType(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); if (activeFunctions.find(address) != activeFunctions.end()) return ST_FUNCTION; if (activeData.find(address) != activeData.end()) @@ -439,7 +429,6 @@ u32 SymbolMap::GetNextSymbolAddress(u32 address, SymbolType symmask) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); const auto functionEntry = symmask & ST_FUNCTION ? activeFunctions.upper_bound(address) : activeFunctions.end(); const auto dataEntry = symmask & ST_DATA ? activeData.upper_bound(address) : activeData.end(); @@ -456,7 +445,6 @@ u32 SymbolMap::GetNextSymbolAddress(u32 address, SymbolType symmask) { } std::string SymbolMap::GetDescription(unsigned int address) { - std::lock_guard guard(lock_); std::string labelName; u32 funcStart = GetFunctionStart(address); @@ -483,7 +471,6 @@ std::vector SymbolMap::GetAllActiveSymbols(SymbolType symmask) { std::vector result; if (symmask & ST_FUNCTION) { - std::lock_guard guard(lock_); for (auto it = activeFunctions.begin(); it != activeFunctions.end(); it++) { SymbolEntry entry; entry.address = it->first; @@ -496,7 +483,6 @@ std::vector SymbolMap::GetAllActiveSymbols(SymbolType symmask) { } if (symmask & ST_DATA) { - std::lock_guard guard(lock_); for (auto it = activeData.begin(); it != activeData.end(); it++) { SymbolEntry entry; entry.address = it->first; @@ -512,8 +498,6 @@ std::vector SymbolMap::GetAllActiveSymbols(SymbolType symmask) { } void SymbolMap::AddModule(const char *name, u32 address, u32 size) { - std::lock_guard guard(lock_); - for (auto it = modules.begin(), end = modules.end(); it != end; ++it) { if (!strcmp(it->name, name)) { // Just reactivate that one. @@ -537,13 +521,11 @@ void SymbolMap::AddModule(const char *name, u32 address, u32 size) { } void SymbolMap::UnloadModule(u32 address, u32 size) { - std::lock_guard guard(lock_); activeModuleEnds.erase(address + size); activeNeedUpdate_ = true; } u32 SymbolMap::GetModuleRelativeAddr(u32 address, int moduleIndex) const { - std::lock_guard guard(lock_); if (moduleIndex == -1) { moduleIndex = GetModuleIndex(address); } @@ -557,7 +539,6 @@ u32 SymbolMap::GetModuleRelativeAddr(u32 address, int moduleIndex) const { } u32 SymbolMap::GetModuleAbsoluteAddr(u32 relative, int moduleIndex) const { - std::lock_guard guard(lock_); for (auto it = modules.begin(), end = modules.end(); it != end; ++it) { if (it->index == moduleIndex) { return it->start + relative; @@ -567,7 +548,6 @@ u32 SymbolMap::GetModuleAbsoluteAddr(u32 relative, int moduleIndex) const { } int SymbolMap::GetModuleIndex(u32 address) const { - std::lock_guard guard(lock_); auto iter = activeModuleEnds.upper_bound(address); if (iter == activeModuleEnds.end()) return -1; @@ -579,7 +559,6 @@ bool SymbolMap::IsModuleActive(int moduleIndex) { return true; } - std::lock_guard guard(lock_); for (auto it = activeModuleEnds.begin(), end = activeModuleEnds.end(); it != end; ++it) { if (it->second.index == moduleIndex) { return true; @@ -589,8 +568,6 @@ bool SymbolMap::IsModuleActive(int moduleIndex) { } std::vector SymbolMap::getAllModules() const { - std::lock_guard guard(lock_); - std::vector result; for (size_t i = 0; i < modules.size(); i++) { LoadedModuleInfo m; @@ -608,8 +585,6 @@ std::vector SymbolMap::getAllModules() const { } void SymbolMap::AddFunction(const char* name, u32 address, u32 size, int moduleIndex) { - std::lock_guard guard(lock_); - if (moduleIndex == -1) { moduleIndex = GetModuleIndex(address); } else if (moduleIndex == 0) { @@ -661,7 +636,6 @@ u32 SymbolMap::GetFunctionStart(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeFunctions.upper_bound(address); if (it == activeFunctions.end()) { // check last element @@ -691,7 +665,6 @@ u32 SymbolMap::FindPossibleFunctionAtAfter(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeFunctions.lower_bound(address); if (it == activeFunctions.end()) { return (u32)-1; @@ -701,7 +674,6 @@ u32 SymbolMap::FindPossibleFunctionAtAfter(u32 address) { u32 SymbolMap::GetFunctionSize(u32 startAddress) { if (activeNeedUpdate_) { - std::lock_guard guard(lock_); // This is common, from the jit. Direct lookup is faster than updating active symbols. auto mod = activeModuleEnds.lower_bound(startAddress); @@ -726,7 +698,6 @@ u32 SymbolMap::GetFunctionSize(u32 startAddress) { return func->second.size; } - std::lock_guard guard(lock_); auto it = activeFunctions.find(startAddress); if (it == activeFunctions.end()) return INVALID_ADDRESS; @@ -738,7 +709,6 @@ u32 SymbolMap::GetFunctionModuleAddress(u32 startAddress) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeFunctions.find(startAddress); if (it == activeFunctions.end()) return INVALID_ADDRESS; @@ -750,7 +720,6 @@ int SymbolMap::GetFunctionNum(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); u32 start = GetFunctionStart(address); if (start == INVALID_ADDRESS) return INVALID_ADDRESS; @@ -763,7 +732,6 @@ int SymbolMap::GetFunctionNum(u32 address) { } void SymbolMap::AssignFunctionIndices() { - std::lock_guard guard(lock_); int index = 0; for (auto mod = activeModuleEnds.begin(), modend = activeModuleEnds.end(); mod != modend; ++mod) { int moduleIndex = mod->second.index; @@ -777,8 +745,6 @@ void SymbolMap::AssignFunctionIndices() { // Copies functions, labels and data to the active set depending on which modules are "active". void SymbolMap::UpdateActiveSymbols() { - std::lock_guard guard(lock_); - activeFunctions.clear(); activeLabels.clear(); activeData.clear(); @@ -828,7 +794,6 @@ bool SymbolMap::SetFunctionSize(u32 startAddress, u32 newSize) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto funcInfo = activeFunctions.find(startAddress); if (funcInfo != activeFunctions.end()) { @@ -849,7 +814,6 @@ bool SymbolMap::RemoveFunction(u32 startAddress, bool removeName) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeFunctions.find(startAddress); if (it == activeFunctions.end()) @@ -878,8 +842,6 @@ bool SymbolMap::RemoveFunction(u32 startAddress, bool removeName) { } void SymbolMap::AddLabel(const char* name, u32 address, int moduleIndex) { - std::lock_guard guard(lock_); - if (moduleIndex == -1) { moduleIndex = GetModuleIndex(address); } else if (moduleIndex == 0) { @@ -929,7 +891,6 @@ void SymbolMap::SetLabelName(const char* name, u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto labelInfo = activeLabels.find(address); if (labelInfo == activeLabels.end()) { AddLabel(name, address); @@ -954,7 +915,6 @@ const char *SymbolMap::GetLabelName(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeLabels.find(address); if (it == activeLabels.end()) return NULL; @@ -963,7 +923,6 @@ const char *SymbolMap::GetLabelName(u32 address) { } const char *SymbolMap::GetLabelNameRel(u32 relAddress, int moduleIndex) const { - std::lock_guard guard(lock_); auto it = labels.find(std::make_pair(moduleIndex, relAddress)); if (it == labels.end()) return NULL; @@ -972,7 +931,6 @@ const char *SymbolMap::GetLabelNameRel(u32 relAddress, int moduleIndex) const { } std::string SymbolMap::GetLabelString(u32 address) { - std::lock_guard guard(lock_); const char *label = GetLabelName(address); if (label == NULL) return ""; @@ -983,7 +941,6 @@ bool SymbolMap::GetLabelValue(const char* name, u32& dest) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); for (auto it = activeLabels.begin(); it != activeLabels.end(); it++) { if (strcasecmp(name, it->second.name) == 0) { dest = it->first; @@ -995,8 +952,6 @@ bool SymbolMap::GetLabelValue(const char* name, u32& dest) { } void SymbolMap::AddData(u32 address, u32 size, DataType type, int moduleIndex) { - std::lock_guard guard(lock_); - if (moduleIndex == -1) { moduleIndex = GetModuleIndex(address); } else if (moduleIndex == 0) { @@ -1047,7 +1002,6 @@ u32 SymbolMap::GetDataStart(u32 address) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeData.upper_bound(address); if (it == activeData.end()) { @@ -1080,7 +1034,6 @@ u32 SymbolMap::GetDataSize(u32 startAddress) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeData.find(startAddress); if (it == activeData.end()) return INVALID_ADDRESS; @@ -1091,7 +1044,6 @@ u32 SymbolMap::GetDataModuleAddress(u32 startAddress) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeData.find(startAddress); if (it == activeData.end()) return INVALID_ADDRESS; @@ -1102,7 +1054,6 @@ DataType SymbolMap::GetDataType(u32 startAddress) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeData.find(startAddress); if (it == activeData.end()) return DATATYPE_NONE; @@ -1113,7 +1064,6 @@ bool SymbolMap::RemoveData(u32 startAddress, bool removeName) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); auto it = activeData.find(startAddress); if (it == activeData.end()) @@ -1145,7 +1095,6 @@ void SymbolMap::GetLabels(std::vector &dest) { if (activeNeedUpdate_) UpdateActiveSymbols(); - std::lock_guard guard(lock_); for (auto it = activeLabels.begin(); it != activeLabels.end(); it++) { LabelDefinition entry; entry.value = it->first; @@ -1176,7 +1125,6 @@ void SymbolMap::FillSymbolListBox(HWND listbox,SymbolType symType) { UpdateActiveSymbols(); wchar_t temp[256]; - std::lock_guard guard(lock_); SendMessage(listbox, WM_SETREDRAW, FALSE, 0); ListBox_ResetContent(listbox); diff --git a/Core/Debugger/SymbolMap.h b/Core/Debugger/SymbolMap.h index 4451647686..3095e494fd 100644 --- a/Core/Debugger/SymbolMap.h +++ b/Core/Debugger/SymbolMap.h @@ -20,7 +20,6 @@ #include #include #include -#include #include "Common/CommonTypes.h" #include "Common/File/Path.h" @@ -172,7 +171,6 @@ private: std::map data; std::vector modules; - mutable std::recursive_mutex lock_; bool sawUnknownModule = false; }; diff --git a/Windows/MainWindow.cpp b/Windows/MainWindow.cpp index d1752faa9e..00ed6b8e34 100644 --- a/Windows/MainWindow.cpp +++ b/Windows/MainWindow.cpp @@ -725,8 +725,12 @@ namespace MainWindow { { return 0; } - // Get all modules from symbol map - auto modules = g_symbolMap->getAllModules(); + // Get all modules from symbol map. Reading it here on the GUI thread would otherwise + // race with the CPU thread - hold g_frameMutex for the duration of the read, which + // NativeFrame() also holds while it's actually touching that state. See g_frameMutex + // in Core.h. + std::lock_guard frameGuard(g_frameMutex); + std::vector modules = g_symbolMap->getAllModules(); for (const auto& module : modules) { if (module.name == moduleName) diff --git a/Windows/MainWindowMenu.cpp b/Windows/MainWindowMenu.cpp index b83a8184be..d4a5a148d8 100644 --- a/Windows/MainWindowMenu.cpp +++ b/Windows/MainWindowMenu.cpp @@ -769,34 +769,41 @@ namespace MainWindow { break; } + // g_symbolMap access below is routed through Core_RunOnCPUThread() instead of poking at it + // directly from this GUI thread - see Core_RunOnCPUThread() in Core.h. The file-browse + // dialogs are modal, so they stay outside the queued callback; NotifyDebuggerMapLoaded() + // also stays outside, since it locks g_frameMutex internally (CDisasm/CMemoryDlg's + // NotifyMapLoaded()) and Core_RunOnCPUThread's queue can be drained from inside a + // g_frameMutex-locked NativeFrame() span, so calling it from within the lambda could + // deadlock. case ID_DEBUG_LOADMAPFILE: if (W32Util::BrowseForFileName(true, hWnd, L"Load .ppmap", nullptr, nullptr, L"Maps\0*.ppmap\0All files\0*.*\0\0", L"ppmap", fn)) { - g_symbolMap->LoadSymbolMap(Path(fn)); + Core_RunOnCPUThread([&] { g_symbolMap->LoadSymbolMap(Path(fn)); }); NotifyDebuggerMapLoaded(); } break; case ID_DEBUG_SAVEMAPFILE: if (W32Util::BrowseForFileName(false, hWnd, L"Save .ppmap", nullptr, L"map.ppmap", L"Maps\0*.ppmap\0All files\0*.*\0\0", L"ppmap", fn)) { - g_symbolMap->SaveSymbolMap(Path(fn)); + Core_RunOnCPUThread([&] { g_symbolMap->SaveSymbolMap(Path(fn)); }); } break; case ID_DEBUG_LOADSYMFILE: if (W32Util::BrowseForFileName(true, hWnd, L"Load .sym", nullptr, nullptr, L"Symbols\0*.sym\0All files\0*.*\0\0", L"sym", fn)) { - g_symbolMap->LoadNocashSym(Path(fn)); + Core_RunOnCPUThread([&] { g_symbolMap->LoadNocashSym(Path(fn)); }); NotifyDebuggerMapLoaded(); } break; case ID_DEBUG_SAVESYMFILE: if (W32Util::BrowseForFileName(false, hWnd, L"Save .sym", nullptr, L"symbols.sym", L"Symbols\0*.sym\0All files\0*.*\0\0", L"sym", fn)) { - g_symbolMap->SaveNocashSym(Path(fn)); + Core_RunOnCPUThread([&] { g_symbolMap->SaveNocashSym(Path(fn)); }); } break; case ID_DEBUG_RESETSYMBOLTABLE: - g_symbolMap->Clear(); + Core_RunOnCPUThread([&] { g_symbolMap->Clear(); }); NotifyDebuggerMapLoaded(); break; diff --git a/Windows/main.cpp b/Windows/main.cpp index a02613cc2f..25c4cd15b3 100644 --- a/Windows/main.cpp +++ b/Windows/main.cpp @@ -508,7 +508,7 @@ void System_Notify(SystemNotification notification) { case SystemNotification::BOOT_DONE: { if (g_symbolMap) - g_symbolMap->SortSymbols(); // internal locking is performed here + g_symbolMap->SortSymbols(); // Always fired from the CPU/NativeFrame thread, see AGENTS.md PostMessage(MainWindow::GetHWND(), WM_USER + 1, 0, 0); if (Achievements::HardcoreModeActive()) { @@ -548,7 +548,7 @@ void System_Notify(SystemNotification notification) { case SystemNotification::SYMBOL_MAP_UPDATED: if (g_symbolMap) - g_symbolMap->SortSymbols(); // internal locking is performed here + g_symbolMap->SortSymbols(); // Always fired from the CPU/NativeFrame thread, see AGENTS.md PostMessage(MainWindow::GetHWND(), WM_USER + 1, 0, 0); break;