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

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

Going to the Host straight from the DynamicLoader is the worst possible option, since then this code will not be correct for remote processes (it will just randomly pick up some data from the local process with the same pid, if you happen to have one). I'd like to hear what you think about the latest comments by Greg & myself.

In D56237#1373788 <https://reviews.llvm.org/D56237#1373788>, @clayborg wrote:

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


That should be fairly easy to achieve. The process class is the one that creates the dynamic loader (in `GetDynamicLoader()`), so it can easily call any special method on it that it needs from OnLoadDLL (i.e., we don't even need to extend the public DynamicLoader API).

However, note that this would not be the first process class that handles module loading on its own. ProcessMinidump already does that too. It could be changed to use a special dynamic loader too, but it's not clear to me whether that is worth the trouble, as the loading process for minidumps is very simple.


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

https://reviews.llvm.org/D56237