[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D62503#1520435 <https://reviews.llvm.org/D62503#1520435>, @aadsm wrote:

> > However, now that I think about it, that is nonsense, because there is no way for us to say to the user that "we failed to read some initial bytes, but this is the memory contents after that". So just using process_vm_readv, and finishing up with ptrace sounds fine to me.
>
> The reason why I thought reading page by page still made sense in the ReadMemory is to cover for the situation where we cross 3 pages and the page in the middle is not readable. However, that might not be realistic to happen?


Yeah, you're right. This can even happen with just two pages if the first page is not readable-by-process_vm_readv but it *is* readable-by-ptrace. I think that's pretty unlikely to happen, but it is possible in theory. I'll leave it up to you to figure out whether it's worth implementing that.

In D62503#1520435 <https://reviews.llvm.org/D62503#1520435>, @aadsm wrote:

> > That would also allow you to test this by sending `m` packets which are deliberately chosen to cross readable/unreadable page boundaries in interesting ways...
>
> BTW: LLDB will usually always send down aligned reads due to it populating its memory cache.


Yep, this kind of test would have to be done at the lldb-server level. Which is reasonable, as we're actually testing lldb-server behavior.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1502-1504
+Status NativeProcessLinux::ReadCStringFromMemory(lldb::addr_t addr,
+                                                 char *buffer, size_t max_size,
+                                                 size_t &total_bytes_read) {
----------------
clayborg wrote:
> Seems like this function could be added to the NativeProcess base class? We would need to add "virtual size_t NativeProcess::GetCacheLineSize() = 0;" to NativeProcess too in that case.
There's `llvm::sys::Process::getPageSizeEstimate()`, which can be used directly here. Since lldb-server is running on the same host as the debugged process, we don't even have to pipe things through the NativeProcess class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62503