llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> This reapplies #<!-- -->195858 with a fix for 32-bit arm (and generally, any architecture that uses software single-stepping). The problem was that the temporary breakpoints used for single-stepping were interfering with the breakpoints set by the client. The fix is to check for existing breakpoints before setting the temporary ones. To achieve this, I've separated the notion of "next PC candidates for a thread" from "step breakpoints we've actually set". The freebsd code had some software single stepping code, but: - this was [introduced](https://reviews.llvm.org/D95802) for mips64 support, which was [removed](https://github.com/llvm/llvm-project/pull/179582) earlier this year - AFAICT, this never worked since the original patch only checked `m_threads_stepping_with_breakpoint`, but never set it to anything. This is why I'm removing the remnants of the single step support instead of trying to adapt it. The original commit message was: We did not say so explictly, but I'd argue that via https://github.com/llvm/llvm-project/pull/195815, we are supporting stubs which do not refcount breakpoints. In these stubs the set/clear breakpoint packets are idempotent: - setting a breakpoint for the second time is a no-op (returns OK) - clearing a breakpoint clears it, regardless of how many times it has been set - clearing a non-existent breakpoint (either because it was already cleared, or because it was never set) returns an error This makes lldb-server one of those stubs, which makes the code slightly simpler, but more importantly, ensures we do not regress this behavior. --- Full diff: https://github.com/llvm/llvm-project/pull/196891.diff 8 Files Affected: - (modified) lldb/include/lldb/Host/common/NativeProcessProtocol.h (+4-1) - (modified) lldb/source/Host/common/NativeProcessProtocol.cpp (+4-10) - (modified) lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp (+1-16) - (modified) lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h (+1-2) - (modified) lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp (+6-7) - (modified) lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp (+17-21) - (modified) lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h (+5-2) - (modified) lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py (+11-2) ``````````diff diff --git a/lldb/include/lldb/Host/common/NativeProcessProtocol.h b/lldb/include/lldb/Host/common/NativeProcessProtocol.h index 06b36c2cc9eb5..b9c0120016a2d 100644 --- a/lldb/include/lldb/Host/common/NativeProcessProtocol.h +++ b/lldb/include/lldb/Host/common/NativeProcessProtocol.h @@ -158,6 +158,10 @@ class NativeProcessProtocol { virtual Status RemoveBreakpoint(lldb::addr_t addr, bool hardware = false); + bool HasSoftwareBreakpoint(lldb::addr_t addr) { + return m_software_breakpoints.find(addr) != m_software_breakpoints.end(); + } + // Hardware Breakpoint functions virtual const HardwareBreakpointMap &GetHardwareBreakpointMap() const; @@ -419,7 +423,6 @@ class NativeProcessProtocol { protected: struct SoftwareBreakpoint { - uint32_t ref_count; llvm::SmallVector<uint8_t, 4> saved_opcodes; llvm::ArrayRef<uint8_t> breakpoint_opcodes; }; diff --git a/lldb/source/Host/common/NativeProcessProtocol.cpp b/lldb/source/Host/common/NativeProcessProtocol.cpp index 196f54b93538d..dbffdc619ef42 100644 --- a/lldb/source/Host/common/NativeProcessProtocol.cpp +++ b/lldb/source/Host/common/NativeProcessProtocol.cpp @@ -344,10 +344,8 @@ Status NativeProcessProtocol::SetSoftwareBreakpoint(lldb::addr_t 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; + if (it != m_software_breakpoints.end()) return Status(); - } auto expected_bkpt = EnableSoftwareBreakpoint(addr, size_hint); if (!expected_bkpt) return Status::FromError(expected_bkpt.takeError()); @@ -362,14 +360,10 @@ Status NativeProcessProtocol::RemoveSoftwareBreakpoint(lldb::addr_t addr) { auto it = m_software_breakpoints.find(addr); if (it == m_software_breakpoints.end()) return Status::FromErrorString("Breakpoint not found."); - assert(it->second.ref_count > 0); - if (--it->second.ref_count > 0) - return Status(); // Remove the entry from m_software_breakpoints rightaway, so that we don't - // leave behind an entry with ref_count == 0 in case one of the following - // conditions returns an error. The breakpoint is moved so that it can be - // accessed below. + // leave behind an entry in case one of the following conditions returns an + // error. The breakpoint is moved so that it can be accessed below. SoftwareBreakpoint bkpt = std::move(it->second); m_software_breakpoints.erase(it); @@ -503,7 +497,7 @@ NativeProcessProtocol::EnableSoftwareBreakpoint(lldb::addr_t addr, } LLDB_LOG(log, "addr = {0:x}: SUCCESS", addr); - return SoftwareBreakpoint{1, saved_opcode_bytes, *expected_trap}; + return SoftwareBreakpoint{saved_opcode_bytes, *expected_trap}; } llvm::Expected<llvm::ArrayRef<uint8_t>> diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp index 46e9ac1cfd6fa..4853ab2827d9e 100644 --- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp +++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp @@ -316,22 +316,7 @@ void NativeProcessFreeBSD::MonitorSIGTRAP(lldb::pid_t pid) { info.pl_siginfo.si_addr); if (thread) { - auto ®ctx = static_cast<NativeRegisterContextFreeBSD &>( - thread->GetRegisterContext()); - auto thread_info = - m_threads_stepping_with_breakpoint.find(thread->GetID()); - if (thread_info != m_threads_stepping_with_breakpoint.end() && - llvm::is_contained(thread_info->second, regctx.GetPC())) { - thread->SetStoppedByTrace(); - for (auto &&bp_addr : thread_info->second) { - Status brkpt_error = RemoveBreakpoint(bp_addr); - if (brkpt_error.Fail()) - LLDB_LOG(log, "pid = {0} remove stepping breakpoint: {1}", - thread_info->first, brkpt_error); - } - m_threads_stepping_with_breakpoint.erase(thread_info); - } else - thread->SetStoppedByBreakpoint(); + thread->SetStoppedByBreakpoint(); FixupBreakpointPCAsNeeded(*thread); SetCurrentThreadID(thread->GetID()); } diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h index 4a3da9e987e3c..7e8bdc527f420 100644 --- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h +++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h @@ -27,8 +27,7 @@ namespace process_freebsd { /// for debugging. /// /// Changes in the inferior process state are broadcasted. -class NativeProcessFreeBSD : public NativeProcessELF, - private NativeProcessSoftwareSingleStep { +class NativeProcessFreeBSD : public NativeProcessELF { public: class Manager : public NativeProcessProtocol::Manager { public: diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp index 1ad57bd0c19e1..80f1b5662ba61 100644 --- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp +++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp @@ -1931,14 +1931,13 @@ void NativeProcessLinux::SignalIfAllThreadsStopped() { // Clear any temporary breakpoints we used to implement software single // stepping. - for (const auto &thread_info : m_threads_stepping_with_breakpoint) { - for (auto &&bp_addr : thread_info.second) { - Status error = RemoveBreakpoint(bp_addr); - if (error.Fail()) - LLDB_LOG(log, "pid = {0} remove stepping breakpoint: {1}", - thread_info.first, error); - } + for (addr_t bp_addr : m_step_breakpoints) { + Status error = RemoveBreakpoint(bp_addr); + if (error.Fail()) + LLDB_LOG(log, "pid = {0} remove stepping breakpoint: {1}", bp_addr, + error); } + m_step_breakpoints.clear(); m_threads_stepping_with_breakpoint.clear(); // Notify the delegate about the stop diff --git a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp index eddf4b97babae..7069e4f8afaba 100644 --- a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp +++ b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp @@ -87,21 +87,6 @@ static size_t WriteMemoryCallback(EmulateInstruction *instruction, void *baton, return length; } -static Status SetSoftwareBreakpoint(lldb::addr_t bp_addr, unsigned bp_size, - NativeProcessProtocol &process) { - Status error; - error = process.SetBreakpoint(bp_addr, bp_size, /*hardware=*/false); - - // If setting the breakpoint fails because pc is out of the address - // space, ignore it and let the debugee segfault. - if (error.GetError() == EIO || error.GetError() == EFAULT) - return Status(); - if (error.Fail()) - return error; - - return Status(); -} - Status NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping( NativeThreadProtocol &thread) { Status error; @@ -122,24 +107,35 @@ Status NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping( emulator_up->SetWriteMemCallback(&WriteMemoryCallback); emulator_up->SetWriteRegCallback(&WriteRegisterCallback); - auto bp_locaions_predictor = + auto bp_locations_predictor = EmulateInstruction::CreateBreakpointLocationPredictor( std::move(emulator_up)); - auto bp_locations = bp_locaions_predictor->GetBreakpointLocations(error); + BreakpointLocations candidates = + bp_locations_predictor->GetBreakpointLocations(error); if (error.Fail()) return error; - for (auto &&bp_addr : bp_locations) { - auto bp_size = bp_locaions_predictor->GetBreakpointSize(bp_addr); + for (addr_t bp_addr : candidates) { + if (process.HasSoftwareBreakpoint(bp_addr)) + continue; + auto bp_size = bp_locations_predictor->GetBreakpointSize(bp_addr); if (auto err = bp_size.takeError()) return Status(toString(std::move(err))); - error = SetSoftwareBreakpoint(bp_addr, *bp_size, process); + error = process.SetBreakpoint(bp_addr, *bp_size, /*hardware=*/false); + + // If setting the breakpoint fails because pc is out of the address + // space, ignore it and let the debugee segfault. + if (error.GetError() == EIO || error.GetError() == EFAULT) + continue; if (error.Fail()) return error; + + m_step_breakpoints.emplace(bp_addr); } - m_threads_stepping_with_breakpoint.insert({thread.GetID(), bp_locations}); + m_threads_stepping_with_breakpoint.emplace(thread.GetID(), + std::move(candidates)); return error; } diff --git a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h index 4e3fca30684fa..0e4d7c5656bb5 100644 --- a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h +++ b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h @@ -11,8 +11,8 @@ #include "lldb/Host/common/NativeProcessProtocol.h" #include "lldb/Host/common/NativeThreadProtocol.h" - #include <map> +#include <set> namespace lldb_private { @@ -22,9 +22,12 @@ class NativeProcessSoftwareSingleStep { protected: // List of thread ids stepping with a breakpoint with the address of - // the relevan breakpoint + // next PC candidates. std::map<lldb::tid_t, std::vector<lldb::addr_t>> m_threads_stepping_with_breakpoint; + + // The list of stepping breakpoints. + std::set<lldb::addr_t> m_step_breakpoints; }; } // namespace lldb_private diff --git a/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py b/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py index eb9e2952d5a49..a4d6351e05d65 100644 --- a/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py +++ b/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py @@ -17,6 +17,8 @@ # Runs on systems where we can always predict the software break size @skipIf(archs=no_match(["x86_64", "arm64", "aarch64"])) class TestMultiBreakpoint(TestBase): + NO_DEBUG_INFO_TESTCASE = True + def check_invalid_packet(self, packet_str): reply = lldbutil.send_packet_get_reply(self, packet_str) if reply.startswith("E"): @@ -60,6 +62,9 @@ def get_function_address(self, name): return f"{addr:x}" def test_multi_breakpoint(self): + # Debugserver uses refcounted breakpoints + breakpoints_are_refcounted = self.platformIsDarwin() + self.build() source_file = lldb.SBFileSpec("main.c") self.target, process, thread, bkpt = lldbutil.run_to_source_breakpoint( @@ -151,7 +156,9 @@ def make_packet(array): # Clean up both. array = [f"z0,{addr_a},{bp_kind}", f"z0,{addr_a},{bp_kind}"] reply = self.send_packet(make_packet(array)) - self.assertMultiResponse(reply, ["OK", "OK"]) + self.assertMultiResponse( + reply, ["OK", "OK" if breakpoints_are_refcounted else "error"] + ) # --- Set the same breakpoint twice, but remove it thrice. array = [f"Z0,{addr_a},{bp_kind}", f"Z0,{addr_a},{bp_kind}"] @@ -163,7 +170,9 @@ def make_packet(array): f"z0,{addr_a},{bp_kind}", ] reply = self.send_packet(make_packet(array)) - self.assertMultiResponse(reply, ["OK", "OK", "error"]) + self.assertMultiResponse( + reply, ["OK", "OK" if breakpoints_are_refcounted else "error", "error"] + ) # --- Set and remove the same address in a single packet --- # The spec requires requests to be executed in order, so the set `````````` </details> https://github.com/llvm/llvm-project/pull/196891 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
