[lldb] Skip overlapping hardware and external breakpoints when writing memory

[tatyana-krasnukha]

Summary

This fixes the assertion assert(intersects); in the Process::WriteMemory function.

[labath]

When does this assertion fire? Is this breaking an existing test? Should we be adding a new one?

[tatyana-krasnukha]

BreakpointSite::IntersectsRange returns false for hardware breakpoints, that's why the assertion fails.

Since my bare-metal target loads process's memory via Process::WriteMemory, I have 200+ unresolved tests without this change.

The added test should reproduce the issue for other targets too.

[labath]

In D84254#2166562, @tatyana-krasnukha wrote:

BreakpointSite::IntersectsRange returns false for hardware breakpoints, that's why the assertion fails.

Since my bare-metal target loads process's memory via Process::WriteMemory, I have 200+ unresolved tests without this change.

The added test should reproduce the issue for other targets too.

I see. That makes sense, thanks for the test.

My eyes nearly popped out when I saw that we were debugging a process from _inside_ a decorator (to determine watchpoint support status). I don't know when this stuff was introduced -- it sounds like a bad idea to me -- but I don't want to hold up your patch over that.

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/write_memory_with_hw_breakpoint/TestWriteMemoryWithHWBreakpoint.py

23–34

These days we have helper functions for this typical test setup. I guess this could be lldbutil.run_to_name_breakpoint

51

We also have a new assertSuccess helper with better error messages. It would be better to write this as assertSuccess(error); assertEquals(result, len(bytes))

lldb/source/Breakpoint/BreakpointSite.cpp

 //===-- BreakpointSite.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <inttypes.h>
 
 #include "lldb/Breakpoint/BreakpointSite.h"
 
 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointSiteList.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
for (BreakpointLocationSP loc_sp : m_owners.BreakpointLocations()) {
     loc_sp->SetHardwareIndex(index);
   }
 }
 
 bool BreakpointSite::IntersectsRange(lldb::addr_t addr, size_t size,
                                      lldb::addr_t *intersect_addr,
                                      size_t *intersect_size,
                                      size_t *opcode_offset) const {
-  // We only use software traps for software breakpoints
-  if (!IsHardware()) {
-    if (m_byte_size > 0) {
-      const lldb::addr_t bp_end_addr = m_addr + m_byte_size;
-      const lldb::addr_t end_addr = addr + size;
-      // Is the breakpoint end address before the passed in start address?
-      if (bp_end_addr <= addr)
-        return false;
-      // Is the breakpoint start address after passed in end address?
-      if (end_addr <= m_addr)
-        return false;
-      if (intersect_addr || intersect_size || opcode_offset) {
-        if (m_addr < addr) {
-          if (intersect_addr)
-            *intersect_addr = addr;
-          if (intersect_size)
-            *intersect_size =
-                std::min<lldb::addr_t>(bp_end_addr, end_addr) - addr;
-          if (opcode_offset)
-            *opcode_offset = addr - m_addr;
-        } else {
-          if (intersect_addr)
-            *intersect_addr = m_addr;
-          if (intersect_size)
-            *intersect_size =
-                std::min<lldb::addr_t>(bp_end_addr, end_addr) - m_addr;
-          if (opcode_offset)
-            *opcode_offset = 0;
-        }
-      }
-      return true;
-    }
-  }
-  return false;
-}
+  // The function should be called only for software breakpoints.
+  lldbassert(GetType() == Type::eSoftware);
+
 
 size_t
 BreakpointSite::CopyOwnersList(BreakpointLocationCollection &out_collection) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
   for (BreakpointLocationSP loc_sp : m_owners.BreakpointLocations()) {
     out_collection.Add(loc_sp);
   }
   return out_collection.GetSize();
 }

lldb/source/Target/Process.cpp

 //===-- Process.cpp -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <atomic>
 #include <memory>
 #include <mutex>
 
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/DynamicCheckerFunctions.h"
#endif
   const uint8_t *ubuf = (const uint8_t *)buf;
   uint64_t bytes_written = 0;
 
   bp_sites_in_range.ForEach([this, addr, size, &bytes_written, &ubuf,
                              &error](BreakpointSite *bp) -> void {
     if (error.Fail())
       return;
 
+    if (bp->GetType() != BreakpointSite::eSoftware)
+      return;
+
     addr_t intersect_addr;
     size_t intersect_size;
     size_t opcode_offset;
     const bool intersects = bp->IntersectsRange(
         addr, size, &intersect_addr, &intersect_size, &opcode_offset);
     UNUSED_IF_ASSERT_DISABLED(intersects);
     assert(intersects);
     assert(addr <= intersect_addr && intersect_addr < addr + size);

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/write_memory_with_hw_breakpoint/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/write_memory_with_hw_breakpoint/TestWriteMemoryWithHWBreakpoint.py

+"""
+Test that writing memory does't affect hardware breakpoints.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+from functionalities.breakpoint.hardware_breakpoints.base import *
+
+class WriteMemoryWithHWBreakpoint(HardwareBreakpointTestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def does_not_support_hw_breakpoints(self):
+        return not super().supports_hw_breakpoints()
+
+    @skipTestIfFn(does_not_support_hw_breakpoints)
+    def test_copy_memory_with_hw_break(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Run the program and stop at entry.
+        self.expect("process launch --stop-at-entry",
+            patterns=["Process .* launched: .*a.out"])
+
+        process = target.GetProcess()
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        # Set a hardware breakpoint.
+        bp_id = lldbutil.run_break_set_by_symbol(self, "hw_break_function",
+                                                 extra_options="--hardware")
+
+        # Get breakpoint location from the breakpoint.
+        location = target.FindBreakpointByID(bp_id).GetLocationAtIndex(0)
+        self.assertTrue(location and location.IsResolved(),
+                        VALID_BREAKPOINT_LOCATION)
+
+        # Check that writing overlapping memory doesn't crash.
+        address = location.GetLoadAddress()
+        data = str("\x01\x02\x03\x04")
+        error = lldb.SBError()
+
+        result = process.WriteMemory(address, data, error)
+        self.assertTrue(error.Success() and result == len(bytes))

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/write_memory_with_hw_breakpoint/main.cpp

+static volatile int num = 1;
+
+bool hw_break_function (int i) {
+  return num == i;
+}
+
+int main (int argc, char const *argv[]) {
+  return hw_break_function(argc) ? 0 : 1;
+}