Merge pull request #13786 from JosJuice/symboldb-locking

PPCSymbolDB: Improve locking

Source/Core/Common/SymbolDB.cpp

@@ -5,6 +5,7 @@
 
 #include <cstring>
 #include <map>
+#include <mutex>
 #include <string>
 #include <utility>
 
@@ -34,6 +35,7 @@ void Symbol::Rename(const std::string& symbol_name)
 
 void SymbolDB::List()
 {
+  std::lock_guard lock(m_mutex);
   for (const auto& func : m_functions)
   {
     DEBUG_LOG_FMT(OSHLE, "{} @ {:08x}: {} bytes (hash {:08x}) : {} calls", func.second.name,
@@ -44,11 +46,14 @@ void SymbolDB::List()
 
 bool SymbolDB::IsEmpty() const
 {
+  std::lock_guard lock(m_mutex);
   return m_functions.empty() && m_notes.empty();
 }
 
 bool SymbolDB::Clear(const char* prefix)
 {
+  std::lock_guard lock(m_mutex);
+
   // TODO: honor prefix
   m_map_name.clear();
   if (IsEmpty())
@@ -61,16 +66,24 @@ bool SymbolDB::Clear(const char* prefix)
 }
 
 void SymbolDB::Index()
+{
+  std::lock_guard lock(m_mutex);
+  Index(&m_functions);
+}
+
+void SymbolDB::Index(XFuncMap* functions)
 {
   int i = 0;
-  for (auto& func : m_functions)
+  for (auto& func : *functions)
   {
     func.second.index = i++;
   }
 }
 
-Symbol* SymbolDB::GetSymbolFromName(std::string_view name)
+const Symbol* SymbolDB::GetSymbolFromName(std::string_view name) const
 {
+  std::lock_guard lock(m_mutex);
+
   for (auto& func : m_functions)
   {
     if (func.second.function_name == name)
@@ -80,9 +93,10 @@ Symbol* SymbolDB::GetSymbolFromName(std::string_view name)
   return nullptr;
 }
 
-std::vector<Symbol*> SymbolDB::GetSymbolsFromName(std::string_view name)
+std::vector<const Symbol*> SymbolDB::GetSymbolsFromName(std::string_view name) const
 {
-  std::vector<Symbol*> symbols;
+  std::lock_guard lock(m_mutex);
+  std::vector<const Symbol*> symbols;
 
   for (auto& func : m_functions)
   {
@@ -93,8 +107,10 @@ std::vector<Symbol*> SymbolDB::GetSymbolsFromName(std::string_view name)
   return symbols;
 }
 
-Symbol* SymbolDB::GetSymbolFromHash(u32 hash)
+const Symbol* SymbolDB::GetSymbolFromHash(u32 hash) const
 {
+  std::lock_guard lock(m_mutex);
+
   auto iter = m_checksum_to_function.find(hash);
   if (iter == m_checksum_to_function.end())
     return nullptr;
@@ -102,8 +118,10 @@ Symbol* SymbolDB::GetSymbolFromHash(u32 hash)
   return *iter->second.begin();
 }
 
-std::vector<Symbol*> SymbolDB::GetSymbolsFromHash(u32 hash)
+std::vector<const Symbol*> SymbolDB::GetSymbolsFromHash(u32 hash) const
 {
+  std::lock_guard lock(m_mutex);
+
   const auto iter = m_checksum_to_function.find(hash);
 
   if (iter == m_checksum_to_function.cend())
@@ -114,6 +132,34 @@ std::vector<Symbol*> SymbolDB::GetSymbolsFromHash(u32 hash)
 
 void SymbolDB::AddCompleteSymbol(const Symbol& symbol)
 {
-  m_functions.emplace(symbol.address, symbol);
+  std::lock_guard lock(m_mutex);
+  m_functions[symbol.address] = symbol;
 }
+
+bool SymbolDB::RenameSymbol(const Symbol& symbol, const std::string& symbol_name)
+{
+  std::lock_guard lock(m_mutex);
+
+  auto it = m_functions.find(symbol.address);
+  if (it == m_functions.end())
+    return false;
+
+  it->second.Rename(symbol_name);
+  return true;
+}
+
+bool SymbolDB::RenameSymbol(const Symbol& symbol, const std::string& symbol_name,
+                            const std::string& object_name)
+{
+  std::lock_guard lock(m_mutex);
+
+  auto it = m_functions.find(symbol.address);
+  if (it == m_functions.end())
+    return false;
+
+  it->second.Rename(symbol_name);
+  it->second.object_name = object_name;
+  return true;
+}
+
 }  // namespace Common

Source/Core/Common/SymbolDB.h

@@ -7,12 +7,14 @@
 #pragma once
 
 #include <map>
+#include <mutex>
 #include <set>
 #include <string>
 #include <string_view>
 #include <utility>
 #include <vector>
 
+#include "Common/Assert.h"
 #include "Common/CommonTypes.h"
 
 namespace Core
@@ -87,27 +89,71 @@ public:
   SymbolDB();
   virtual ~SymbolDB();
 
-  virtual Symbol* GetSymbolFromAddr(u32 addr) { return nullptr; }
-  virtual Symbol* AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr) { return nullptr; }
+  virtual const Symbol* GetSymbolFromAddr(u32 addr) const { return nullptr; }
+  virtual const Symbol* AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr)
+  {
+    return nullptr;
+  }
   void AddCompleteSymbol(const Symbol& symbol);
+  bool RenameSymbol(const Symbol& symbol, const std::string& symbol_name);
+  bool RenameSymbol(const Symbol& symbol, const std::string& symbol_name,
+                    const std::string& object_name);
 
-  Symbol* GetSymbolFromName(std::string_view name);
-  std::vector<Symbol*> GetSymbolsFromName(std::string_view name);
-  Symbol* GetSymbolFromHash(u32 hash);
-  std::vector<Symbol*> GetSymbolsFromHash(u32 hash);
+  const Symbol* GetSymbolFromName(std::string_view name) const;
+  std::vector<const Symbol*> GetSymbolsFromName(std::string_view name) const;
+  const Symbol* GetSymbolFromHash(u32 hash) const;
+  std::vector<const Symbol*> GetSymbolsFromHash(u32 hash) const;
+
+  template <typename F>
+  void ForEachSymbol(F f) const
+  {
+    std::lock_guard lock(m_mutex);
+    for (const auto& [addr, symbol] : m_functions)
+      f(symbol);
+  }
+
+  template <typename F>
+  void ForEachSymbolWithMutation(F f)
+  {
+    std::lock_guard lock(m_mutex);
+    for (auto& [addr, symbol] : m_functions)
+    {
+      f(symbol);
+      ASSERT_MSG(COMMON, addr == symbol.address, "Symbol address was unexpectedly changed");
+    }
+  }
+
+  template <typename F>
+  void ForEachNote(F f) const
+  {
+    std::lock_guard lock(m_mutex);
+    for (const auto& [addr, note] : m_notes)
+      f(note);
+  }
+
+  template <typename F>
+  void ForEachNoteWithMutation(F f)
+  {
+    std::lock_guard lock(m_mutex);
+    for (auto& [addr, note] : m_notes)
+    {
+      f(note);
+      ASSERT_MSG(COMMON, addr == note.address, "Note address was unexpectedly changed");
+    }
+  }
 
-  const XFuncMap& Symbols() const { return m_functions; }
-  const XNoteMap& Notes() const { return m_notes; }
-  XFuncMap& AccessSymbols() { return m_functions; }
   bool IsEmpty() const;
   bool Clear(const char* prefix = "");
   void List();
   void Index();
 
 protected:
+  static void Index(XFuncMap* functions);
+
   XFuncMap m_functions;
   XNoteMap m_notes;
   XFuncPtrMap m_checksum_to_function;
   std::string m_map_name;
+  mutable std::recursive_mutex m_mutex;
 };
 }  // namespace Common

Source/Core/Core/Debugger/DebugInterface.h

@@ -6,7 +6,6 @@
 #include <cstddef>
 #include <optional>
 #include <string>
-#include <string_view>
 #include <vector>
 
 #include "Common/CommonTypes.h"
@@ -104,7 +103,7 @@ public:
   {
     return 0xFFFFFFFF;
   }
-  virtual std::string_view GetDescription(u32 /*address*/) const = 0;
+  virtual std::string GetDescription(u32 /*address*/) const = 0;
   virtual void Clear(const CPUThreadGuard& guard) = 0;
 };
 }  // namespace Core

Source/Core/Core/Debugger/Debugger_SymbolMap.cpp

@@ -80,7 +80,7 @@ bool GetCallstack(const Core::CPUThreadGuard& guard, std::vector<CallstackEntry>
   });
 
   WalkTheStack(guard, [&output, &ppc_symbol_db](u32 func_addr) {
-    std::string_view func_desc = ppc_symbol_db.GetDescription(func_addr);
+    std::string func_desc = ppc_symbol_db.GetDescription(func_addr);
     if (func_desc.empty() || func_desc == "Invalid")
       func_desc = "(unknown)";
 
@@ -107,14 +107,14 @@ void PrintCallstack(const Core::CPUThreadGuard& guard, Common::Log::LogType type
     GENERIC_LOG_FMT(type, level, " LR = 0 - this is bad");
   }
 
-  if (const std::string_view lr_desc = ppc_symbol_db.GetDescription(LR(ppc_state));
+  if (const std::string lr_desc = ppc_symbol_db.GetDescription(LR(ppc_state));
       lr_desc != ppc_symbol_db.GetDescription(ppc_state.pc))
   {
     GENERIC_LOG_FMT(type, level, " * {}  [ LR = {:08x} ]", lr_desc, LR(ppc_state));
   }
 
   WalkTheStack(guard, [type, level, &ppc_symbol_db](u32 func_addr) {
-    std::string_view func_desc = ppc_symbol_db.GetDescription(func_addr);
+    std::string func_desc = ppc_symbol_db.GetDescription(func_addr);
     if (func_desc.empty() || func_desc == "Invalid")
       func_desc = "(unknown)";
     GENERIC_LOG_FMT(type, level, " * {} [ addr = {:08x} ]", func_desc, func_addr);

Source/Core/Core/Debugger/PPCDebugInterface.cpp

@@ -439,7 +439,7 @@ u32 PPCDebugInterface::GetColor(const Core::CPUThreadGuard* guard, u32 address)
 }
 // =============
 
-std::string_view PPCDebugInterface::GetDescription(u32 address) const
+std::string PPCDebugInterface::GetDescription(u32 address) const
 {
   return m_ppc_symbol_db.GetDescription(address);
 }

Source/Core/Core/Debugger/PPCDebugInterface.h

@@ -102,7 +102,7 @@ public:
   void Step() override {}
   void RunTo(u32 address) override;
   u32 GetColor(const Core::CPUThreadGuard* guard, u32 address) const override;
-  std::string_view GetDescription(u32 address) const override;
+  std::string GetDescription(u32 address) const override;
 
   std::shared_ptr<Core::NetworkCaptureLogger> NetworkLogger();
 

Source/Core/Core/Debugger/RSO.cpp

@@ -381,7 +381,7 @@ void RSOView::Apply(const Core::CPUThreadGuard& guard, PPCSymbolDB* symbol_db) c
     u32 address = GetExportAddress(rso_export);
     if (address != 0)
     {
-      Common::Symbol* symbol = symbol_db->AddFunction(guard, address);
+      const Common::Symbol* symbol = symbol_db->AddFunction(guard, address);
       if (!symbol)
         symbol = symbol_db->GetSymbolFromAddr(address);
 
@@ -389,8 +389,7 @@ void RSOView::Apply(const Core::CPUThreadGuard& guard, PPCSymbolDB* symbol_db) c
       if (symbol)
       {
         // Function symbol
-        symbol->Rename(export_name);
-        symbol->object_name = rso_name;
+        symbol_db->RenameSymbol(*symbol, export_name, rso_name);
       }
       else
       {

Source/Core/Core/PowerPC/JitCommon/JitCache.cpp

@@ -187,7 +187,7 @@ void JitBaseBlockCache::FinalizeBlock(JitBlock& block, bool block_link,
     LinkBlock(block);
   }
 
-  Common::Symbol* symbol = nullptr;
+  const Common::Symbol* symbol = nullptr;
   if (Common::JitRegister::IsEnabled() &&
       (symbol = m_jit.m_ppc_symbol_db.GetSymbolFromAddr(block.effectiveAddress)) != nullptr)
   {

Source/Core/Core/PowerPC/PPCAnalyst.cpp

@@ -346,10 +346,10 @@ static void FindFunctionsFromHandlers(const Core::CPUThreadGuard& guard, PPCSymb
     if (read_result.valid && PPCTables::IsValidInstruction(read_result.hex, entry.first))
     {
       // Check if this function is already mapped
-      Common::Symbol* f = func_db->AddFunction(guard, entry.first);
+      const Common::Symbol* f = func_db->AddFunction(guard, entry.first);
       if (!f)
         continue;
-      f->Rename(entry.second);
+      func_db->RenameSymbol(*f, entry.second);
     }
   }
 }
@@ -359,8 +359,8 @@ static void FindFunctionsAfterReturnInstruction(const Core::CPUThreadGuard& guar
 {
   std::vector<u32> funcAddrs;
 
-  for (const auto& func : func_db->Symbols())
-    funcAddrs.push_back(func.second.address + func.second.size);
+  func_db->ForEachSymbol(
+      [&](const Common::Symbol& symbol) { funcAddrs.push_back(symbol.address + symbol.size); });
 
   auto& mmu = guard.GetSystem().GetMMU();
   for (u32& location : funcAddrs)
@@ -380,7 +380,7 @@ static void FindFunctionsAfterReturnInstruction(const Core::CPUThreadGuard& guar
       if (read_result.valid && PPCTables::IsValidInstruction(read_result.hex, location))
       {
         // check if this function is already mapped
-        Common::Symbol* f = func_db->AddFunction(guard, location);
+        const Common::Symbol* f = func_db->AddFunction(guard, location);
         if (!f)
           break;
         else
@@ -407,15 +407,14 @@ void FindFunctions(const Core::CPUThreadGuard& guard, u32 startAddr, u32 endAddr
   int numLeafs = 0, numNice = 0, numUnNice = 0;
   int numTimer = 0, numRFI = 0, numStraightLeaf = 0;
   int leafSize = 0, niceSize = 0, unniceSize = 0;
-  for (auto& func : func_db->AccessSymbols())
-  {
-    if (func.second.address == 4)
+  func_db->ForEachSymbolWithMutation([&](Common::Symbol& f) {
+    if (f.address == 4)
     {
       WARN_LOG_FMT(SYMBOLS, "Weird function");
-      continue;
     }
-    AnalyzeFunction2(func_db, &(func.second));
-    Common::Symbol& f = func.second;
+    else
+    {
+      AnalyzeFunction2(func_db, &f);
       if (f.name.substr(0, 3) == "zzz")
       {
         if (f.flags & Common::FFLAG_LEAF)
@@ -446,6 +445,7 @@ void FindFunctions(const Core::CPUThreadGuard& guard, u32 startAddr, u32 endAddr
       if ((f.flags & Common::FFLAG_STRAIGHT) && (f.flags & Common::FFLAG_LEAF))
         numStraightLeaf++;
     }
+  });
   if (numLeafs == 0)
     leafSize = 0;
   else

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

@@ -35,8 +35,10 @@ PPCSymbolDB::PPCSymbolDB() = default;
 PPCSymbolDB::~PPCSymbolDB() = default;
 
 // Adds the function to the list, unless it's already there
-Common::Symbol* PPCSymbolDB::AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr)
+const Common::Symbol* PPCSymbolDB::AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr)
 {
+  std::lock_guard lock(m_mutex);
+
   // It's already in the list
   if (m_functions.contains(start_addr))
     return nullptr;
@@ -56,8 +58,18 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
                                  const std::string& name, const std::string& object_name,
                                  Common::Symbol::Type type)
 {
-  auto iter = m_functions.find(startAddr);
-  if (iter != m_functions.end())
+  std::lock_guard lock(m_mutex);
+  AddKnownSymbol(guard, startAddr, size, name, object_name, type, &m_functions,
+                 &m_checksum_to_function);
+}
+
+void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAddr, u32 size,
+                                 const std::string& name, const std::string& object_name,
+                                 Common::Symbol::Type type, XFuncMap* functions,
+                                 XFuncPtrMap* checksum_to_function)
+{
+  auto iter = functions->find(startAddr);
+  if (iter != functions->end())
   {
     // already got it, let's just update name, checksum & size to be sure.
     Common::Symbol* tempfunc = &iter->second;
@@ -70,7 +82,7 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
   else
   {
     // new symbol. run analyze.
-    auto& new_symbol = m_functions.emplace(startAddr, name).first->second;
+    auto& new_symbol = functions->emplace(startAddr, name).first->second;
     new_symbol.object_name = object_name;
     new_symbol.type = type;
     new_symbol.address = startAddr;
@@ -85,7 +97,7 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
                      name, size, new_symbol.size);
         new_symbol.size = size;
       }
-      m_checksum_to_function[new_symbol.hash].insert(&new_symbol);
+      (*checksum_to_function)[new_symbol.hash].insert(&new_symbol);
     }
     else
     {
@@ -96,9 +108,15 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
 
 void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name)
 {
-  auto iter = m_notes.find(start_addr);
+  std::lock_guard lock(m_mutex);
+  AddKnownNote(start_addr, size, name, &m_notes);
+}
 
-  if (iter != m_notes.end())
+void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name, XNoteMap* notes)
+{
+  auto iter = notes->find(start_addr);
+
+  if (iter != notes->end())
   {
     // Already got it, just update the name and size.
     Common::Note* tempfunc = &iter->second;
@@ -112,35 +130,38 @@ void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name
     tf.address = start_addr;
     tf.size = size;
 
-    m_notes[start_addr] = tf;
+    (*notes)[start_addr] = tf;
   }
 }
 
 void PPCSymbolDB::DetermineNoteLayers()
 {
-  if (m_notes.empty())
+  std::lock_guard lock(m_mutex);
+  DetermineNoteLayers(&m_notes);
+}
+
+void PPCSymbolDB::DetermineNoteLayers(XNoteMap* notes)
+{
+  if (notes->empty())
     return;
 
-  for (auto& note : m_notes)
+  for (auto& note : *notes)
     note.second.layer = 0;
 
-  for (auto iter = m_notes.begin(); iter != m_notes.end(); ++iter)
+  for (auto iter = notes->begin(); iter != notes->end(); ++iter)
   {
     const u32 range = iter->second.address + iter->second.size;
-    auto search = m_notes.lower_bound(range);
+    auto search = notes->lower_bound(range);
 
     while (--search != iter)
       search->second.layer += 1;
   }
 }
 
-Common::Symbol* PPCSymbolDB::GetSymbolFromAddr(u32 addr)
+const Common::Symbol* PPCSymbolDB::GetSymbolFromAddr(u32 addr) const
 {
-  // If m_functions is changing, there should be a PPCSymbolsChanged signal afterward. The signal
-  // will re-update persistent symbol displays by calling this function. Only one-off calls to this
-  // function, such as printing the symbol to console, should be affected by leaving early.
-  std::unique_lock<std::mutex> lock(m_write_lock, std::try_to_lock);
-  if (!lock.owns_lock() || m_functions.empty())
+  std::lock_guard lock(m_mutex);
+  if (m_functions.empty())
     return nullptr;
 
   auto it = m_functions.lower_bound(addr);
@@ -162,10 +183,10 @@ Common::Symbol* PPCSymbolDB::GetSymbolFromAddr(u32 addr)
   return nullptr;
 }
 
-Common::Note* PPCSymbolDB::GetNoteFromAddr(u32 addr)
+const Common::Note* PPCSymbolDB::GetNoteFromAddr(u32 addr) const
 {
-  std::unique_lock<std::mutex> lock(m_write_lock, std::try_to_lock);
-  if (!lock.owns_lock() || m_notes.empty())
+  std::lock_guard lock(m_mutex);
+  if (m_notes.empty())
     return nullptr;
 
   auto itn = m_notes.lower_bound(addr);
@@ -195,15 +216,17 @@ Common::Note* PPCSymbolDB::GetNoteFromAddr(u32 addr)
 
 void PPCSymbolDB::DeleteFunction(u32 start_address)
 {
+  std::lock_guard lock(m_mutex);
   m_functions.erase(start_address);
 }
 
 void PPCSymbolDB::DeleteNote(u32 start_address)
 {
+  std::lock_guard lock(m_mutex);
   m_notes.erase(start_address);
 }
 
-std::string_view PPCSymbolDB::GetDescription(u32 addr)
+std::string PPCSymbolDB::GetDescription(u32 addr) const
 {
   if (const Common::Symbol* const symbol = GetSymbolFromAddr(addr))
     return symbol->name;
@@ -212,9 +235,7 @@ std::string_view PPCSymbolDB::GetDescription(u32 addr)
 
 void PPCSymbolDB::FillInCallers()
 {
-  std::unique_lock<std::mutex> lock(m_write_lock, std::try_to_lock);
-  if (!lock.owns_lock())
-    return;
+  std::lock_guard lock(m_mutex);
 
   for (auto& p : m_functions)
   {
@@ -247,6 +268,8 @@ void PPCSymbolDB::FillInCallers()
 
 void PPCSymbolDB::PrintCalls(u32 funcAddr) const
 {
+  std::lock_guard lock(m_mutex);
+
   const auto iter = m_functions.find(funcAddr);
   if (iter == m_functions.end())
   {
@@ -268,6 +291,8 @@ void PPCSymbolDB::PrintCalls(u32 funcAddr) const
 
 void PPCSymbolDB::PrintCallers(u32 funcAddr) const
 {
+  std::lock_guard lock(m_mutex);
+
   const auto iter = m_functions.find(funcAddr);
   if (iter == m_functions.end())
     return;
@@ -286,6 +311,8 @@ void PPCSymbolDB::PrintCallers(u32 funcAddr) const
 
 void PPCSymbolDB::LogFunctionCall(u32 addr)
 {
+  std::lock_guard lock(m_mutex);
+
   auto iter = m_functions.find(addr);
   if (iter == m_functions.end())
     return;
@@ -314,25 +341,22 @@ bool PPCSymbolDB::FindMapFile(std::string* existing_map_file, std::string* writa
   return false;
 }
 
+// Returns true if m_functions was changed.
 bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 {
-  // Loads from emuthread and can crash with main thread accessing the map. Any other loads will be
-  // done on the main thread and should be safe. Returns true if m_functions was changed.
-  std::lock_guard lock(m_write_lock);
-
   std::string existing_map_file;
   if (!PPCSymbolDB::FindMapFile(&existing_map_file, nullptr))
     return Clear();
 
+  {
+    std::lock_guard lock(m_mutex);
     // If the map is already loaded (such as restarting the same game), skip reloading.
     if (!IsEmpty() && existing_map_file == m_map_name)
       return false;
+  }
 
-  // Load map into cleared m_functions.
-  bool changed = Clear();
-
-  if (!LoadMap(guard, existing_map_file))
-    return changed;
+  if (!LoadMap(guard, std::move(existing_map_file)))
+    return Clear();
 
   return true;
 }
@@ -344,14 +368,11 @@ bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 // function names and addresses that have a BLR before the start and at the end, but ignore any that
 // don't, and then tell you how many were good and how many it ignored. That way you either find out
 // it is all good and use it, find out it is partly good and use the good part, or find out that
-// only
-// a handful of functions lined up by coincidence and then you can clear the symbols. In the future
-// I
-// want to make it smarter, so it checks that there are no BLRs in the middle of the function
-// (by checking the code length), and also make it cope with added functions in the middle or work
-// based on the order of the functions and their approximate length. Currently that process has to
-// be
-// done manually and is very tedious.
+// only a handful of functions lined up by coincidence and then you can clear the symbols. In the
+// future I want to make it smarter, so it checks that there are no BLRs in the middle of the
+// function (by checking the code length), and also make it cope with added functions in the middle
+// or work based on the order of the functions and their approximate length. Currently that process
+// has to be done manually and is very tedious.
 // The use case for separate handling of map files that aren't bad is that you usually want to also
 // load names that aren't functions(if included in the map file) without them being rejected as
 // invalid.
@@ -362,12 +383,16 @@ bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 // This one can load both leftover map files on game discs (like Zelda), and mapfiles
 // produced by SaveSymbolMap below.
 // bad=true means carefully load map files that might not be from exactly the right version
-bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string& filename, bool bad)
+bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, std::string filename, bool bad)
 {
   File::IOFile f(filename, "r");
   if (!f)
     return false;
 
+  XFuncMap new_functions;
+  XNoteMap new_notes;
+  XFuncPtrMap checksum_to_function;
+
   // Two columns are used by Super Smash Bros. Brawl Korean map file
   // Three columns are commonly used
   // Four columns are used in American Mensa Academy map files and perhaps other games
@@ -579,9 +604,14 @@ bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string&
         ++good_count;
 
         if (section_name == ".note")
-          AddKnownNote(vaddress, size, name);
+        {
+          AddKnownNote(vaddress, size, name, &new_notes);
+        }
         else
-          AddKnownSymbol(guard, vaddress, size, name_string, object_filename_string, type);
+        {
+          AddKnownSymbol(guard, vaddress, size, name_string, object_filename_string, type,
+                         &new_functions, &checksum_to_function);
+        }
       }
       else
       {
@@ -590,10 +620,15 @@ bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string&
     }
   }
 
-  m_map_name = filename;
+  Index(&new_functions);
+  DetermineNoteLayers(&new_notes);
+
+  std::lock_guard lock(m_mutex);
+  std::swap(m_functions, new_functions);
+  std::swap(m_notes, new_notes);
+  std::swap(m_checksum_to_function, checksum_to_function);
+  std::swap(m_map_name, filename);
 
-  Index();
-  DetermineNoteLayers();
   NOTICE_LOG_FMT(SYMBOLS, "{} symbols loaded, {} symbols ignored.", good_count, bad_count);
   return true;
 }
@@ -605,6 +640,8 @@ bool PPCSymbolDB::SaveSymbolMap(const std::string& filename) const
   if (!file)
     return false;
 
+  std::lock_guard lock(m_mutex);
+
   // Write .text section
   auto function_symbols =
       m_functions |
@@ -655,7 +692,7 @@ bool PPCSymbolDB::SaveSymbolMap(const std::string& filename) const
   return true;
 }
 
-// Save code map (won't work if Core is running)
+// Save code map
 //
 // Notes:
 //  - Dolphin doesn't load back code maps
@@ -670,6 +707,8 @@ bool PPCSymbolDB::SaveCodeMap(const Core::CPUThreadGuard& guard, const std::stri
   // Write ".text" at the top
   f.WriteString(".text\n");
 
+  std::lock_guard lock(m_mutex);
+
   const auto& ppc_debug_interface = guard.GetSystem().GetPowerPC().GetDebugInterface();
 
   u32 next_address = 0;

Source/Core/Core/PowerPC/PPCSymbolDB.h

@@ -3,7 +3,6 @@
 
 #pragma once
 
-#include <mutex>
 #include <string>
 #include <string_view>
 
@@ -22,25 +21,25 @@ public:
   PPCSymbolDB();
   ~PPCSymbolDB() override;
 
-  Common::Symbol* AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr) override;
+  const Common::Symbol* AddFunction(const Core::CPUThreadGuard& guard, u32 start_addr) override;
   void AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAddr, u32 size,
                       const std::string& name, const std::string& object_name,
                       Common::Symbol::Type type = Common::Symbol::Type::Function);
   void AddKnownNote(u32 start_addr, u32 size, const std::string& name);
 
-  Common::Symbol* GetSymbolFromAddr(u32 addr) override;
+  const Common::Symbol* GetSymbolFromAddr(u32 addr) const override;
   bool NoteExists() const { return !m_notes.empty(); }
-  Common::Note* GetNoteFromAddr(u32 addr);
+  const Common::Note* GetNoteFromAddr(u32 addr) const;
   void DetermineNoteLayers();
   void DeleteFunction(u32 start_address);
   void DeleteNote(u32 start_address);
 
-  std::string_view GetDescription(u32 addr);
+  std::string GetDescription(u32 addr) const;
 
   void FillInCallers();
 
   bool LoadMapOnBoot(const Core::CPUThreadGuard& guard);
-  bool LoadMap(const Core::CPUThreadGuard& guard, const std::string& filename, bool bad = false);
+  bool LoadMap(const Core::CPUThreadGuard& guard, std::string filename, bool bad = false);
   bool SaveSymbolMap(const std::string& filename) const;
   bool SaveCodeMap(const Core::CPUThreadGuard& guard, const std::string& filename) const;
 
@@ -51,5 +50,11 @@ public:
   static bool FindMapFile(std::string* existing_map_file, std::string* writable_map_file);
 
 private:
-  std::mutex m_write_lock;
+  static void AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAddr, u32 size,
+                             const std::string& name, const std::string& object_name,
+                             Common::Symbol::Type type, XFuncMap* functions,
+                             XFuncPtrMap* checksum_to_function);
+  static void AddKnownNote(u32 start_addr, u32 size, const std::string& name, XNoteMap* notes);
+
+  static void DetermineNoteLayers(XNoteMap* notes);
 };

Source/Core/Core/PowerPC/SignatureDB/MEGASignatureDB.cpp

@@ -160,20 +160,18 @@ bool MEGASignatureDB::Save(const std::string& file_path) const
 
 void MEGASignatureDB::Apply(const Core::CPUThreadGuard& guard, PPCSymbolDB* symbol_db) const
 {
-  for (auto& it : symbol_db->AccessSymbols())
-  {
-    auto& symbol = it.second;
+  symbol_db->ForEachSymbol([&](const Common::Symbol& symbol) {
     for (const auto& sig : m_signatures)
     {
       if (Compare(guard, symbol.address, symbol.size, sig))
       {
-        symbol.name = sig.name;
+        symbol_db->RenameSymbol(symbol, sig.name);
         INFO_LOG_FMT(SYMBOLS, "Found {} at {:08x} (size: {:08x})!", sig.name, symbol.address,
                      symbol.size);
         break;
       }
     }
-  }
+  });
   symbol_db->Index();
 }
 

Source/Core/Core/PowerPC/SignatureDB/SignatureDB.cpp

@@ -128,7 +128,7 @@ void HashSignatureDB::Apply(const Core::CPUThreadGuard& guard, PPCSymbolDB* symb
     for (const auto& function : symbol_db->GetSymbolsFromHash(entry.first))
     {
       // Found the function. Let's rename it according to the symbol file.
-      function->Rename(entry.second.name);
+      symbol_db->RenameSymbol(*function, entry.second.name);
       if (entry.second.size == static_cast<unsigned int>(function->size))
       {
         INFO_LOG_FMT(SYMBOLS, "Found {} at {:08x} (size: {:08x})!", entry.second.name,
@@ -146,18 +146,17 @@ void HashSignatureDB::Apply(const Core::CPUThreadGuard& guard, PPCSymbolDB* symb
 
 void HashSignatureDB::Populate(const PPCSymbolDB* symbol_db, const std::string& filter)
 {
-  for (const auto& symbol : symbol_db->Symbols())
-  {
-    if ((filter.empty() && (!symbol.second.name.empty()) &&
-         symbol.second.name.substr(0, 3) != "zz_" && symbol.second.name.substr(0, 1) != ".") ||
-        ((!filter.empty()) && symbol.second.name.substr(0, filter.size()) == filter))
+  symbol_db->ForEachSymbol([&](const Common::Symbol& symbol) {
+    if ((filter.empty() && (!symbol.name.empty()) && symbol.name.substr(0, 3) != "zz_" &&
+         symbol.name.substr(0, 1) != ".") ||
+        ((!filter.empty()) && symbol.name.substr(0, filter.size()) == filter))
     {
       DBFunc temp_dbfunc;
-      temp_dbfunc.name = symbol.second.name;
-      temp_dbfunc.size = symbol.second.size;
-      m_database[symbol.second.hash] = temp_dbfunc;
-    }
+      temp_dbfunc.name = symbol.name;
+      temp_dbfunc.size = symbol.size;
+      m_database[symbol.hash] = temp_dbfunc;
     }
+  });
 }
 
 u32 HashSignatureDB::ComputeCodeChecksum(const Core::CPUThreadGuard& guard, u32 offsetStart,

Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp

@@ -335,7 +335,7 @@ void CodeViewWidget::Update(const Core::CPUThreadGuard* guard)
 
     std::string ins = (split == std::string::npos ? disas : disas.substr(0, split));
     std::string param = (split == std::string::npos ? "" : disas.substr(split + 1));
-    const std::string_view desc = debug_interface.GetDescription(addr);
+    const std::string desc = debug_interface.GetDescription(addr);
 
     const Common::Note* note = m_ppc_symbol_db.GetNoteFromAddr(addr);
     std::string note_string;
@@ -950,7 +950,7 @@ void CodeViewWidget::OnFollowBranch()
 void CodeViewWidget::OnEditSymbol()
 {
   const u32 addr = GetContextAddress();
-  Common::Symbol* const symbol = m_ppc_symbol_db.GetSymbolFromAddr(addr);
+  const Common::Symbol* const symbol = m_ppc_symbol_db.GetSymbolFromAddr(addr);
 
   if (symbol == nullptr)
   {
@@ -974,12 +974,14 @@ void CodeViewWidget::OnEditSymbol()
   }
 
   if (symbol->name != name)
-    symbol->Rename(name);
+    m_ppc_symbol_db.RenameSymbol(*symbol, name);
 
   if (symbol->size != size)
   {
     Core::CPUThreadGuard guard(m_system);
-    PPCAnalyst::ReanalyzeFunction(guard, symbol->address, *symbol, size);
+    Common::Symbol new_symbol = *symbol;
+    PPCAnalyst::ReanalyzeFunction(guard, symbol->address, new_symbol, size);
+    m_ppc_symbol_db.AddCompleteSymbol(new_symbol);
   }
 
   emit Host::GetInstance()->PPCSymbolsChanged();
@@ -988,7 +990,7 @@ void CodeViewWidget::OnEditSymbol()
 void CodeViewWidget::OnDeleteSymbol()
 {
   const u32 addr = GetContextAddress();
-  Common::Symbol* const symbol = m_ppc_symbol_db.GetSymbolFromAddr(addr);
+  const Common::Symbol* const symbol = m_ppc_symbol_db.GetSymbolFromAddr(addr);
 
   if (symbol == nullptr)
     return;
@@ -1039,7 +1041,7 @@ void CodeViewWidget::OnSelectionChanged()
 void CodeViewWidget::OnEditNote()
 {
   const u32 context_address = GetContextAddress();
-  Common::Note* const note = m_ppc_symbol_db.GetNoteFromAddr(context_address);
+  const Common::Note* const note = m_ppc_symbol_db.GetNoteFromAddr(context_address);
 
   if (note == nullptr)
     return;
@@ -1071,7 +1073,7 @@ void CodeViewWidget::OnEditNote()
 void CodeViewWidget::OnDeleteNote()
 {
   const u32 context_address = GetContextAddress();
-  Common::Note* const note = m_ppc_symbol_db.GetNoteFromAddr(context_address);
+  const Common::Note* const note = m_ppc_symbol_db.GetNoteFromAddr(context_address);
 
   if (note == nullptr)
     return;

Source/Core/DolphinQt/Debugger/CodeWidget.cpp

@@ -423,14 +423,13 @@ void CodeWidget::UpdateSymbols()
                                 m_symbols_list->selectedItems()[0]->text();
   m_symbols_list->clear();
 
-  for (const auto& symbol : m_ppc_symbol_db.Symbols())
-  {
-    QString name = QString::fromStdString(symbol.second.name);
+  m_ppc_symbol_db.ForEachSymbol([&](const Common::Symbol& symbol) {
+    QString name = QString::fromStdString(symbol.name);
 
     // If the symbol has an object name, add it to the entry name.
-    if (!symbol.second.object_name.empty())
+    if (!symbol.object_name.empty())
     {
-      name += QString::fromStdString(fmt::format(" ({})", symbol.second.object_name));
+      name += QString::fromStdString(fmt::format(" ({})", symbol.object_name));
     }
 
     auto* item = new QListWidgetItem(name);
@@ -438,14 +437,14 @@ void CodeWidget::UpdateSymbols()
       item->setSelected(true);
 
     // Disable non-function symbols as you can't do anything with them.
-    if (symbol.second.type != Common::Symbol::Type::Function)
+    if (symbol.type != Common::Symbol::Type::Function)
       item->setFlags(Qt::NoItemFlags);
 
-    item->setData(Qt::UserRole, symbol.second.address);
+    item->setData(Qt::UserRole, symbol.address);
 
     if (name.contains(m_symbol_filter, Qt::CaseInsensitive))
       m_symbols_list->addItem(item);
-  }
+  });
 
   m_symbols_list->sortItems();
 }
@@ -457,19 +456,18 @@ void CodeWidget::UpdateNotes()
                                 m_note_list->selectedItems()[0]->text();
   m_note_list->clear();
 
-  for (const auto& note : m_ppc_symbol_db.Notes())
-  {
-    const QString name = QString::fromStdString(note.second.name);
+  m_ppc_symbol_db.ForEachNote([&](const Common::Note& note) {
+    const QString name = QString::fromStdString(note.name);
 
     auto* item = new QListWidgetItem(name);
     if (name == selection)
       item->setSelected(true);
 
-    item->setData(Qt::UserRole, note.second.address);
+    item->setData(Qt::UserRole, note.address);
 
     if (name.toUpper().indexOf(m_symbol_filter.toUpper()) != -1)
       m_note_list->addItem(item);
-  }
+  });
 
   m_note_list->sortItems();
 }