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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D56237#1373234 <https://reviews.llvm.org/D56237#1373234>, @labath wrote:

> I think there are some architectural issues here in how ProcessWindows works. Normally (i.e., on all non-windows process classes), it is the job of the dynamic loader plugin to create modules and set their load addresses. However, ProcessWindows does things differently, and creates the modules by itself (ProcessWindows::OnLoadDLL).
>  I think the behavior of ProcessWindows is fine (it flows naturally from how modules are handled on windows), but if it's going to handle module loading by itself, then I think it should do the job completely, and not only half-way. So, if we want ProcessWindows to do the module loading, then it should return a null value for `GetDynamicLoader()` (or a reasonably stubbed out implementation) instead of relying on a generic dynamic loader plugin to finish the job.
>
> Or, if we want to have the DynamicLoader plugin, then ProcessWindows should stop meddling with the module list in the OnLoadDLL function. One way to achieve that would be:
>
> - ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
> - dynamic loader loads the module and sets load address
> - dynamic loader does not have to call `ProcessWindows::GetFileLoadAddress` as it already got the load address from in the OnLoadDLL function This would make the flow very similar to how other platforms handle it (the only difference would be that the "OnLoadDLL" equivalent is called from a breakpoint callback function, instead of from a special debug event callback, but that's just due to a difference in how modules are reported).


I would like the see the dynamic loader do all of the work. We can easily add any new virtual functions to the DynamicLoader class to fit how windows does things.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56237