Author: David Spickett Date: 2026-08-21T12:23:58Z New Revision: a132493da2f60a08cc66c75fa0dd12ccb3fe9c26
URL: https://github.com/llvm/llvm-project/commit/a132493da2f60a08cc66c75fa0dd12ccb3fe9c26 DIFF: https://github.com/llvm/llvm-project/commit/a132493da2f60a08cc66c75fa0dd12ccb3fe9c26.diff LOG: [LLDB][lldb-server] Handle writes over software breakpoint sites (#217348) Fixes #202672 lldb-server was not handling them at all, and I found bugs in debugserver's implementation. This change only fixes lldb-server, the debugserver issue is tracked by #217359 and #217840 (though the added API tests can be run for debugserver once that's done). When data is written over a software breakpoint location, it should not be written to memory but instead to the saved copy of the original data at that location. If this is not done, writes corrupt the software break instruction and it's no longer a valid breakpoint. This is how it should work: ``` Memory contents: ----abcd---- Place a breakpoint: ----bkpt---- (saved bytes: abcd) Then we write over it: --abcdefgh-- The result: --abbkptgh-- (saved bytes: cdef) Remove the breakpoint: --abcdefgh-- ``` When the breakpoint is removed, the saved bytes are written to memory and it looks just like you had written it all in the first place. This is what was happening with lldb-server: ``` Memory contents: ----abcd---- Place a breakpoint: ----bkpt---- (saved bytes: abcd) Then we write over it: --abcdefgh-- The result: --abcdefgh-- (saved bytes: bkpt) (break no longer valid) Remove the breakpoint: Errors because it doesn't see the break instruction in memory. ``` debugserver already has an algorithm to work out how to do this. It's in MachProcess::WriteMemory and what I've added to NativeProcessProtocol follows the same basic logic. * Assume breakpoints are sorted in ascending order of address (hence the change from std::unordered_map to std::map). * Skip forward to the first breakpoint that starts > the write address. * If that breakpoint is not the first, go back one breakpoint. This catches the case where a breakpoint starts earlier than the write but still over laps it, or starts exactly when the write does. * If the write address is before the first breakpoint, write up to that breakpoint. * If the write address is at or in a breakpoint, update that breakpoint's saved bytes until you reach the end of it, or the write is complete. * If there's write data left, move the write address forward to just beyond the breakpoint. * Repeat for all breakpoints from that point on in the list, or until the write is complete. * Finally, check for any remaining bytes to be written after the last breakpoint. This means a single write is split into multiple sometimes. The alternative is to make a copy of the write data, and patch the breakpoint instructions into that to create 1 write request. I decided not to do that because: * debugserver does not do this, and we may need to compare them later. * It is harder to reason about recovery if the single write fails. NativeProcessProtocol ReadMemory is no longer abstract, instead there is a lower level DoWriteMemory which sub-classes implement. DoWriteMemory is what WriteMemory used to be, so some places call it directly because they want to ignore breakpoint sites. I tried to avoid this extra abstraction layer, but I do not see a good alternative. Tests have been added at the NativeProccess level for different layouts. The expected number of write calls is checked and breakpoint insertion is tested in 2 orders if there is > 1. There is an API test based on the steps reported in #216000. This test should run on all architectures. For now it won't run for debugserver. I have had to take a small risk to get some dead code immediately before the breakpoint site. Short of using target specific assembly, I think what I've got is as safe as we can be. Limiting this test to AArch64 would not be a big problem, as the same write splitting algorithm is used on all architectures. However just on principle I would like it to be as generic as it can be. I did have to skip Windows, because I could not find a way to make the breakpoint location label visible to lldb. The code being tested should work on Windows, if someone can figure out the symbol part. The loss of coverage is minimal because the algorithm under test is not OS specific. Added: lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/Makefile lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/main.c Modified: lldb/include/lldb/Host/common/NativeProcessProtocol.h lldb/source/Host/common/NativeProcessProtocol.cpp lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp lldb/source/Plugins/Process/AIX/NativeProcessAIX.h lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp lldb/source/Plugins/Process/Linux/NativeProcessLinux.h lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.cpp lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.h lldb/unittests/Host/NativeProcessProtocolTest.cpp lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h Removed: ################################################################################ diff --git a/lldb/include/lldb/Host/common/NativeProcessProtocol.h b/lldb/include/lldb/Host/common/NativeProcessProtocol.h index 67206c4b55b79..2bb9e9a2532ad 100644 --- a/lldb/include/lldb/Host/common/NativeProcessProtocol.h +++ b/lldb/include/lldb/Host/common/NativeProcessProtocol.h @@ -27,9 +27,9 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" +#include <map> #include <mutex> #include <optional> -#include <unordered_map> #include <vector> namespace lldb_private { @@ -134,8 +134,10 @@ class NativeProcessProtocol { ReadCStringFromMemory(lldb::addr_t addr, char *buffer, size_t max_size, size_t &total_bytes_read); - virtual Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) = 0; + /// Write memory while not overwriting breakpoints in memory. The breakpoints' + /// saved bytes are updated with what would have been written. + Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written); virtual llvm::Expected<lldb::addr_t> AllocateMemory(size_t size, uint32_t permissions) { @@ -457,7 +459,9 @@ class NativeProcessProtocol { llvm::ArrayRef<uint8_t> breakpoint_opcodes; }; - std::unordered_map<lldb::addr_t, SoftwareBreakpoint> m_software_breakpoints; + // Using std::map so that breakpoints are sorted in ascending address order. + // WriteMemory relies on this. + std::map<lldb::addr_t, SoftwareBreakpoint> m_software_breakpoints; lldb::pid_t m_pid; std::vector<std::unique_ptr<NativeThreadProtocol>> m_threads; @@ -482,6 +486,13 @@ class NativeProcessProtocol { // Extensions enabled per the last SetEnabledExtensions() call. Extension m_enabled_extensions; + // Write to memory, with no awareness of software breakpoint sites. Used to + // implement WriteMemory. May be called directly for use cases that should + // ignore software breakpoint sites, for example adding or removing those + // breakpoints. + virtual Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) = 0; + // lldb_private::Host calls should be used to launch a process for debugging, // and then the process should be attached to. When attaching to a process // lldb_private::Host calls should be used to locate the process to attach diff --git a/lldb/source/Host/common/NativeProcessProtocol.cpp b/lldb/source/Host/common/NativeProcessProtocol.cpp index 8c5991e93aab3..71737c3101b90 100644 --- a/lldb/source/Host/common/NativeProcessProtocol.cpp +++ b/lldb/source/Host/common/NativeProcessProtocol.cpp @@ -18,6 +18,7 @@ #include "lldb/lldb-enumerations.h" #include "llvm/Support/Process.h" +#include <iterator> #include <optional> using namespace lldb; @@ -396,7 +397,7 @@ Status NativeProcessProtocol::RemoveSoftwareBreakpoint(lldb::addr_t addr) { // 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); + error = DoWriteMemory(addr, saved.data(), saved.size(), bytes_written); if (error.Fail() || bytes_written < saved.size()) { return Status::FromErrorStringWithFormat( "addr=0x%" PRIx64 ": tried to write %zu bytes but only wrote %zu", @@ -454,8 +455,8 @@ NativeProcessProtocol::EnableSoftwareBreakpoint(lldb::addr_t addr, // 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); + error = DoWriteMemory(addr, expected_trap->data(), expected_trap->size(), + bytes_written); if (error.Fail()) return error.ToError(); @@ -649,6 +650,87 @@ Status NativeProcessProtocol::RemoveBreakpoint(lldb::addr_t addr, return RemoveSoftwareBreakpoint(addr); } +Status NativeProcessProtocol::WriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { + Status error; + bytes_written = 0; + + if (!size) + return error; + + if (m_software_breakpoints.empty()) + return DoWriteMemory(addr, buf, size, bytes_written); + + // Find first breakpoint that starts >= addr. + std::map<lldb::addr_t, SoftwareBreakpoint>::iterator bkpt = + m_software_breakpoints.lower_bound(addr); + + // it points to the first breakpoint starting at >= addr, but the one + // immediately before it may extend over addr, or begin exactly at addr. + if (bkpt != m_software_breakpoints.begin()) + bkpt = std::prev(bkpt); + + const uint8_t *byte_buf = static_cast<const uint8_t *>(buf); + for (; bkpt != m_software_breakpoints.end(); ++bkpt) { + auto &[sbp_addr, sbp_data] = *bkpt; + // If the address is before a breakpoint site, write up to the site, or to + // the end of the write. Whichever comes first. + if (addr < sbp_addr) { + const size_t to_write = std::min(size, size_t{sbp_addr - addr}); + size_t part_bytes_written = 0; + error = DoWriteMemory(addr, byte_buf, to_write, part_bytes_written); + bytes_written += part_bytes_written; + + if (error.Fail() || part_bytes_written < to_write) { + return Status::FromErrorStringWithFormat( + "addr=0x%" PRIx64 ": tried to write %zu bytes but only wrote %zu", + addr, to_write, part_bytes_written); + } + + byte_buf += to_write; + addr += to_write; + size -= to_write; + + if (!size) + break; + } + + // If the address is within a breakpoint site, update the saved opcodes + // for that site. + if ((addr >= sbp_addr) && + (addr < (sbp_addr + sbp_data.saved_opcodes.size()))) { + // Instead of writing this chunk, update the saved bytes in the + // breakpoint. + const size_t idx = addr - sbp_addr; + const size_t to_write = + std::min(size, sbp_data.saved_opcodes.size() - idx); + for (size_t copied = 0; copied < to_write; + ++bytes_written, ++byte_buf, ++addr, --size, ++copied) + sbp_data.saved_opcodes[idx + copied] = *byte_buf; + } + + if (!size) + break; + } + + // If the write range extends beyond the last breakpoint site, write the + // remaining data. + if (size) { + // Write the remaining part after the last breakpoint, or the whole range + // in the case that there were no breakpoints. + size_t part_bytes_written = 0; + error = DoWriteMemory(addr, byte_buf, size, part_bytes_written); + bytes_written += part_bytes_written; + if (error.Fail() || part_bytes_written < size) { + return Status::FromErrorStringWithFormat( + "addr=0x%" PRIx64 ": tried to write %zu bytes but only wrote %zu", + addr, size, part_bytes_written); + } + } + + return Status(); +} + Status NativeProcessProtocol::ReadMemoryWithoutTrap(const ProcessAddress &process_addr, void *buf, size_t size, diff --git a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp index de11641790655..6d99c22857549 100644 --- a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp +++ b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp @@ -244,8 +244,8 @@ Status NativeProcessAIX::ReadMemory(const ProcessAddress &process_addr, return Status("unsupported"); } -Status NativeProcessAIX::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status NativeProcessAIX::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { return Status("unsupported"); } diff --git a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.h b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.h index 068b428e1c8a9..e862250055165 100644 --- a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.h +++ b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.h @@ -80,8 +80,8 @@ class NativeProcessAIX : public NativeProcessProtocol { Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; size_t UpdateThreads() override; diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp index 74fdda1fd7934..1e8f37367eb7f 100644 --- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp +++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp @@ -900,8 +900,8 @@ Status NativeProcessFreeBSD::ReadMemory(const ProcessAddress &process_addr, return Status(); } -Status NativeProcessFreeBSD::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status NativeProcessFreeBSD::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { const unsigned char *src = static_cast<const unsigned char *>(buf); Status error; struct ptrace_io_desc io; diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h index aecb7ab74d0f3..a22a385dfc6a4 100644 --- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h +++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h @@ -62,8 +62,8 @@ class NativeProcessFreeBSD : public NativeProcessELF { Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; size_t UpdateThreads() override; diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp index d15907d7cedb7..758e46959965c 100644 --- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp +++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp @@ -1347,8 +1347,9 @@ NativeProcessLinux::Syscall(llvm::ArrayRef<uint64_t> args) { return std::move(Err); } - llvm::scope_exit restore_mem( - [&] { WriteMemory(exe_addr, memory.data(), memory.size(), bytes_read); }); + llvm::scope_exit restore_mem([&] { + DoWriteMemory(exe_addr, memory.data(), memory.size(), bytes_read); + }); if (llvm::Error Err = reg_ctx.SetPC(exe_addr).ToError()) return std::move(Err); @@ -1361,8 +1362,8 @@ NativeProcessLinux::Syscall(llvm::ArrayRef<uint64_t> args) { return std::move(Err); } } - if (llvm::Error Err = WriteMemory(exe_addr, syscall_data.Insn.data(), - syscall_data.Insn.size(), bytes_read) + if (llvm::Error Err = DoWriteMemory(exe_addr, syscall_data.Insn.data(), + syscall_data.Insn.size(), bytes_read) .ToError()) return std::move(Err); @@ -1667,8 +1668,8 @@ Status NativeProcessLinux::ReadMemory(const ProcessAddress &process_addr, return Status(); } -Status NativeProcessLinux::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status NativeProcessLinux::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { const unsigned char *src = static_cast<const unsigned char *>(buf); size_t remainder; Status error; @@ -1699,7 +1700,7 @@ Status NativeProcessLinux::WriteMemory(lldb::addr_t addr, const void *buf, memcpy(buff, src, remainder); size_t bytes_written_rec; - error = WriteMemory(addr, buff, k_ptrace_word_size, bytes_written_rec); + error = DoWriteMemory(addr, buff, k_ptrace_word_size, bytes_written_rec); if (error.Fail()) return error; diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h index b45e5ff1546ef..23a0ccc3e4ffd 100644 --- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h +++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.h @@ -99,8 +99,8 @@ class NativeProcessLinux : public NativeProcessELF, Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; llvm::Expected<lldb::addr_t> AllocateMemory(size_t size, uint32_t permissions) override; diff --git a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp index 5b2b80866c98c..a69a33313923a 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp +++ b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp @@ -928,8 +928,8 @@ Status NativeProcessNetBSD::ReadMemory(const ProcessAddress &process_addr, return Status(); } -Status NativeProcessNetBSD::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status NativeProcessNetBSD::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { const unsigned char *src = static_cast<const unsigned char *>(buf); Status error; struct ptrace_io_desc io; diff --git a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h index 599943290c163..ea9f8a219122b 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h +++ b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h @@ -60,8 +60,8 @@ class NativeProcessNetBSD : public NativeProcessELF { Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; lldb::addr_t GetSharedLibraryInfoAddress() override; diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp index fc93a7e5747ed..fc00e3089e750 100644 --- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp @@ -239,8 +239,8 @@ Status NativeProcessWindows::ReadMemory(const ProcessAddress &process_addr, return ProcessDebugger::ReadMemory(addr, buf, size, bytes_read); } -Status NativeProcessWindows::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status NativeProcessWindows::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, size_t &bytes_written) { return ProcessDebugger::WriteMemory(addr, buf, size, bytes_written); } diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h index 7801d6febd28a..3074eea1bc932 100644 --- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h +++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h @@ -71,8 +71,8 @@ class NativeProcessWindows : public NativeProcessProtocol, Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; llvm::Expected<lldb::addr_t> AllocateMemory(size_t size, uint32_t permissions) override; diff --git a/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/Makefile b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/Makefile new file mode 100644 index 0000000000000..10495940055b6 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c + +include Makefile.rules diff --git a/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py new file mode 100644 index 0000000000000..4d33382fe3bd9 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py @@ -0,0 +1,161 @@ +""" +Check that when data is written over a software breakpoint site, it does not +corrupt the breakpoint instruction, and is later written to memory when the +breakpoint is removed. +""" + +import lldb +from lldbsuite.test.lldbtest import * +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * + + +class WriteOverSoftwareBreakpoint(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + # Could not find a way to make place_break_here visible to lldb on Windows. + @skipIfWindows + # debugserver needs fixing, see: + # https://github.com/llvm/llvm-project/issues/217359 + # https://github.com/llvm/llvm-project/issues/217840 + @llgs_test + def test_write_over_breakpoint(self): + TestBase.setUp(self) + self.line = line_number("main.c", "// break here") + self.build() + exe = self.getBuildArtifact("a.out") + self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + + lldbutil.run_break_set_by_file_and_line( + self, "main.c", self.line, num_expected_locations=1, loc_exact=True + ) + self.runCmd("run", RUN_SUCCEEDED) + self.expect( + "thread list", + STOPPED_DUE_TO_BREAKPOINT, + substrs=["stopped", "stop reason = breakpoint"], + ) + + target = self.dbg.GetSelectedTarget() + process = target.GetProcess() + + loop_start_breakpoint_addr = ( + target.breakpoints[0].GetLocationAtIndex(0).GetLoadAddress() + ) + + # Memory operations and breakpoint actions must be sent to the server + # right away instead of waiting for the next continue event. + self.runCmd("settings set target.process.disable-memory-cache on") + # Breakpoints set by address are never delayed but to make this test + # less fragile if later changed, disable delayed breakpoints entirely. + self.runCmd("settings set target.process.use-delayed-breakpoints false") + + # At this point we are stopped at the start of the for loop. + # We will set a further breakpoint in foo, and this is the one we will + # test by writing over it then continuing to it. + # We could use the same breakpoint, but arranging for dead code + # immediately before it is more risky. + + # The breakpoint must be on an exact address, because we do not want + # lldb to adjust it based on the debug information for prologues + # and epilogues. + symbol_contexts = target.FindSymbols("place_break_here") + self.assertEqual(1, len(symbol_contexts)) + label_symbol = symbol_contexts[0].GetSymbol() + self.assertTrue(label_symbol.IsValid()) + bkpt_address = label_symbol.GetStartAddress().GetLoadAddress(target) + + # lldb-server has its algorithm unit tested as part of + # NativeProcessProtocol, but debugserver does not use that. So we will + # do a few diff erent types of overwrite here so we have coverage for both + # debug servers. + # + # We cannot be sure what the software break size will be, so I'm assuming + # 4 bytes because that's what Arm/AArch64 use. This means we may not be + # getting full coverage on Thumb or x86 but the test should still pass + # there. + # + # We assume that the instruction immediately before the breakpoint in + # foo is dead code, so we are allowed to corrupt it. + writes = [ + # Up to but not over breakpoint. + (-4, 4), + # Over start of breakpoint. + (-2, 4), + # Exactly over breakpoint. + (0, 4), + # Over end of breakpoint. + (2, 4), + # Immediately after breakpoint. + (4, 4), + # From before to after breakpoint. + (-4, 12), + ] + + for write_offset, write_size in writes: + # Place a breakpoint immediately after the dead code in foo. + bkpt = target.BreakpointCreateByAddress(bkpt_address) + self.assertTrue(bkpt.IsValid()) + self.assertEqual(bkpt.GetNumLocations(), 1) + self.assertFalse(bkpt.IsHardware()) + self.assertEqual(bkpt.GetLocationAtIndex(0).GetLoadAddress(), bkpt_address) + + check_address = bkpt_address + write_offset + + # Read around the breakpoint site. We assume that read subsitution + # is working, so the data here is the original contents of memory + # without the trap instruction. + err = lldb.SBError() + original_data = bytearray( + process.ReadMemory(check_address, write_size, err) + ) + self.assertSuccess(err) + self.assertEqual(len(original_data), write_size) + + # Write around/in/over the breakpoint site. + write_data = bytearray(range(write_size)) + wrote = process.WriteMemory(check_address, write_data, err) + self.assertSuccess(err) + self.assertEqual(wrote, write_size) + + # The data overlapping the breakpoint site should be in that breakpoint's + # saved data. So it will appear as if all the data was written to memory, + # even though it was not yet. + after_write = bytearray(process.ReadMemory(check_address, write_size, err)) + self.assertSuccess(err) + self.assertEqual(after_write, write_data) + + # The instruction in memory should still be intact so we can continue + # to the breakpoint. + process.Continue() + + thread = process.thread[0] + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint) + # Should be stopped at the breakpoint we placed in foo. This proves that + # the breakpoint instruction was intact. + self.assertEqual( + bkpt_address, + thread.selected_frame.GetPC(), + ) + + # When the breakpoint is removed the saved bytes will be written to + # memory. + self.assertTrue(target.BreakpointDelete(bkpt.GetID())) + + data = process.ReadMemory(check_address, write_size, err) + self.assertSuccess(err) + self.assertEqual(bytearray(data), write_data) + + # Restore the original instruction data. The dead code before the + # breakpoint is ok but the instructions after it must be put back + # so we can continue. + wrote = process.WriteMemory(check_address, original_data, err) + self.assertSuccess(err) + self.assertEqual(wrote, write_size) + + # Continue back to the start of the loop. + process.Continue() + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint) + self.assertEqual(loop_start_breakpoint_addr, thread.selected_frame.GetPC()) diff --git a/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/main.c b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/main.c new file mode 100644 index 0000000000000..4c9100dda604a --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/main.c @@ -0,0 +1,34 @@ +volatile int always_false; + +int foo() { + // This nop is deliberately dead code so that we have an instruction + // immediately prior to the breakpoint that we are allowed to corrupt. + if (always_false) { + // This dead code must be at least 4 bytes even on an architecture where + // nop is 1 byte. + asm volatile("nop\n" + "nop\n" + "nop\n" + "nop\n"); + } + // We are assuming that there are no instructions placed between the dead code + // above and the assembly below. This is not guaranteed but it's safer than + // assuming that this function will have no prologue instructions or breaking + // in the very first instruction of foo and hoping whatever comes before is + // not important. + asm volatile(".globl place_break_here\n" + "place_break_here:\n" + // The test will repeatedly add and remove a breakpoint here. + "nop"); + + return 0; +} + +int main() { + volatile int sum = 0; + // Pick a number of loops >= the number of write patterns being tested. + for (unsigned i = 0; i < 10; ++i) + sum += foo(); // break here + + return 0; +} diff --git a/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.cpp b/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.cpp index 8ee3510e67e0b..f179b9c6b8781 100644 --- a/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.cpp +++ b/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.cpp @@ -66,8 +66,9 @@ Status ProcessMockAccelerator::ReadMemory(const ProcessAddress &process_addr, return Status::FromErrorString("unimplemented"); } -Status ProcessMockAccelerator::WriteMemory(lldb::addr_t addr, const void *buf, - size_t size, size_t &bytes_written) { +Status ProcessMockAccelerator::DoWriteMemory(lldb::addr_t addr, const void *buf, + size_t size, + size_t &bytes_written) { bytes_written = 0; return Status::FromErrorString("unimplemented"); } diff --git a/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.h b/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.h index 6346773e40141..7245e4aeca5d5 100644 --- a/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.h +++ b/lldb/tools/lldb-server/Plugins/Accelerator/Mock/ProcessMockAccelerator.h @@ -42,8 +42,8 @@ class ProcessMockAccelerator : public NativeProcessProtocol { Status ReadMemory(const ProcessAddress &addr, void *buf, size_t size, size_t &bytes_read) override; - Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size, - size_t &bytes_written) override; + Status DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + size_t &bytes_written) override; lldb::addr_t GetSharedLibraryInfoAddress() override; size_t UpdateThreads() override; diff --git a/lldb/unittests/Host/NativeProcessProtocolTest.cpp b/lldb/unittests/Host/NativeProcessProtocolTest.cpp index 91c4fd69d6e54..e5756d1db9f10 100644 --- a/lldb/unittests/Host/NativeProcessProtocolTest.cpp +++ b/lldb/unittests/Host/NativeProcessProtocolTest.cpp @@ -238,3 +238,267 @@ TEST(NativeProcessProtocolTest, ReadCStringFromMemory_CrossPageBoundary) { llvm::HasValue(llvm::StringRef("hello"))); EXPECT_EQ(bytes_read, 6UL); } + +void DoTestWriteMemoryPreservingTrap( + const std::vector<lldb::addr_t> &bp_addrs, + std::optional<lldb::addr_t> write_addr, + const std::vector<uint8_t> &write_data, uint32_t expected_number_of_writes, + const std::vector<uint8_t> expected_after_write_read_memory, + const std::vector<uint8_t> expected_after_write_read_memory_without_trap) { + NiceMock<MockDelegate> DummyDelegate; + MockProcess<NativeProcessProtocol> Process(DummyDelegate, + ArchSpec("aarch64-pc-linux")); + const std::vector<uint8_t> fake_memory{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + FakeMemory M{fake_memory}; + ON_CALL(Process, ReadMemory(_, _)) + .WillByDefault(Invoke(&M, &FakeMemory::Read)); + ON_CALL(Process, WriteMemory(_, _)) + .WillByDefault(Invoke(&M, &FakeMemory::Write)); + + for (auto bp_addr : bp_addrs) + EXPECT_THAT_ERROR( + Process.SetBreakpoint(bp_addr, 0, /*hardware=*/false).ToError(), + llvm::Succeeded()); + + if (expected_number_of_writes) + EXPECT_CALL(Process, WriteMemory(_, _)) + .Times(expected_number_of_writes) + .WillRepeatedly(DoDefault()); + else + EXPECT_CALL(Process, WriteMemory(_, _)).Times(0); + + if (write_addr) { + size_t bytes_written = 0; + Status err = Process.WriteMemory(*write_addr, write_data.data(), + write_data.size(), bytes_written); + EXPECT_THAT_ERROR(err.ToError(), llvm::Succeeded()); + EXPECT_EQ(bytes_written, write_data.size()); + } + + Mock::VerifyAndClearExpectations(&Process); + + // Anything written over a breakpoint should go into the saved bytes instead + // of into memory. + + // ReadMemory should show that the breakpoint instructions are unchanged. + auto memory_or_err = Process.ReadMemory(0, fake_memory.size()); + EXPECT_THAT_EXPECTED(memory_or_err, llvm::Succeeded()); + EXPECT_EQ(*memory_or_err, expected_after_write_read_memory); + + // ReadMemoryWithoutTrap should show that writes to the breakpoints have + // updated the saved data in the breakpoint. + memory_or_err = Process.ReadMemoryWithoutTrap(0, fake_memory.size()); + EXPECT_THAT_EXPECTED(memory_or_err, llvm::Succeeded()); + EXPECT_EQ(*memory_or_err, expected_after_write_read_memory_without_trap); + + // When the breakpoint is removed, the saved bytes are actually written + // to memory. + for (auto bp_addr : bp_addrs) + EXPECT_THAT_ERROR(Process.RemoveBreakpoint(bp_addr, false).ToError(), + llvm::Succeeded()); + + // The memory should contain the written data now. + memory_or_err = Process.ReadMemory(0, fake_memory.size()); + EXPECT_THAT_EXPECTED(memory_or_err, llvm::Succeeded()); + EXPECT_EQ(*memory_or_err, expected_after_write_read_memory_without_trap); + + // As there are no breakpoints, the result of ReadMemoryWithoutTrap should + // be the same. + memory_or_err = Process.ReadMemoryWithoutTrap(0, fake_memory.size()); + EXPECT_THAT_EXPECTED(memory_or_err, llvm::Succeeded()); + EXPECT_EQ(*memory_or_err, expected_after_write_read_memory_without_trap); +} + +void TestWriteMemoryPreservingTrap( + const std::vector<lldb::addr_t> &bp_addrs, + std::optional<lldb::addr_t> write_addr, + const std::vector<uint8_t> &write_data, uint32_t expected_number_of_writes, + const std::vector<uint8_t> expected_after_write_read_memory, + const std::vector<uint8_t> expected_after_write_read_memory_without_trap) { + auto bp_addrs_in_order = bp_addrs; + DoTestWriteMemoryPreservingTrap( + bp_addrs_in_order, write_addr, write_data, expected_number_of_writes, + expected_after_write_read_memory, + expected_after_write_read_memory_without_trap); + + // WriteMemoryPreservingTrap should not care in what order the breakpoints + // were inserted. + if (bp_addrs_in_order.size()) { + std::reverse(bp_addrs_in_order.begin(), bp_addrs_in_order.end()); + DoTestWriteMemoryPreservingTrap( + bp_addrs_in_order, write_addr, write_data, expected_number_of_writes, + expected_after_write_read_memory, + expected_after_write_read_memory_without_trap); + } +} + +TEST(NativeProcessProtocolTest, WriteMemoryPreservingTrap) { +// Software breakpoint instruction encoding for AArch64. +#define S__W__B__P 0x0, 0x0, 0x20, 0xd4 + + // In these tests, numbers are used for the initial memory contents and + // letters for the data being written. These variables are used for letters + // so that the inputs can be vertically aligned. + uint8_t a = 'a'; + uint8_t b = 'b'; + uint8_t c = 'c'; + uint8_t d = 'd'; + uint8_t e = 'e'; + uint8_t f = 'f'; + uint8_t g = 'g'; + uint8_t h = 'h'; + uint8_t i = 'i'; + uint8_t j = 'j'; + uint8_t k = 'k'; + + // Write nothing, set no breakpoints, nothing changes. + TestWriteMemoryPreservingTrap({}, std::nullopt, {}, 0, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // Write nothing, set a breakpoint. Breakpoint encoding should be visible in + // memory. + TestWriteMemoryPreservingTrap({0}, std::nullopt, {}, 0, + {S__W__B__P, 4, 5, 6, 7, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // 0 size write with no breakpoints set. + TestWriteMemoryPreservingTrap({}, 0, {}, 0, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // 0 size write not within a breakpoint. + TestWriteMemoryPreservingTrap({4}, 0, {}, 0, + {0, 1, 2, 3, S__W__B__P, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // 0 size write at start of a breakpoint. + TestWriteMemoryPreservingTrap({4}, 4, {}, 0, + {0, 1, 2, 3, S__W__B__P, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // 0 size write at end of a breakpoint. + TestWriteMemoryPreservingTrap({4}, 7, {}, 0, + {0, 1, 2, 3, S__W__B__P, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // 0 size write one beyond the end of a breakpoint. + TestWriteMemoryPreservingTrap({4}, 8, {}, 0, + {0, 1, 2, 3, S__W__B__P, 8, 9, 10}, + {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); + + // Write something but set no breakpoints. Data is written directly to memory. + TestWriteMemoryPreservingTrap({}, 2, {a, b}, 1, + {0, 1, a, b, 4, 5, 6, 7, 8, 9, 10}, + {0, 1, a, b, 4, 5, 6, 7, 8, 9, 10}); + + // Write before a breakpoint and do not overlap it. + TestWriteMemoryPreservingTrap({4}, 1, {a, b, c}, 1, + {0, a, b, c, S__W__B__P, 8, 9, 10}, + {0, a, b, c, 4, 5, 6, 7, 8, 9, 10}); + + // Write within a breakpoint, without writing outside of it. Data kept in + // saved bytes until removal. + TestWriteMemoryPreservingTrap({4}, 4, {a, b, c, d}, 0, + {0, 1, 2, 3, S__W__B__P, 8, 9, 10}, + {0, 1, 2, 3, a, b, c, d, 8, 9, 10}); + + // Write immediately after a breakpoint. Data is written directly to memory. + TestWriteMemoryPreservingTrap({2}, 6, {a, b, c}, 1, + {0, 1, S__W__B__P, a, b, c, 9, 10}, + {0, 1, 2, 3, 4, 5, a, b, c, 9, 10}); + + // Write a range overlapping the beginning of a breakpoint. First part will + // go direct to memory, the other to the saved bytes. + TestWriteMemoryPreservingTrap({2}, 0, {a, b, c, d}, 1, + {a, b, S__W__B__P, 6, 7, 8, 9, 10}, + {a, b, c, d, 4, 5, 6, 7, 8, 9, 10}); + + // Write overlapping end of breakpoint. First part goes to saved bytes, + // second part direct to memory. + TestWriteMemoryPreservingTrap({3}, 5, {a, b, c, d}, 1, + {0, 1, 2, S__W__B__P, c, d, 9, 10}, + {0, 1, 2, 3, 4, a, b, c, d, 9, 10}); + + // Write from before to after break. Ends go to memory, middle to saved bytes. + TestWriteMemoryPreservingTrap({2}, 0, {a, b, c, d, e, f, g, h}, 2, + {a, b, S__W__B__P, g, h, 8, 9, 10}, + {a, b, c, d, e, f, g, h, 8, 9, 10}); + + // Overlap a breakpoint at the very start of memory. + TestWriteMemoryPreservingTrap({0}, 0, {a, b, c, d, e, f}, 1, + {S__W__B__P, e, f, 6, 7, 8, 9, 10}, + {a, b, c, d, e, f, 6, 7, 8, 9, 10}); + + // Overlap a breakpoint at the very end of memory. + TestWriteMemoryPreservingTrap({7}, 5, {a, b, c, d, e, f}, 1, + {0, 1, 2, 3, 4, a, b, S__W__B__P}, + {0, 1, 2, 3, 4, a, b, c, d, e, f}); + + // Write up to a breakpoint. + TestWriteMemoryPreservingTrap({5}, 1, {a, b, c, d}, 1, + {0, a, b, c, d, S__W__B__P, 9, 10}, + {0, a, b, c, d, 5, 6, 7, 8, 9, 10}); + + // Write starting immediately after a breakpoint. + TestWriteMemoryPreservingTrap({2}, 6, {a, b, c, d}, 1, + {0, 1, S__W__B__P, a, b, c, d, 10}, + {0, 1, 2, 3, 4, 5, a, b, c, d, 10}); + + // Overlap 2 breakpoints, write extends before and after them. + TestWriteMemoryPreservingTrap({1, 6}, 0, {a, b, c, d, e, f, g, h, i, j, k}, 3, + {a, S__W__B__P, f, S__W__B__P, k}, + {a, b, c, d, e, f, g, h, i, j, k}); + + // Write starts within the first one, and ends after the second one. + TestWriteMemoryPreservingTrap({1, 6}, 2, {a, b, c, d, e, f, g, h, i}, 2, + {0, S__W__B__P, d, S__W__B__P, i}, + {0, 1, a, b, c, d, e, f, g, h, i}); + + // Write range from before first breakpoint to within second breakpoint. + TestWriteMemoryPreservingTrap({1, 6}, 0, {a, b, c, d, e, f, g, h, i}, 2, + {a, S__W__B__P, f, S__W__B__P, 10}, + {a, b, c, d, e, f, g, h, i, 9, 10}); + + // Write range from within first to beyond second. + TestWriteMemoryPreservingTrap({1, 6}, 2, {a, b, c, d, e, f, g, h, i}, 2, + {0, S__W__B__P, d, S__W__B__P, i}, + {0, 1, a, b, c, d, e, f, g, h, i}); + + // Write range from within first to within second. + TestWriteMemoryPreservingTrap({1, 6}, 2, {a, b, c, d, e, f, g}, 1, + {0, S__W__B__P, d, S__W__B__P, 10}, + {0, 1, a, b, c, d, e, f, g, 9, 10}); + + // Write in range between 2 breakpoints. + TestWriteMemoryPreservingTrap({0, 7}, 4, {a, b, c}, 1, + {S__W__B__P, a, b, c, S__W__B__P}, + {0, 1, 2, 3, a, b, c, 7, 8, 9, 10}); + + // 2 breakpoints with no gap between them, write overlaps the first one. + TestWriteMemoryPreservingTrap({2, 6}, 0, {a, b, c, d}, 1, + {a, b, S__W__B__P, S__W__B__P, 10}, + {a, b, c, d, 4, 5, 6, 7, 8, 9, 10}); + + // 2 breakpoints with no gap between them, write is within the first one. + TestWriteMemoryPreservingTrap({2, 6}, 3, {a, b}, 0, + {0, 1, S__W__B__P, S__W__B__P, 10}, + {0, 1, 2, a, b, 5, 6, 7, 8, 9, 10}); + + // 2 breakpoints with no gap between them, write is across both. + TestWriteMemoryPreservingTrap({2, 6}, 4, {a, b, c, d}, 0, + {0, 1, S__W__B__P, S__W__B__P, 10}, + {0, 1, 2, 3, a, b, c, d, 8, 9, 10}); + + // 2 breakpoints with no gap between them, write is within second one. + TestWriteMemoryPreservingTrap({2, 6}, 7, {a, b}, 0, + {0, 1, S__W__B__P, S__W__B__P, 10}, + {0, 1, 2, 3, 4, 5, 6, a, b, 9, 10}); + + // 2 breakpoints with no gap between them, write overlaps second one. + TestWriteMemoryPreservingTrap({2, 6}, 8, {a, b, c}, 1, + {0, 1, S__W__B__P, S__W__B__P, c}, + {0, 1, 2, 3, 4, 5, 6, 7, a, b, c}); + +#undef S__W__B__P +} diff --git a/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h b/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h index 087a6e7f92913..cbfebc292d867 100644 --- a/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h +++ b/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h @@ -84,10 +84,20 @@ template <typename T> class MockProcess : public T { return Status(); } - Status WriteMemory(addr_t Addr, const void *Buf, size_t Size, - size_t &BytesWritten) /*override*/ { + using T::WriteMemory; + + MOCK_METHOD2(ReadMemory, + llvm::Expected<std::vector<uint8_t>>(addr_t Addr, size_t Size)); + MOCK_METHOD2(WriteMemory, + llvm::Expected<size_t>(addr_t Addr, + llvm::ArrayRef<uint8_t> Data)); + + Status DoWriteMemory(addr_t Addr, const void *Buf, size_t Size, + size_t &BytesWritten) /*override*/ { auto ExpectedBytes = this->WriteMemory( - Addr, llvm::ArrayRef(static_cast<const uint8_t *>(Buf), Size)); + Addr, + llvm::ArrayRef(const_cast<uint8_t *>(static_cast<const uint8_t *>(Buf)), + Size)); if (!ExpectedBytes) { BytesWritten = 0; return Status::FromError(ExpectedBytes.takeError()); @@ -96,12 +106,6 @@ template <typename T> class MockProcess : public T { return Status(); } - MOCK_METHOD2(ReadMemory, - llvm::Expected<std::vector<uint8_t>>(addr_t Addr, size_t Size)); - MOCK_METHOD2(WriteMemory, - llvm::Expected<size_t>(addr_t Addr, - llvm::ArrayRef<uint8_t> Data)); - using T::GetSoftwareBreakpointTrapOpcode; llvm::Expected<std::vector<uint8_t>> ReadMemoryWithoutTrap(addr_t Addr, size_t Size) { _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
