[Lldb-commits] [lldb] [lldb][windows] fix duplicate OnLoadModule events (PR #189376)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

<details>
<summary>Changes</summary>

On Windows, lldb relies on the `DebuggerThread::DebugLoop` to handle debuggin event. The `LOAD_DLL_DEBUG_INFO` can be fired multiple times for the same module at different addresses. Currently, the following can happen:
1. `LOAD_DLL_DEBUG_INFO` is fired for `foo.dll` at address `A`. It's inserted in the cache.
2. `LOAD_DLL_DEBUG_INFO` is fired for `foo.dll` at address `B`. It's inserted in the cache, overwritting the previous entry.
3. `UNLOAD_DLL_DEBUG_INFO` is fired for `foo.dll` at address `A`. It's not longer in the cache, `foo.dll` at address `B` is unloaded instead.

In this situation, `foo.dll` is no longer loaded: we unloaded the wrong module.

With the fix, lldb now looks up the module in the cache by its address. Given the previous scenario, lldb is now unloading only the first, temporary `foo.dll`.

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


2 Files Affected:

- (modified) lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp (+25-11) 
- (modified) lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h (+9-5) 


``````````diff
diff --git a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
index 44d671f843b74..393d4e859abba 100644
--- a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
@@ -64,7 +64,6 @@ DynamicLoader *DynamicLoaderWindowsDYLD::CreateInstance(Process *process,
 void DynamicLoaderWindowsDYLD::OnLoadModule(lldb::ModuleSP module_sp,
                                             const ModuleSpec module_spec,
                                             lldb::addr_t module_addr) {
-
   // Resolve the module unless we already have one.
   if (!module_sp) {
     Status error;
@@ -74,7 +73,7 @@ void DynamicLoaderWindowsDYLD::OnLoadModule(lldb::ModuleSP module_sp,
       return;
   }
 
-  m_loaded_modules[module_sp] = module_addr;
+  m_loaded_modules[module_addr] = lldb::ModuleWP(module_sp);
   UpdateLoadedSectionsCommon(module_sp, module_addr, false);
   ModuleList module_list;
   module_list.Append(module_sp);
@@ -82,13 +81,26 @@ void DynamicLoaderWindowsDYLD::OnLoadModule(lldb::ModuleSP module_sp,
 }
 
 void DynamicLoaderWindowsDYLD::OnUnloadModule(lldb::addr_t module_addr) {
-  Address resolved_addr;
-  if (!m_process->GetTarget().ResolveLoadAddress(module_addr, resolved_addr))
+  auto it = m_loaded_modules.find(module_addr);
+  if (it == m_loaded_modules.end())
+    return;
+
+  ModuleSP module_sp = it->second.lock();
+  m_loaded_modules.erase(it);
+
+  if (!module_sp)
     return;
 
-  ModuleSP module_sp = resolved_addr.GetModule();
-  if (module_sp) {
-    m_loaded_modules.erase(module_sp);
+  bool full_unload = true;
+  for (const auto &entry : m_loaded_modules) {
+    ModuleSP other_sp = entry.second.lock();
+    if (other_sp == module_sp) {
+      UpdateLoadedSectionsCommon(module_sp, entry.first, false);
+      full_unload = false;
+    }
+  }
+
+  if (full_unload) {
     UnloadSectionsCommon(module_sp);
     ModuleList module_list;
     module_list.Append(module_sp);
@@ -98,9 +110,11 @@ void DynamicLoaderWindowsDYLD::OnUnloadModule(lldb::addr_t module_addr) {
 
 lldb::addr_t DynamicLoaderWindowsDYLD::GetLoadAddress(ModuleSP executable) {
   // First, see if the load address is already cached.
-  auto it = m_loaded_modules.find(executable);
-  if (it != m_loaded_modules.end() && it->second != LLDB_INVALID_ADDRESS)
-    return it->second;
+  for (const auto &entry : m_loaded_modules) {
+    ModuleSP mod = entry.second.lock();
+    if (mod == executable && entry.first != LLDB_INVALID_ADDRESS)
+      return entry.first;
+  }
 
   lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
 
@@ -112,7 +126,7 @@ lldb::addr_t DynamicLoaderWindowsDYLD::GetLoadAddress(ModuleSP executable) {
       m_process->GetFileLoadAddress(file_spec, is_loaded, load_addr);
   // Servers other than lldb server could respond with a bogus address.
   if (status.Success() && is_loaded && load_addr != LLDB_INVALID_ADDRESS) {
-    m_loaded_modules[executable] = load_addr;
+    m_loaded_modules[load_addr] = lldb::ModuleWP(executable);
     return load_addr;
   }
 
diff --git a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h
index 8b1c3c3f467f4..67d2104b6ef1f 100644
--- a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h
+++ b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h
@@ -12,8 +12,6 @@
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/lldb-forward.h"
 
-#include <map>
-
 namespace lldb_private {
 
 class DynamicLoaderWindowsDYLD : public DynamicLoader {
@@ -44,9 +42,15 @@ class DynamicLoaderWindowsDYLD : public DynamicLoader {
 protected:
   lldb::addr_t GetLoadAddress(lldb::ModuleSP executable);
 
-private:
-  std::map<lldb::ModuleWP, lldb::addr_t, std::owner_less<lldb::ModuleWP>>
-      m_loaded_modules;
+  /// Maps load addresses to their corresponding modules.
+  ///
+  /// Weak pointers are used intentionally: on Windows, a Module holds a
+  /// memory-mapped view of the DLL file, and an open memory mapping locks
+  /// the file on disk. Holding a strong reference (ModuleSP) here would
+  /// prevent the mapping from being released even after the Target drops
+  /// its own reference, keeping the file locked and blocking recompilation
+  /// during an active debug session.
+  llvm::DenseMap<lldb::addr_t, lldb::ModuleWP> m_loaded_modules;
 };
 
 } // namespace lldb_private

``````````

</details>


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