NativeProcessProtocol: Simplify breakpoint setting code

Summary:
A fairly simple operation as setting a breakpoint (writing a breakpoint
opcode) at a given address was going through three classes:
NativeProcessProtocol which called NativeBreakpointList, which then
called SoftwareBrekpoint, only to end up again in NativeProcessProtocol
to do the actual writing itself. This is unnecessarily complex and can
be simplified by moving all of the logic into NativeProcessProtocol
class itself, removing a lot of boilerplate.

One of the reeasons for this complexity was that (it seems)
NativeBreakpointList class was meant to hold both software and hardware
breakpoints. However, that never materialized, and hardware breakpoints
are stored in a separate map holding only hardware breakpoints.
Essentially, this patch makes software breakpoints follow that approach
by replacing the heavy SoftwareBraekpoint with a light struct of the
same name, which holds only the data necessary to describe one
breakpoint. The rest of the logic is in the main class. As, at the
lldb-server level, handling software and hardware breakpoints is very
different, this seems like a reasonable state of things.

Reviewers: krytarowski, zturner, clayborg

Subscribers: mgorny, lldb-commits

Differential Revision: https://reviews.llvm.org/D52941

llvm-svn: 346093

1 files changed, 183 insertions, 40 deletions

diff --git a/lldb/source/Host/common/NativeProcessProtocol.cpp b/lldb/source/Host/common/NativeProcessProtocol.cpp
index 5d0eff7..6411c9f 100644
--- a/lldb/source/Host/common/NativeProcessProtocol.cpp
+++ b/lldb/source/Host/common/NativeProcessProtocol.cpp
@@ -9,9 +9,9 @@
 
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/common/NativeBreakpointList.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/common/NativeThreadProtocol.h"
-#include "lldb/Host/common/SoftwareBreakpoint.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
@@ -359,17 +359,161 @@ void NativeProcessProtocol::NotifyDidExec() {
 Status NativeProcessProtocol::SetSoftwareBreakpoint(lldb::addr_t addr,
                                                     uint32_t size_hint) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
-  if (log)
-    log->Printf("NativeProcessProtocol::%s addr = 0x%" PRIx64, __FUNCTION__,
-                addr);
+  LLDB_LOG(log, "addr = {0:x}, size_hint = {1}", addr, size_hint);
+
+  auto it = m_software_breakpoints.find(addr);
+  if (it != m_software_breakpoints.end()) {
+    ++it->second.ref_count;
+    return Status();
+  }
+  auto expected_bkpt = EnableSoftwareBreakpoint(addr, size_hint);
+  if (!expected_bkpt)
+    return Status(expected_bkpt.takeError());
 
-  return m_breakpoint_list.AddRef(
-      addr, size_hint, false,
-      [this](lldb::addr_t addr, size_t size_hint, bool /* hardware */,
-             NativeBreakpointSP &breakpoint_sp) -> Status {
-        return SoftwareBreakpoint::CreateSoftwareBreakpoint(
-            *this, addr, size_hint, breakpoint_sp);
-      });
+  m_software_breakpoints.emplace(addr, std::move(*expected_bkpt));
+  return Status();
+}
+
+Status NativeProcessProtocol::RemoveSoftwareBreakpoint(lldb::addr_t addr) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
+  LLDB_LOG(log, "addr = {0:x}", addr);
+  auto it = m_software_breakpoints.find(addr);
+  if (it == m_software_breakpoints.end())
+    return Status("Breakpoint not found.");
+  assert(it->second.ref_count > 0);
+  if (--it->second.ref_count > 0)
+    return Status();
+
+  // This is the last reference. Let's remove the breakpoint.
+  Status error;
+
+  // Clear a software breakpoint instruction
+  llvm::SmallVector<uint8_t, 4> curr_break_op(
+      it->second.breakpoint_opcodes.size(), 0);
+
+  // Read the breakpoint opcode
+  size_t bytes_read = 0;
+  error =
+      ReadMemory(addr, curr_break_op.data(), curr_break_op.size(), bytes_read);
+  if (error.Fail() || bytes_read < curr_break_op.size()) {
+    return Status("addr=0x%" PRIx64
+                  ": tried to read %zu bytes but only read %zu",
+                  addr, curr_break_op.size(), bytes_read);
+  }
+  const auto &saved = it->second.saved_opcodes;
+  // Make sure the breakpoint opcode exists at this address
+  if (makeArrayRef(curr_break_op) != it->second.breakpoint_opcodes) {
+    if (curr_break_op != it->second.saved_opcodes)
+      return Status("Original breakpoint trap is no longer in memory.");
+    LLDB_LOG(log,
+             "Saved opcodes ({0:@[x]}) have already been restored at {1:x}.",
+             llvm::make_range(saved.begin(), saved.end()), addr);
+  } else {
+    // We found a valid breakpoint opcode at this address, now restore the
+    // saved opcode.
+    size_t bytes_written = 0;
+    error = WriteMemory(addr, saved.data(), saved.size(), bytes_written);
+    if (error.Fail() || bytes_written < saved.size()) {
+      return Status("addr=0x%" PRIx64
+                    ": tried to write %zu bytes but only wrote %zu",
+                    addr, saved.size(), bytes_written);
+    }
+
+    // Verify that our original opcode made it back to the inferior
+    llvm::SmallVector<uint8_t, 4> verify_opcode(saved.size(), 0);
+    size_t verify_bytes_read = 0;
+    error = ReadMemory(addr, verify_opcode.data(), verify_opcode.size(),
+                       verify_bytes_read);
+    if (error.Fail() || verify_bytes_read < verify_opcode.size()) {
+      return Status("addr=0x%" PRIx64
+                    ": tried to read %zu verification bytes but only read %zu",
+                    addr, verify_opcode.size(), verify_bytes_read);
+    }
+    if (verify_opcode != saved)
+      LLDB_LOG(log, "Restoring bytes at {0:x}: {1:@[x]}", addr,
+               llvm::make_range(saved.begin(), saved.end()));
+  }
+
+  m_software_breakpoints.erase(it);
+  return Status();
+}
+
+llvm::Expected<NativeProcessProtocol::SoftwareBreakpoint>
+NativeProcessProtocol::EnableSoftwareBreakpoint(lldb::addr_t addr,
+                                                uint32_t size_hint) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
+
+  auto expected_trap = GetSoftwareBreakpointTrapOpcode(size_hint);
+  if (!expected_trap)
+    return expected_trap.takeError();
+
+  llvm::SmallVector<uint8_t, 4> saved_opcode_bytes(expected_trap->size(), 0);
+  // Save the original opcodes by reading them so we can restore later.
+  size_t bytes_read = 0;
+  Status error = ReadMemory(addr, saved_opcode_bytes.data(),
+                            saved_opcode_bytes.size(), bytes_read);
+  if (error.Fail())
+    return error.ToError();
+
+  // Ensure we read as many bytes as we expected.
+  if (bytes_read != saved_opcode_bytes.size()) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Failed to read memory while attempting to set breakpoint: attempted "
+        "to read {0} bytes but only read {1}.",
+        saved_opcode_bytes.size(), bytes_read);
+  }
+
+  LLDB_LOG(
+      log, "Overwriting bytes at {0:x}: {1:@[x]}", addr,
+      llvm::make_range(saved_opcode_bytes.begin(), saved_opcode_bytes.end()));
+
+  // Write a software breakpoint in place of the original opcode.
+  size_t bytes_written = 0;
+  error = WriteMemory(addr, expected_trap->data(), expected_trap->size(),
+                      bytes_written);
+  if (error.Fail())
+    return error.ToError();
+
+  // Ensure we wrote as many bytes as we expected.
+  if (bytes_written != expected_trap->size()) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Failed write memory while attempting to set "
+        "breakpoint: attempted to write {0} bytes but only wrote {1}",
+        expected_trap->size(), bytes_written);
+  }
+
+  llvm::SmallVector<uint8_t, 4> verify_bp_opcode_bytes(expected_trap->size(),
+                                                       0);
+  size_t verify_bytes_read = 0;
+  error = ReadMemory(addr, verify_bp_opcode_bytes.data(),
+                     verify_bp_opcode_bytes.size(), verify_bytes_read);
+  if (error.Fail())
+    return error.ToError();
+
+  // Ensure we read as many verification bytes as we expected.
+  if (verify_bytes_read != verify_bp_opcode_bytes.size()) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Failed to read memory while "
+        "attempting to verify breakpoint: attempted to read {0} bytes "
+        "but only read {1}",
+        verify_bp_opcode_bytes.size(), verify_bytes_read);
+  }
+
+  if (llvm::makeArrayRef(verify_bp_opcode_bytes.data(), verify_bytes_read) !=
+      *expected_trap) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Verification of software breakpoint "
+        "writing failed - trap opcodes not successfully read back "
+        "after writing when setting breakpoint at {0:x}",
+        addr);
+  }
+
+  LLDB_LOG(log, "addr = {0:x}: SUCCESS");
+  return SoftwareBreakpoint{1, saved_opcode_bytes, *expected_trap};
 }
 
 llvm::Expected<llvm::ArrayRef<uint8_t>>
@@ -455,27 +599,15 @@ void NativeProcessProtocol::FixupBreakpointPCAsNeeded(
   if (breakpoint_addr >= breakpoint_size)
     breakpoint_addr -= breakpoint_size;
 
-  // Check if we stopped because of a breakpoint.
-  NativeBreakpointSP breakpoint_sp;
-  error = m_breakpoint_list.GetBreakpoint(breakpoint_addr, breakpoint_sp);
-  if (!error.Success() || !breakpoint_sp) {
+  if (m_software_breakpoints.count(breakpoint_addr) == 0) {
     // We didn't find one at a software probe location.  Nothing to do.
     LLDB_LOG(log,
-             "pid {0} no lldb breakpoint found at current pc with "
+             "pid {0} no lldb software breakpoint found at current pc with "
              "adjustment: {1}",
              GetID(), breakpoint_addr);
     return;
   }
 
-  // If the breakpoint is not a software breakpoint, nothing to do.
-  if (!breakpoint_sp->IsSoftwareBreakpoint()) {
-    LLDB_LOG(
-        log,
-        "pid {0} breakpoint found at {1:x}, not software, nothing to adjust",
-        GetID(), breakpoint_addr);
-    return;
-  }
-
   //
   // We have a software breakpoint and need to adjust the PC.
   //
@@ -499,20 +631,7 @@ Status NativeProcessProtocol::RemoveBreakpoint(lldb::addr_t addr,
   if (hardware)
     return RemoveHardwareBreakpoint(addr);
   else
-    return m_breakpoint_list.DecRef(addr);
-}
-
-Status NativeProcessProtocol::EnableBreakpoint(lldb::addr_t addr) {
-  return m_breakpoint_list.EnableBreakpoint(addr);
-}
-
-Status NativeProcessProtocol::DisableBreakpoint(lldb::addr_t addr) {
-  return m_breakpoint_list.DisableBreakpoint(addr);
-}
-
-lldb::StateType NativeProcessProtocol::GetState() const {
-  std::lock_guard<std::recursive_mutex> guard(m_state_mutex);
-  return m_state;
+    return RemoveSoftwareBreakpoint(addr);
 }
 
 Status NativeProcessProtocol::ReadMemoryWithoutTrap(lldb::addr_t addr,
@@ -521,7 +640,31 @@ Status NativeProcessProtocol::ReadMemoryWithoutTrap(lldb::addr_t addr,
   Status error = ReadMemory(addr, buf, size, bytes_read);
   if (error.Fail())
     return error;
-  return m_breakpoint_list.RemoveTrapsFromBuffer(addr, buf, size);
+
+  auto data =
+      llvm::makeMutableArrayRef(static_cast<uint8_t *>(buf), bytes_read);
+  for (const auto &pair : m_software_breakpoints) {
+    lldb::addr_t bp_addr = pair.first;
+    auto saved_opcodes = makeArrayRef(pair.second.saved_opcodes);
+
+    if (bp_addr + saved_opcodes.size() < addr || addr + bytes_read <= bp_addr)
+      continue; // Breapoint not in range, ignore
+
+    if (bp_addr < addr) {
+      saved_opcodes = saved_opcodes.drop_front(addr - bp_addr);
+      bp_addr = addr;
+    }
+    auto bp_data = data.drop_front(bp_addr - addr);
+    std::copy_n(saved_opcodes.begin(),
+                std::min(saved_opcodes.size(), bp_data.size()),
+                bp_data.begin());
+  }
+  return Status();
+}
+
+lldb::StateType NativeProcessProtocol::GetState() const {
+  std::lock_guard<std::recursive_mutex> guard(m_state_mutex);
+  return m_state;
 }
 
 void NativeProcessProtocol::SetState(lldb::StateType state,