[Lldb-commits] [PATCH] D63868: Unify+fix remote XML libraries handling with the legacy one

Mon Jul 1 01:58:42 PDT 2019

labath added a comment.

@jankratochvil wrote:

> That is because it fixed failing ReadMemory (due to PAGE_SIZE size reading shared library name overlapping to a next page which is accidentally unmapped)

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

In D63868#1560990 <https://reviews.llvm.org/D63868#1560990>, @jankratochvil wrote:

> In D63868#1560872 <https://reviews.llvm.org/D63868#1560872>, @aadsm wrote:
>
> > I don't think we can stop loading/unloading the libraries in `ProcessGDBRemote::LoadModules`. The windows dynamic loader relies on this
>
>
> Then it could be Windows-specific.

I also think it would be good to move this stuff into the windows loader. Right now we have a lot of confusion about who is responsible for "loading" modules into a target. Some processes (I think mostly post-mortem plugins) do the loading themselves. Others create dynamic loader plugins to do that. And in the case of svr4, we have ProcessGDBRemote creating a DynamicLoader plugin, which then calls back into the process to do the actual loading. That seems too convoluted to me...

I think it would be better to do the loading inside the DynamicLoader plugin, and have `ProcessGDBRemote::LoadModules` be responsible only for sending the appropriate qXfer packet and parsing the response. The fact that loading the libraries from a `LoadedModuleInfoList` is largely similar for posix&windows can be handled (at a later date) by factoring the common code into a utility function, a common base class or something else...

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868