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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D63868#1563802 <https://reviews.llvm.org/D63868#1563802>, @labath wrote:

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


OK, I will try to reproduce it - you are right GDB documentation <https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#Packets> says for the `m` packet: "//The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory. //"

And LLDB is using `ReadMemory` in `NativeProcessProtocol::ReadMemoryWithoutTrap` from `GDBRemoteCommunicationServerLLGS::Handle_memory_read`.

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

IIUC you believe this patch should be more refactored? I do not have much overview of this code of LLDB (+Windows libraries in general). IIUC you propose:

- The current libraries loading code from `LoadedModuleInfoList` of `ProcessGDBRemote::LoadModules` moved to `lldb/source/Plugins/Platform/Windows/`
- later: `DynamicLoaderPOSIXDYLD::RefreshModules` and `DynamicLoaderPOSIXDYLD::LoadAllCurrentModules` somehow reusing that code (after it is moved back to some common library)? But that looks to me as too little code to make it worth it.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868