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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

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


Thanks. To deterministically reproduce an unmapped page of memory, the best approach I can think of is to first mmap a larger block of memory, and then munmap a part of it.

> 
> 
>>   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/`

Yes, that's more or less it. Though I am open to being convinced otherwise...

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

Possibly. If the same code is useful for both windows and posix, then it might make sense to put it into the base DynamicLoader class, which means we wouldn't have to create new plugins or whatnot. Or we might actually want to create a DynamicLoaderSVR4, which only works with SVR4, and have these delegate to it. But that's a discussion for another day...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63868