[Lldb-commits] [lldb] [lldb][Windows] Key lldb-server's loaded-module list by base address (PR #223445)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

<details>
<summary>Changes</summary>

`NativeProcessWindows` keys `m_loaded_modules` by `FileSpec`, so a second mapping of an already-loaded DLL overwrites the first one's base address, and `OnUnloadDll` then erases by address. The wrong mapping gets dropped.

This reproduces reliably in swiftlang where `swiftCore.dll` is loaded twice.

Key mappings by base address instead (the identity `UNLOAD_DLL` carries), so an unload resolves to the right file and only retires the reported entry when it is that file's last mapping.

---
Full diff: https://github.com/llvm/llvm-project/pull/223445.diff


2 Files Affected:

- (modified) lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp (+32-12) 
- (modified) lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h (+15) 


``````````diff
diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
index bb1feca5eb097..86bb5c283bfa7 100644
--- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
@@ -358,6 +358,34 @@ Status NativeProcessWindows::RemoveBreakpoint(lldb::addr_t addr,
   return RemoveSoftwareBreakpoint(addr);
 }
 
+void NativeProcessWindows::AddLoadedModule(const FileSpec &file_spec,
+                                           addr_t base_addr) {
+  m_module_addresses[base_addr] = file_spec;
+  m_loaded_modules.try_emplace(file_spec, base_addr);
+}
+
+FileSpec NativeProcessWindows::RemoveLoadedModule(addr_t base_addr) {
+  auto addr_it = m_module_addresses.find(base_addr);
+  if (addr_it == m_module_addresses.end())
+    return {};
+
+  FileSpec file_spec = addr_it->second;
+  m_module_addresses.erase(addr_it);
+
+  auto module_it = m_loaded_modules.find(file_spec);
+  if (module_it == m_loaded_modules.end() || module_it->second != base_addr)
+    return {};
+
+  for (const auto &[addr, spec] : m_module_addresses)
+    if (spec == file_spec) {
+      module_it->second = addr;
+      return {};
+    }
+
+  m_loaded_modules.erase(module_it);
+  return file_spec;
+}
+
 Status NativeProcessWindows::CacheLoadedModules() {
   Status error;
   if (!m_loaded_modules.empty())
@@ -376,7 +404,7 @@ Status NativeProcessWindows::CacheLoadedModules() {
 
         FileSpec file_spec(path);
         FileSystem::Instance().Resolve(file_spec);
-        m_loaded_modules[file_spec] = (addr_t)me.modBaseAddr;
+        AddLoadedModule(file_spec, (addr_t)me.modBaseAddr);
       } while (Module32Next(snapshot.get(), &me));
     }
 
@@ -492,7 +520,7 @@ void NativeProcessWindows::OnDebuggerConnected(lldb::addr_t image_base) {
     FileSpec exe = info.GetExecutableFile();
     if (exe) {
       FileSystem::Instance().Resolve(exe);
-      m_loaded_modules[exe] = image_base;
+      AddLoadedModule(exe, image_base);
     }
   }
 
@@ -742,7 +770,7 @@ DllEventAction NativeProcessWindows::OnLoadDll(const ModuleSpec &module_spec,
   FileSpec resolved = module_spec.GetFileSpec();
   if (resolved) {
     FileSystem::Instance().Resolve(resolved);
-    m_loaded_modules[resolved] = module_addr;
+    AddLoadedModule(resolved, module_addr);
   }
   m_pending_library_events = true;
 
@@ -779,15 +807,7 @@ DllEventAction NativeProcessWindows::OnUnloadDll(lldb::addr_t module_addr,
   Log *log = GetLog(WindowsLog::Process);
   llvm::sys::ScopedLock lock(m_mutex);
 
-  FileSpec unloaded_spec;
-  for (auto it = m_loaded_modules.begin(); it != m_loaded_modules.end();) {
-    if (it->second == module_addr) {
-      unloaded_spec = it->first;
-      it = m_loaded_modules.erase(it);
-    } else {
-      ++it;
-    }
-  }
+  FileSpec unloaded_spec = RemoveLoadedModule(module_addr);
   m_pending_library_events = true;
 
   if (!m_initial_stop_seen || !m_client_supports_libraries_read)
diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h
index 3074eea1bc932..16f61d5062256 100644
--- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h
+++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h
@@ -172,8 +172,23 @@ class NativeProcessWindows : public NativeProcessProtocol,
                                          const ExceptionRecord &record);
 
   Status CacheLoadedModules();
+
+  /// Record that \p file_spec is mapped at \p base_addr.
+  void AddLoadedModule(const lldb_private::FileSpec &file_spec,
+                       lldb::addr_t base_addr);
+
+  /// Drop the mapping at \p base_addr. \return the module's file spec if this
+  /// was the last mapping of that file, an empty FileSpec otherwise.
+  lldb_private::FileSpec RemoveLoadedModule(lldb::addr_t base_addr);
+
+  /// The loaded modules to report to the client, one entry per file. The same
+  /// file can be mapped more than once.
   std::map<lldb_private::FileSpec, lldb::addr_t> m_loaded_modules;
 
+  /// Every image mapping, keyed by base address. This is the identity an
+  /// UNLOAD_DLL_DEBUG_EVENT carries.
+  std::map<lldb::addr_t, lldb_private::FileSpec> m_module_addresses;
+
   /// Set whenever an OS DLL load/unload event has been seen since the last stop
   /// reply.
   bool m_pending_library_events = false;

``````````

</details>


https://github.com/llvm/llvm-project/pull/223445