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

Jan Kratochvil via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jul 1 02:32:17 PDT 2019

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.




More information about the lldb-commits mailing list