[Lldb-commits] [lldb] [lldb/linux] Make truncated reads work (PR #106532)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Aug 29 04:46:36 PDT 2024
https://github.com/labath updated https://github.com/llvm/llvm-project/pull/106532
>From 610757ced98139d3d10451dcca195692b0c2d832 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 29 Aug 2024 12:26:01 +0200
Subject: [PATCH 1/2] [lldb/linux] Make truncated reads work
Previously, we were returning an error if we couldn't read the whole
region. This doesn't matter most of the time, because lldb caches memory
reads, and in that process it aligns them to cache line boundaries. As
(LLDB) cache lines are smaller than pages, the reads are unlikely to
cross page boundaries.
Nonetheless, this can cause a problem for large reads (which bypass the
cache), where we're unable to read anything even if just a single byte
of the memory is unreadable. This patch fixes the lldb-server to do
that, and also changes the linux implementation, to reuse any partial
results it got from the process_vm_readv call (to avoid having to
re-read everything again using ptrace, only to find that it stopped at
the same place).
This matches debugserver behavior. It is also consistent with the gdb
remote protocol documentation, but -- notably -- not with actual
gdbserver behavior (which returns errors instead of partial results). We
filed a
[clarification bug](https://sourceware.org/bugzilla/show_bug.cgi?id=24751)
several years ago. Though we did not really reach a conclusion there, I
think this is the most logical behavior.
The associated test does not currently pass on windows, because the
windows memory read APIs don't support partial reads (I have a WIP patch
to work around that).
---
.../Process/Linux/NativeProcessLinux.cpp | 41 ++++-----
.../GDBRemoteCommunicationServerLLGS.cpp | 20 +----
.../API/functionalities/memory/holes/Makefile | 3 +
.../memory/holes/TestMemoryHoles.py | 63 ++++++++++++++
.../API/functionalities/memory/holes/main.cpp | 87 +++++++++++++++++++
5 files changed, 176 insertions(+), 38 deletions(-)
create mode 100644 lldb/test/API/functionalities/memory/holes/Makefile
create mode 100644 lldb/test/API/functionalities/memory/holes/TestMemoryHoles.py
create mode 100644 lldb/test/API/functionalities/memory/holes/main.cpp
diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index cb95da9fc72363..1e2e3a80b18bf6 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -1641,6 +1641,10 @@ NativeProcessLinux::GetSoftwareBreakpointTrapOpcode(size_t size_hint) {
Status NativeProcessLinux::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
size_t &bytes_read) {
+ Log *log = GetLog(POSIXLog::Memory);
+ LLDB_LOG(log, "addr = {0}, buf = {1}, size = {2}", addr, buf, size);
+
+ bytes_read = 0;
if (ProcessVmReadvSupported()) {
// The process_vm_readv path is about 50 times faster than ptrace api. We
// want to use this syscall if it is supported.
@@ -1651,32 +1655,29 @@ Status NativeProcessLinux::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
remote_iov.iov_base = reinterpret_cast<void *>(addr);
remote_iov.iov_len = size;
- bytes_read = process_vm_readv(GetCurrentThreadID(), &local_iov, 1,
- &remote_iov, 1, 0);
- const bool success = bytes_read == size;
+ ssize_t read_result = process_vm_readv(GetCurrentThreadID(), &local_iov, 1,
+ &remote_iov, 1, 0);
+ int error = 0;
+ if (read_result < 0)
+ error = errno;
+ else
+ bytes_read = read_result;
- Log *log = GetLog(POSIXLog::Process);
LLDB_LOG(log,
- "using process_vm_readv to read {0} bytes from inferior "
- "address {1:x}: {2}",
- size, addr, success ? "Success" : llvm::sys::StrError(errno));
-
- if (success)
- return Status();
- // else the call failed for some reason, let's retry the read using ptrace
- // api.
+ "process_vm_readv({0}, [iovec({1}, {2})], [iovec({3:x}, {2})], 1, "
+ "0) => {4} ({5})",
+ GetCurrentThreadID(), buf, size, addr, read_result,
+ error > 0 ? llvm::sys::StrError(errno) : "sucesss");
}
unsigned char *dst = static_cast<unsigned char *>(buf);
size_t remainder;
long data;
- Log *log = GetLog(POSIXLog::Memory);
- LLDB_LOG(log, "addr = {0}, buf = {1}, size = {2}", addr, buf, size);
-
- for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
+ for (; bytes_read < size; bytes_read += remainder) {
Status error = NativeProcessLinux::PtraceWrapper(
- PTRACE_PEEKDATA, GetCurrentThreadID(), (void *)addr, nullptr, 0, &data);
+ PTRACE_PEEKDATA, GetCurrentThreadID(),
+ reinterpret_cast<void *>(addr + bytes_read), nullptr, 0, &data);
if (error.Fail())
return error;
@@ -1684,11 +1685,7 @@ Status NativeProcessLinux::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
remainder = remainder > k_ptrace_word_size ? k_ptrace_word_size : remainder;
// Copy the data into our buffer
- memcpy(dst, &data, remainder);
-
- LLDB_LOG(log, "[{0:x}]:{1:x}", addr, data);
- addr += k_ptrace_word_size;
- dst += k_ptrace_word_size;
+ memcpy(dst + bytes_read, &data, remainder);
}
return Status();
}
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index f6f13a88587272..fc01586b72a792 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2526,28 +2526,16 @@ GDBRemoteCommunicationServerLLGS::Handle_memory_read(
size_t bytes_read = 0;
Status error = m_current_process->ReadMemoryWithoutTrap(
read_addr, &buf[0], byte_count, bytes_read);
- if (error.Fail()) {
- LLDB_LOGF(log,
- "GDBRemoteCommunicationServerLLGS::%s pid %" PRIu64
- " mem 0x%" PRIx64 ": failed to read. Error: %s",
- __FUNCTION__, m_current_process->GetID(), read_addr,
- error.AsCString());
+ LLDB_LOG(log, "ReadMemoryWithoutTrap({0}) read {1}/{2} bytes (error: {3})",
+ read_addr, byte_count, bytes_read, error);
+ if (bytes_read == 0)
return SendErrorResponse(0x08);
- }
-
- if (bytes_read == 0) {
- LLDB_LOGF(log,
- "GDBRemoteCommunicationServerLLGS::%s pid %" PRIu64
- " mem 0x%" PRIx64 ": read 0 of %" PRIu64 " requested bytes",
- __FUNCTION__, m_current_process->GetID(), read_addr, byte_count);
- return SendErrorResponse(0x08);
- }
StreamGDBRemote response;
packet.SetFilePos(0);
char kind = packet.GetChar('?');
if (kind == 'x')
- response.PutEscapedBytes(buf.data(), byte_count);
+ response.PutEscapedBytes(buf.data(), bytes_read);
else {
assert(kind == 'm');
for (size_t i = 0; i < bytes_read; ++i)
diff --git a/lldb/test/API/functionalities/memory/holes/Makefile b/lldb/test/API/functionalities/memory/holes/Makefile
new file mode 100644
index 00000000000000..99998b20bcb050
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/holes/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/memory/holes/TestMemoryHoles.py b/lldb/test/API/functionalities/memory/holes/TestMemoryHoles.py
new file mode 100644
index 00000000000000..49f26477d2e254
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/holes/TestMemoryHoles.py
@@ -0,0 +1,63 @@
+"""
+Test the memory commands operating on memory regions with holes
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+
+
+class MemoryHolesTestCase(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def setUp(self):
+ super().setUp()
+ # Find the line number to break inside main().
+ self.line = line_number("main.cpp", "// break here")
+
+ def _prepare_inferior(self):
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+ # Break in main() after the variables are assigned values.
+ lldbutil.run_break_set_by_file_and_line(
+ self, "main.cpp", self.line, num_expected_locations=1, loc_exact=True
+ )
+
+ self.runCmd("run", RUN_SUCCEEDED)
+
+ # The stop reason of the thread should be breakpoint.
+ self.expect(
+ "thread list",
+ STOPPED_DUE_TO_BREAKPOINT,
+ substrs=["stopped", "stop reason = breakpoint"],
+ )
+
+ # The breakpoint should have a hit count of 1.
+ lldbutil.check_breakpoint(self, bpno=1, expected_hit_count=1)
+
+ # Avoid the expression evaluator, as it can allocate allocate memory
+ # inside the holes we've deliberately left empty.
+ self.memory = self.frame().FindVariable("mem_with_holes").GetValueAsUnsigned()
+ self.pagesize = self.frame().FindVariable("pagesize").GetValueAsUnsigned()
+ positions = self.frame().FindVariable("positions")
+ self.positions = [
+ positions.GetChildAtIndex(i).GetValueAsUnsigned()
+ for i in range(positions.GetNumChildren())
+ ]
+ self.assertEqual(len(self.positions), 5)
+
+ @expectedFailureWindows
+ def test_memory_read(self):
+ self._prepare_inferior()
+
+ error = lldb.SBError()
+ content = self.process().ReadMemory(self.memory, 2 * self.pagesize, error)
+ self.assertEqual(len(content), self.pagesize)
+ self.assertEqual(content[0:7], b"needle\0")
+ self.assertTrue(error.Fail())
+ self.assertEqual(
+ f"memory read failed for {self.memory+self.pagesize:#x}", error.GetCString()
+ )
diff --git a/lldb/test/API/functionalities/memory/holes/main.cpp b/lldb/test/API/functionalities/memory/holes/main.cpp
new file mode 100644
index 00000000000000..b641fd2ebe1b96
--- /dev/null
+++ b/lldb/test/API/functionalities/memory/holes/main.cpp
@@ -0,0 +1,87 @@
+#include <algorithm>
+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+#include <iostream>
+
+constexpr size_t num_pages = 7;
+constexpr size_t accessible_pages[] = {0, 2, 4, 6};
+
+bool is_accessible(size_t page) {
+ return std::find(std::begin(accessible_pages), std::end(accessible_pages),
+ page) != std::end(accessible_pages);
+}
+
+// allocate_memory_with_holes returns a pointer to `num_pages` pages of memory,
+// where some of the pages are inaccessible (even to debugging APIs). We use
+// this to test lldb's ability to skip over inaccessible blocks.
+#ifdef _WIN32
+#include "Windows.h"
+
+int getpagesize() {
+ SYSTEM_INFO system_info;
+ GetSystemInfo(&system_info);
+ return system_info.dwPageSize;
+}
+
+char *allocate_memory_with_holes() {
+ int pagesize = getpagesize();
+ void *mem = VirtualAlloc(nullptr, num_pages * pagesize, MEM_RESERVE, PAGE_NOACCESS);
+ if (!mem) {
+ std::cerr << std::system_category().message(GetLastError()) << std::endl;
+ exit(1);
+ }
+ char *bytes = static_cast<char *>(mem);
+ for (size_t page = 0; page < num_pages; ++page) {
+ if (!is_accessible(page))
+ continue;
+ if (!VirtualAlloc(bytes + page * pagesize, pagesize, MEM_COMMIT,
+ PAGE_READWRITE)) {
+ std::cerr << std::system_category().message(GetLastError()) << std::endl;
+ exit(1);
+ }
+ }
+ return bytes;
+}
+#else
+#include "sys/mman.h"
+#include "unistd.h"
+
+char *allocate_memory_with_holes() {
+ int pagesize = getpagesize();
+ void *mem = mmap(nullptr, num_pages * pagesize, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (mem == MAP_FAILED) {
+ perror("mmap");
+ exit(1);
+ }
+ char *bytes = static_cast<char *>(mem);
+ for (size_t page = 0; page < num_pages; ++page) {
+ if (is_accessible(page))
+ continue;
+ if (munmap(bytes + page * pagesize, pagesize) != 0) {
+ perror("munmap");
+ exit(1);
+ }
+ }
+ return bytes;
+}
+#endif
+
+int main(int argc, char const *argv[]) {
+ char *mem_with_holes = allocate_memory_with_holes();
+ int pagesize = getpagesize();
+ char *positions[] = {
+ mem_with_holes, // Beginning of memory
+ mem_with_holes + 2 * pagesize, // After a hole
+ mem_with_holes + 2 * pagesize +
+ pagesize / 2, // Middle of a block, after an existing match.
+ mem_with_holes + 5 * pagesize - 7, // End of a block
+ mem_with_holes + 7 * pagesize - 7, // End of memory
+ };
+ for (char *p : positions)
+ strcpy(p, "needle");
+
+ return 0; // break here
+}
>From 03896b5a8dd2ef5893f47547d892e4bd9d772751 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 29 Aug 2024 13:46:16 +0200
Subject: [PATCH 2/2] clang-format
---
lldb/test/API/functionalities/memory/holes/main.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lldb/test/API/functionalities/memory/holes/main.cpp b/lldb/test/API/functionalities/memory/holes/main.cpp
index b641fd2ebe1b96..3228a1bff76716 100644
--- a/lldb/test/API/functionalities/memory/holes/main.cpp
+++ b/lldb/test/API/functionalities/memory/holes/main.cpp
@@ -27,7 +27,8 @@ int getpagesize() {
char *allocate_memory_with_holes() {
int pagesize = getpagesize();
- void *mem = VirtualAlloc(nullptr, num_pages * pagesize, MEM_RESERVE, PAGE_NOACCESS);
+ void *mem =
+ VirtualAlloc(nullptr, num_pages * pagesize, MEM_RESERVE, PAGE_NOACCESS);
if (!mem) {
std::cerr << std::system_category().message(GetLastError()) << std::endl;
exit(1);
More information about the lldb-commits
mailing list