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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56237#1383232 <https://reviews.llvm.org/D56237#1383232>, @Hui wrote:

> In D56237#1383070 <https://reviews.llvm.org/D56237#1383070>, @labath wrote:
>
> > 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.
>
>
> I am thinking getting load address and setting load address by dyld could be done in separate reviews?  For the latter, it might lead to some architecture changes in processwindows plugin.
>
> For this review, getting load address of a host process can just leverage the OS API if no processwindows plugin's help is wanted.


Yes, two reviews would be great. However, the way I see this, the second patch would basically remove the need for the special Host function you're trying to introduce here. So I'd suggest to do the second part first, and then this should be  very simple addon on top of that.

> Servers other than lldb-server.exe can easily implement the qFileLoadAddress packet (using their own loader) as a reply to m_process->GetFileLoadAddress().

I am aware of that. However, my point is that you shouldn't need *any* fallback here. If ProcessWindows::GetFileLoadAddress does the right thing, then all the dyld plugin would not need to call any host functions *at all* (which is just a fundamentally wrong thing to do).


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

https://reviews.llvm.org/D56237