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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56237