[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like the direction this is going. I have one small comment about the code organization inline, but the bigger question I have in mind is: After these changes, is there any use for `Host::GetProcessBaseAddress` left? If so, what is it? I'd like to understand it and remove it.



================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:868
 
+DynamicLoader *ProcessWindows::GetDynamicLoader() {
+  if (m_dyld_ap.get() == NULL)
----------------
You could have this function return `DynamicLoaderWindowsDYLD *` (thanks to the covariant return type thingy), and avoid having to cast the result in a bunch of places.


================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:1039-1044
   ModuleSP module = GetTarget().GetSharedModule(module_spec, &error);
-  bool load_addr_changed = false;
-  module->SetLoadAddress(GetTarget(), module_addr, false, load_addr_changed);
-
-  ModuleList loaded_modules;
-  loaded_modules.Append(module);
-  GetTarget().ModulesDidLoad(loaded_modules);
+  if (error.Success()) {
+    auto dyld = static_cast<DynamicLoaderWindowsDYLD *>(GetDynamicLoader());
+    assert(dyld);
+    dyld->OnLoadModule(module, module_addr);
+  }
----------------
On other platforms, it is the responsibility of the DYLD plugin to add the module to the target list (i.e. call Target::GetSharedModule or something equivalent). I think it would be more consistent to move this code there too. At that point you may just delete `ProcessWindows::OnLoadDll` and have the debugger thread call `process_windows->GetDynamicLoader()->OnLoadDLL()` directly. The same goes for module UnloadDLL.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56237/new/

https://reviews.llvm.org/D56237