[lldb/linux] Make truncated reads work

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,44 +1655,37 @@ 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;
 
     remainder = size - bytes_read;
     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();
 }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

@@ -2526,28 +2526,18 @@ 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());
-    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);
+  LLDB_LOG(
+      log,
+      "ReadMemoryWithoutTrap({0}) read {1} of {2} requested bytes (error: {3})",
+      read_addr, byte_count, bytes_read, error);
+  if (bytes_read == 0)
     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)

lldb/test/API/functionalities/memory/holes/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/functionalities/memory/holes/TestMemoryHoles.py

@@ -0,0 +1,61 @@
+"""
+Test the memory commands operating on memory regions with holes (inaccessible
+pages)
+"""
+
+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())

lldb/test/API/functionalities/memory/holes/main.cpp

@@ -0,0 +1,88 @@
+#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
+}