Merge pull request #13750 from TryTwo/PR_Symbol_Loading

PPCSymbols: Restructure loading on boot and add a mutex to prevent crashes.

Source/Core/Common/SymbolDB.cpp

@@ -44,15 +44,20 @@ void SymbolDB::List()
 
 bool SymbolDB::IsEmpty() const
 {
-  return m_functions.empty();
+  return m_functions.empty() && m_notes.empty();
 }
 
-void SymbolDB::Clear(const char* prefix)
+bool SymbolDB::Clear(const char* prefix)
 {
   // TODO: honor prefix
+  m_map_name.clear();
+  if (IsEmpty())
+    return false;
+
   m_functions.clear();
   m_notes.clear();
   m_checksum_to_function.clear();
+  return true;
 }
 
 void SymbolDB::Index()

Source/Core/Common/SymbolDB.h

@@ -100,7 +100,7 @@ public:
   const XNoteMap& Notes() const { return m_notes; }
   XFuncMap& AccessSymbols() { return m_functions; }
   bool IsEmpty() const;
-  void Clear(const char* prefix = "");
+  bool Clear(const char* prefix = "");
   void List();
   void Index();
 
@@ -108,5 +108,6 @@ protected:
   XFuncMap m_functions;
   XNoteMap m_notes;
   XFuncPtrMap m_checksum_to_function;
+  std::string m_map_name;
 };
 }  // namespace Common

Source/Core/Core/Boot/Boot.cpp

@@ -354,39 +354,6 @@ bool CBoot::DVDReadDiscID(Core::System& system, const DiscIO::VolumeDisc& disc,
   return true;
 }
 
-// Get map file paths for the active title.
-bool CBoot::FindMapFile(std::string* existing_map_file, std::string* writable_map_file)
-{
-  const std::string& game_id = SConfig::GetInstance().m_debugger_game_id;
-  std::string path = File::GetUserPath(D_MAPS_IDX) + game_id + ".map";
-
-  if (writable_map_file)
-    *writable_map_file = path;
-
-  if (File::Exists(path))
-  {
-    if (existing_map_file)
-      *existing_map_file = std::move(path);
-
-    return true;
-  }
-
-  return false;
-}
-
-bool CBoot::LoadMapFromFilename(const Core::CPUThreadGuard& guard, PPCSymbolDB& ppc_symbol_db)
-{
-  std::string strMapFilename;
-  bool found = FindMapFile(&strMapFilename, nullptr);
-  if (found && ppc_symbol_db.LoadMap(guard, strMapFilename))
-  {
-    Host_PPCSymbolsChanged();
-    return true;
-  }
-
-  return false;
-}
-
 // If ipl.bin is not found, this function does *some* of what BS1 does:
 // loading IPL(BS2) and jumping to it.
 // It does not initialize the hardware or anything else like BS1 does.
@@ -524,12 +491,6 @@ bool CBoot::BootUp(Core::System& system, const Core::CPUThreadGuard& guard,
 {
   SConfig& config = SConfig::GetInstance();
 
-  if (auto& ppc_symbol_db = system.GetPPCSymbolDB(); !ppc_symbol_db.IsEmpty())
-  {
-    ppc_symbol_db.Clear();
-    Host_PPCSymbolsChanged();
-  }
-
   // PAL Wii uses NTSC framerate and linecount in 60Hz modes
   system.GetVideoInterface().Preset(DiscIO::IsNTSC(config.m_region) ||
                                     (system.IsWii() && Config::Get(Config::SYSCONF_PAL60)));
@@ -611,11 +572,18 @@ bool CBoot::BootUp(Core::System& system, const Core::CPUThreadGuard& guard,
 
       const std::string filename = PathToFileName(executable.path);
 
-      if (executable.reader->LoadSymbols(guard, system.GetPPCSymbolDB(), filename))
+      auto& ppc_symbol_db = system.GetPPCSymbolDB();
+      bool symbols_changed = ppc_symbol_db.Clear();
+
+      if (executable.reader->LoadSymbols(guard, ppc_symbol_db, filename))
       {
-        Host_PPCSymbolsChanged();
+        symbols_changed = true;
         HLE::PatchFunctions(system);
       }
+
+      if (symbols_changed)
+        Host_PPCSymbolsChanged();
+
       return true;
     }
 

Source/Core/Core/Boot/Boot.h

@@ -159,19 +159,6 @@ public:
   static bool BootUp(Core::System& system, const Core::CPUThreadGuard& guard,
                      std::unique_ptr<BootParameters> boot);
 
-  // Tries to find a map file for the current game by looking first in the
-  // local user directory, then in the shared user directory.
-  //
-  // If existing_map_file is not nullptr and a map file exists, it is set to the
-  // path to the existing map file.
-  //
-  // If writable_map_file is not nullptr, it is set to the path to where a map
-  // file should be saved.
-  //
-  // Returns true if a map file exists, false if none could be found.
-  static bool FindMapFile(std::string* existing_map_file, std::string* writable_map_file);
-  static bool LoadMapFromFilename(const Core::CPUThreadGuard& guard, PPCSymbolDB& ppc_symbol_db);
-
 private:
   static bool DVDRead(Core::System& system, const DiscIO::VolumeDisc& disc, u64 dvd_offset,
                       u32 output_address, u32 length, const DiscIO::Partition& partition);

Source/Core/Core/ConfigManager.cpp

@@ -271,12 +271,9 @@ void SConfig::OnTitleDirectlyBooted(const Core::CPUThreadGuard& guard)
     return;
 
   auto& ppc_symbol_db = system.GetPPCSymbolDB();
-  if (!ppc_symbol_db.IsEmpty())
+
-  {
+  if (ppc_symbol_db.LoadMapOnBoot(guard))
-    ppc_symbol_db.Clear();
     Host_PPCSymbolsChanged();
-  }
-  CBoot::LoadMapFromFilename(guard, ppc_symbol_db);
   HLE::Reload(system);
 
   PatchEngine::Reload(system);

Source/Core/Core/IOS/MIOS.cpp

@@ -71,18 +71,18 @@ bool Load(Core::System& system)
   auto& ppc_symbol_db = power_pc.GetSymbolDB();
 
   // Load symbols for the IPL if they exist.
-  if (!ppc_symbol_db.IsEmpty())
+  bool symbols_changed = ppc_symbol_db.Clear();
-  {
+
-    ppc_symbol_db.Clear();
-    Host_PPCSymbolsChanged();
-  }
   if (ppc_symbol_db.LoadMap(guard, File::GetUserPath(D_MAPS_IDX) + "mios-ipl.map"))
   {
     ::HLE::Clear();
     ::HLE::PatchFunctions(system);
-    Host_PPCSymbolsChanged();
+    symbols_changed = true;
   }
 
+  if (symbols_changed)
+    Host_PPCSymbolsChanged();
+
   const PowerPC::CoreMode core_mode = power_pc.GetMode();
   power_pc.SetMode(PowerPC::CoreMode::Interpreter);
 

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

@@ -6,6 +6,7 @@
 #include <algorithm>
 #include <cstring>
 #include <map>
+#include <mutex>
 #include <ranges>
 #include <sstream>
 #include <string>
@@ -15,10 +16,12 @@
 #include <fmt/format.h>
 
 #include "Common/CommonTypes.h"
+#include "Common/FileUtil.h"
 #include "Common/IOFile.h"
 #include "Common/Logging/Log.h"
 #include "Common/StringUtil.h"
 #include "Common/Unreachable.h"
+#include "Core/ConfigManager.h"
 #include "Core/Core.h"
 #include "Core/Debugger/DebugInterface.h"
 #include "Core/PowerPC/MMU.h"
@@ -133,6 +136,13 @@ void PPCSymbolDB::DetermineNoteLayers()
 
 Common::Symbol* PPCSymbolDB::GetSymbolFromAddr(u32 addr)
 {
+  // 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())
+    return nullptr;
+
   auto it = m_functions.lower_bound(addr);
 
   if (it != m_functions.end())
@@ -154,7 +164,8 @@ Common::Symbol* PPCSymbolDB::GetSymbolFromAddr(u32 addr)
 
 Common::Note* PPCSymbolDB::GetNoteFromAddr(u32 addr)
 {
-  if (m_notes.empty())
+  std::unique_lock<std::mutex> lock(m_write_lock, std::try_to_lock);
+  if (!lock.owns_lock() || m_notes.empty())
     return nullptr;
 
   auto itn = m_notes.lower_bound(addr);
@@ -201,6 +212,10 @@ 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;
+
   for (auto& p : m_functions)
   {
     p.second.callers.clear();
@@ -279,6 +294,49 @@ void PPCSymbolDB::LogFunctionCall(u32 addr)
   f.num_calls++;
 }
 
+// Get map file paths for the active title.
+bool PPCSymbolDB::FindMapFile(std::string* existing_map_file, std::string* writable_map_file)
+{
+  const std::string& game_id = SConfig::GetInstance().m_debugger_game_id;
+  std::string path = File::GetUserPath(D_MAPS_IDX) + game_id + ".map";
+
+  if (writable_map_file)
+    *writable_map_file = path;
+
+  if (File::Exists(path))
+  {
+    if (existing_map_file)
+      *existing_map_file = std::move(path);
+
+    return true;
+  }
+
+  return false;
+}
+
+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();
+
+  // 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;
+
+  return true;
+}
+
 // The use case for handling bad map files is when you have a game with a map file on the disc,
 // but you can't tell whether that map file is for the particular release version used in that game,
 // or when you know that the map file is not for that build, but perhaps half the functions in the
@@ -532,6 +590,8 @@ bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string&
     }
   }
 
+  m_map_name = filename;
+
   Index();
   DetermineNoteLayers();
   NOTICE_LOG_FMT(SYMBOLS, "{} symbols loaded, {} symbols ignored.", good_count, bad_count);

Source/Core/Core/PowerPC/PPCSymbolDB.h

@@ -3,6 +3,7 @@
 
 #pragma once
 
+#include <mutex>
 #include <string>
 #include <string_view>
 
@@ -37,6 +38,8 @@ public:
   std::string_view GetDescription(u32 addr);
 
   void FillInCallers();
+
+  bool LoadMapOnBoot(const Core::CPUThreadGuard& guard);
   bool LoadMap(const Core::CPUThreadGuard& guard, const std::string& filename, bool bad = false);
   bool SaveSymbolMap(const std::string& filename) const;
   bool SaveCodeMap(const Core::CPUThreadGuard& guard, const std::string& filename) const;
@@ -44,4 +47,9 @@ public:
   void PrintCalls(u32 funcAddr) const;
   void PrintCallers(u32 funcAddr) const;
   void LogFunctionCall(u32 addr);
+
+  static bool FindMapFile(std::string* existing_map_file, std::string* writable_map_file);
+
+private:
+  std::mutex m_write_lock;
 };

Source/Core/DolphinQt/MenuBar.cpp

@@ -26,7 +26,6 @@
 #include "Common/StringUtil.h"
 
 #include "Core/AchievementManager.h"
-#include "Core/Boot/Boot.h"
 #include "Core/CommonTitles.h"
 #include "Core/Config/AchievementSettings.h"
 #include "Core/Config/MainSettings.h"
@@ -1717,7 +1716,7 @@ void MenuBar::LoadSymbolMap()
   auto& ppc_symbol_db = system.GetPPCSymbolDB();
 
   std::string existing_map_file, writable_map_file;
-  bool map_exists = CBoot::FindMapFile(&existing_map_file, &writable_map_file);
+  bool map_exists = PPCSymbolDB::FindMapFile(&existing_map_file, &writable_map_file);
 
   if (!map_exists)
   {
@@ -1755,7 +1754,7 @@ void MenuBar::LoadSymbolMap()
 void MenuBar::SaveSymbolMap()
 {
   std::string existing_map_file, writable_map_file;
-  CBoot::FindMapFile(&existing_map_file, &writable_map_file);
+  PPCSymbolDB::FindMapFile(&existing_map_file, &writable_map_file);
 
   TrySaveSymbolMap(QString::fromStdString(writable_map_file));
 }
@@ -1811,7 +1810,7 @@ void MenuBar::SaveSymbolMapAs()
 void MenuBar::SaveCode()
 {
   std::string existing_map_file, writable_map_file;
-  CBoot::FindMapFile(&existing_map_file, &writable_map_file);
+  PPCSymbolDB::FindMapFile(&existing_map_file, &writable_map_file);
 
   const std::string path =
       writable_map_file.substr(0, writable_map_file.find_last_of('.')) + "_code.map";