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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
