https://github.com/DuncanMcBain updated https://github.com/llvm/llvm-project/pull/174348
>From 8b869da43c65e2690bd15277c4a8d307983035be Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Wed, 3 Dec 2025 17:02:11 +0000 Subject: [PATCH 1/8] [lldb] Step over non-lldb breakpoints Several languages support some sort of "breakpoint" function, which adds ISA-specific instructions to generate an interrupt at runtime. However, on some platforms, these instructions don't increment the program counter. When LLDB sets these instructions it isn't a problem, as we remove them before continuing, then re-add them after stepping over the location. However, for breakpoint sequences that are part of the inferior process, this doesn't happen - and so users might be left unable to continue past the breakpoint without manually interfering with the program counter. This patch adds logic to LLDB to intercept SIGTRAPs, inspect the bytes of the inferior at the program counter, and if the instruction looks like a BRK or BKPT or similar, increment the pc by the size of the instruction we found. This unifies platform behaviour (e.g. on x86_64, LLDB debug sessions already look like this) and improves UX (in my opinion, but I think it beats messing with stuff every break). Some ISAs (like AArch64) require slightly different handling, as while there are multiple possible instructions, we should be careful only to find the ones likely to have been emitted by a compiler backend, and not those inserted from (for example) the UB sanitizer, or any others. There is an existing builtin-debugtrap test which was under the macos folder before. I've now moved that to "functionalities", made it pure C only, and updated it a little bit so that it works regardless of platform. --- lldb/include/lldb/Core/Architecture.h | 12 ++ lldb/include/lldb/Target/Platform.h | 11 ++ .../AArch64/ArchitectureAArch64.cpp | 17 +++ .../AArch64/ArchitectureAArch64.h | 4 + .../Architecture/Arm/ArchitectureArm.cpp | 30 +++++ .../Architecture/Arm/ArchitectureArm.h | 4 + .../Architecture/Mips/ArchitectureMips.cpp | 11 ++ .../Architecture/Mips/ArchitectureMips.h | 4 + lldb/source/Target/Platform.cpp | 126 +++++++++--------- lldb/source/Target/StopInfo.cpp | 34 +++++ .../builtin-debugtrap/Makefile | 2 +- .../builtin-debugtrap/TestBuiltinDebugTrap.py | 23 ++-- .../builtin-debugtrap/main.c} | 3 +- 13 files changed, 206 insertions(+), 75 deletions(-) rename lldb/test/API/{macosx => functionalities}/builtin-debugtrap/Makefile (50%) rename lldb/test/API/{macosx => functionalities}/builtin-debugtrap/TestBuiltinDebugTrap.py (74%) rename lldb/test/API/{macosx/builtin-debugtrap/main.cpp => functionalities/builtin-debugtrap/main.c} (93%) diff --git a/lldb/include/lldb/Core/Architecture.h b/lldb/include/lldb/Core/Architecture.h index ed64a895717a1..aaff8deb3582a 100644 --- a/lldb/include/lldb/Core/Architecture.h +++ b/lldb/include/lldb/Core/Architecture.h @@ -138,6 +138,18 @@ class Architecture : public PluginInterface { std::shared_ptr<const UnwindPlan> current_unwindplan) { return lldb::UnwindPlanSP(); } + + /// Returns whether a given byte sequence is a valid breakpoint for the + /// architecture. Some architectures have breakpoint instructions that + /// have immediates that can take on any value, resulting in a family + /// of valid byte sequences. Bases the size comparison on the reference. + virtual bool + IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const { + if (reference.size() > observed.size()) + return false; + return !std::memcmp(reference.data(), observed.data(), reference.size()); + } }; } // namespace lldb_private diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index 637d4c37b90bc..bd1fdb9011569 100644 --- a/lldb/include/lldb/Target/Platform.h +++ b/lldb/include/lldb/Target/Platform.h @@ -330,6 +330,17 @@ class Platform : public PluginInterface { virtual std::vector<ArchSpec> GetSupportedArchitectures(const ArchSpec &process_host_arch) = 0; + /// Get the bytes of the platform's software interrupt instruction. + /// + /// \param[in] arch + /// The architecture of the inferior + /// \param size_hint + /// A hint to disambiguate which instruction is used on platforms where + /// there are multiple interrupts with different sizes in the ISA (e.g + /// ARM Thumb, RISC-V) + llvm::ArrayRef<uint8_t> SoftwareTrapOpcodeTable(const ArchSpec &arch, + size_t size_hint = 0); + virtual size_t GetSoftwareBreakpointTrapOpcode(Target &target, BreakpointSite *bp_site); diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp index 6a072354972ac..9dca0d9e3a265 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp @@ -13,6 +13,8 @@ #include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/DataExtractor.h" +#include "llvm/Support/Endian.h" + using namespace lldb_private; using namespace lldb; @@ -141,3 +143,18 @@ bool ArchitectureAArch64::ReconfigureRegisterInfo(DynamicRegisterInfo ®_info, return true; } + +bool ArchitectureAArch64::IsValidBreakpointInstruction( + llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { + if (reference.size() < 4 || observed.size() < 4) + return false; + auto ref_bytes = llvm::support::endian::read32le(reference.data()); + auto bytes = llvm::support::endian::read32le(observed.data()); + // Only the 11 highest bits define the breakpoint, the others include an + // immediate which is stored to a CPU register. + uint32_t mask = 0xFFE00000; + // Check that the masked bytes match the reference, but also check that the + // immediate in the instruction is the default output by llvm.debugtrap + // The reference has the immediate set as all-zero, so mask and check here + return (ref_bytes == (bytes & mask)) && ((bytes & ~mask) >> 5 == 0xF000); +} diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h index ba409428c951e..502a55ba23a91 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h @@ -39,6 +39,10 @@ class ArchitectureAArch64 : public Architecture { DataExtractor ®_data, RegisterContext ®_context) const override; + bool + IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; + private: static std::unique_ptr<Architecture> Create(const ArchSpec &arch); ArchitectureAArch64() = default; diff --git a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp index 721c4bcfe6342..138d28b50988b 100644 --- a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp +++ b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp @@ -22,6 +22,8 @@ #include "lldb/Utility/Log.h" #include "lldb/Utility/RegisterValue.h" +#include "llvm/Support/Endian.h" + using namespace lldb_private; using namespace lldb; @@ -336,3 +338,31 @@ UnwindPlanSP ArchitectureArm::GetArchitectureUnwindPlan( } return new_plan; } + +bool ArchitectureArm::IsValidBreakpointInstruction( + llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { + auto is_bkpt = false; + if (observed.size() < reference.size()) + return false; + if (reference.size() == 2) { + auto ref_bytes = llvm::support::endian::read16le(reference.data()); + auto obs_bytes = llvm::support::endian::read16le(observed.data()); + is_bkpt = ref_bytes == obs_bytes; + // LLDB uses an undef instruction encoding for breakpoints + // perhaps we have an actual BKPT in the inferior + if (!is_bkpt) { + uint16_t mask = 0xF0; + is_bkpt = (obs_bytes & mask) == 0xBE00; + } + } else if (reference.size() == 4) { + auto ref_bytes = llvm::support::endian::read32le(reference.data()); + auto obs_bytes = llvm::support::endian::read32le(observed.data()); + is_bkpt = ref_bytes == obs_bytes; + if (!is_bkpt) { + uint32_t mask = 0x0FF000F0; + uint32_t bkpt_pattern = 0xE1200070; + is_bkpt = (obs_bytes & mask) == bkpt_pattern; + } + } + return is_bkpt; +} diff --git a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h index 52277dc5dbae0..071f756c96a0a 100644 --- a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h +++ b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h @@ -34,6 +34,10 @@ class ArchitectureArm : public Architecture { lldb_private::Thread &thread, lldb_private::RegisterContextUnwind *regctx, std::shared_ptr<const UnwindPlan> current_unwindplan) override; + bool + IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; + private: static std::unique_ptr<Architecture> Create(const ArchSpec &arch); ArchitectureArm() = default; diff --git a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp index 3748be0533ad7..8d921149bb1b3 100644 --- a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp +++ b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp @@ -19,6 +19,8 @@ #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "llvm/Support/Endian.h" + using namespace lldb_private; using namespace lldb; @@ -229,3 +231,12 @@ Instruction *ArchitectureMips::GetInstructionAtAddress( return nullptr; } + +bool ArchitectureMips::IsValidBreakpointInstruction( + llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { + // The middle twenty bits of BREAK can be anything, so zero them + uint32_t mask = 0xFC00003F; + auto ref_bytes = llvm::support::endian::read32le(reference.data()); + auto bytes = llvm::support::endian::read32le(observed.data()); + return (ref_bytes & mask) == (bytes & mask); +} diff --git a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h index cedd4127afcb6..8d48eef61019f 100644 --- a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h +++ b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h @@ -33,6 +33,10 @@ class ArchitectureMips : public Architecture { lldb::addr_t GetOpcodeLoadAddress(lldb::addr_t load_addr, AddressClass addr_class) const override; + bool + IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; + private: Instruction *GetInstructionAtAddress(Target &target, const Address &resolved_addr, diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index bf9e2f2c2b2f6..c7648911d6e2f 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -39,6 +39,7 @@ #include "lldb/Utility/Log.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/StructuredData.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLExtras.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" @@ -1946,121 +1947,99 @@ size_t Platform::ConnectToWaitingProcesses(lldb_private::Debugger &debugger, return 0; } -size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target, - BreakpointSite *bp_site) { - ArchSpec arch = target.GetArchitecture(); - assert(arch.IsValid()); - const uint8_t *trap_opcode = nullptr; - size_t trap_opcode_size = 0; +llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeTable(const ArchSpec &arch, + size_t size_hint) { + llvm::ArrayRef<uint8_t> trap_opcode; switch (arch.GetMachine()) { case llvm::Triple::aarch64_32: case llvm::Triple::aarch64: { static const uint8_t g_aarch64_opcode[] = {0x00, 0x00, 0x20, 0xd4}; - trap_opcode = g_aarch64_opcode; - trap_opcode_size = sizeof(g_aarch64_opcode); + trap_opcode = + llvm::ArrayRef<uint8_t>(g_aarch64_opcode, sizeof(g_aarch64_opcode)); } break; case llvm::Triple::arc: { - static const uint8_t g_hex_opcode[] = { 0xff, 0x7f }; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + static const uint8_t g_hex_opcode[] = {0xff, 0x7f}; + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; - // TODO: support big-endian arm and thumb trap codes. case llvm::Triple::arm: { - // The ARM reference recommends the use of 0xe7fddefe and 0xdefe but the - // linux kernel does otherwise. + // ARM CPUs have dedicated BKPT instructions: 0xe7fddefe and 0xdefe. + // However, the linux kernel recognizes two different sequences based on + // undefined instruction encodings (linux/arch/arm/kernel/ptrace.c) static const uint8_t g_arm_breakpoint_opcode[] = {0xf0, 0x01, 0xf0, 0xe7}; static const uint8_t g_thumb_breakpoint_opcode[] = {0x01, 0xde}; - lldb::BreakpointLocationSP bp_loc_sp(bp_site->GetConstituentAtIndex(0)); - AddressClass addr_class = AddressClass::eUnknown; - - if (bp_loc_sp) { - addr_class = bp_loc_sp->GetAddress().GetAddressClass(); - if (addr_class == AddressClass::eUnknown && - (bp_loc_sp->GetAddress().GetFileAddress() & 1)) - addr_class = AddressClass::eCodeAlternateISA; - } - - if (addr_class == AddressClass::eCodeAlternateISA) { - trap_opcode = g_thumb_breakpoint_opcode; - trap_opcode_size = sizeof(g_thumb_breakpoint_opcode); + if (size_hint == 2) { + trap_opcode = llvm::ArrayRef<uint8_t>(g_thumb_breakpoint_opcode, + sizeof(g_thumb_breakpoint_opcode)); } else { - trap_opcode = g_arm_breakpoint_opcode; - trap_opcode_size = sizeof(g_arm_breakpoint_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_arm_breakpoint_opcode, + sizeof(g_arm_breakpoint_opcode)); } } break; case llvm::Triple::avr: { static const uint8_t g_hex_opcode[] = {0x98, 0x95}; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; case llvm::Triple::mips: case llvm::Triple::mips64: { static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d}; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; case llvm::Triple::mipsel: case llvm::Triple::mips64el: { static const uint8_t g_hex_opcode[] = {0x0d, 0x00, 0x00, 0x00}; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; case llvm::Triple::msp430: { static const uint8_t g_msp430_opcode[] = {0x43, 0x43}; - trap_opcode = g_msp430_opcode; - trap_opcode_size = sizeof(g_msp430_opcode); + trap_opcode = + llvm::ArrayRef<uint8_t>(g_msp430_opcode, sizeof(g_msp430_opcode)); } break; case llvm::Triple::systemz: { static const uint8_t g_hex_opcode[] = {0x00, 0x01}; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; case llvm::Triple::hexagon: { static const uint8_t g_hex_opcode[] = {0x0c, 0xdb, 0x00, 0x54}; - trap_opcode = g_hex_opcode; - trap_opcode_size = sizeof(g_hex_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_hex_opcode, sizeof(g_hex_opcode)); } break; case llvm::Triple::ppc: case llvm::Triple::ppc64: { static const uint8_t g_ppc_opcode[] = {0x7f, 0xe0, 0x00, 0x08}; - trap_opcode = g_ppc_opcode; - trap_opcode_size = sizeof(g_ppc_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_ppc_opcode, sizeof(g_ppc_opcode)); } break; case llvm::Triple::ppc64le: { static const uint8_t g_ppc64le_opcode[] = {0x08, 0x00, 0xe0, 0x7f}; // trap - trap_opcode = g_ppc64le_opcode; - trap_opcode_size = sizeof(g_ppc64le_opcode); + trap_opcode = + llvm::ArrayRef<uint8_t>(g_ppc64le_opcode, sizeof(g_ppc64le_opcode)); } break; case llvm::Triple::x86: case llvm::Triple::x86_64: { static const uint8_t g_i386_opcode[] = {0xCC}; - trap_opcode = g_i386_opcode; - trap_opcode_size = sizeof(g_i386_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_i386_opcode, sizeof(g_i386_opcode)); } break; case llvm::Triple::riscv32: case llvm::Triple::riscv64: { static const uint8_t g_riscv_opcode[] = {0x73, 0x00, 0x10, 0x00}; // ebreak static const uint8_t g_riscv_opcode_c[] = {0x02, 0x90}; // c.ebreak - if (arch.GetFlags() & ArchSpec::eRISCV_rvc) { + if (size_hint == 2) { trap_opcode = g_riscv_opcode_c; - trap_opcode_size = sizeof(g_riscv_opcode_c); } else { - trap_opcode = g_riscv_opcode; - trap_opcode_size = sizeof(g_riscv_opcode); + trap_opcode = + llvm::ArrayRef<uint8_t>(g_riscv_opcode, sizeof(g_riscv_opcode)); } } break; @@ -2068,24 +2047,47 @@ size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target, case llvm::Triple::loongarch64: { static const uint8_t g_loongarch_opcode[] = {0x05, 0x00, 0x2a, 0x00}; // break 0x5 - trap_opcode = g_loongarch_opcode; - trap_opcode_size = sizeof(g_loongarch_opcode); + trap_opcode = + llvm::ArrayRef<uint8_t>(g_loongarch_opcode, sizeof(g_loongarch_opcode)); } break; - case llvm::Triple::wasm32: { - // Unreachable (0x00) triggers an unconditional trap. + // Unreachable (0x00) triggers an unconditional trap. + case llvm::Triple::wasm32: + // In the case of an unkown platform, return 0x00 too + default: { static const uint8_t g_wasm_opcode[] = {0x00}; - trap_opcode = g_wasm_opcode; - trap_opcode_size = sizeof(g_wasm_opcode); + trap_opcode = llvm::ArrayRef<uint8_t>(g_wasm_opcode, sizeof(g_wasm_opcode)); } break; + } + return trap_opcode; +} - default: - return 0; +size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target, + BreakpointSite *bp_site) { + ArchSpec arch = target.GetArchitecture(); + assert(arch.IsValid()); + AddressClass addr_class = AddressClass::eUnknown; + if (bp_site) { + // TODO: support big-endian arm and thumb trap codes. + lldb::BreakpointLocationSP bp_loc_sp(bp_site->GetConstituentAtIndex(0)); + if (bp_loc_sp) { + addr_class = bp_loc_sp->GetAddress().GetAddressClass(); + if (addr_class == AddressClass::eUnknown && + (bp_loc_sp->GetAddress().GetFileAddress() & 1)) + addr_class = AddressClass::eCodeAlternateISA; + } } - assert(bp_site); - if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size)) - return trap_opcode_size; + size_t size_hint = 0; + // Check for either ARM or RISC-V short instruction conditions + if ((addr_class == AddressClass::eCodeAlternateISA) || + (arch.GetFlags() & ArchSpec::eRISCV_rvc)) + size_hint = 2; + auto trap_opcode = SoftwareTrapOpcodeTable(arch, size_hint); + + if (bp_site && + bp_site->SetTrapOpcode(trap_opcode.begin(), trap_opcode.size())) + return trap_opcode.size(); return 0; } diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index e9e534a57973e..97d5276634a79 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -1159,6 +1159,40 @@ class StopInfoUnixSignal : public StopInfo { return false; } + void PerformAction(Event *event_ptr) override { + // A signal of SIGTRAP indicates that a break instruction has been hit + if (m_value == SIGTRAP) { + Log *log = GetLog(LLDBLog::Process); + Status error; + std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; + auto reg_ctx_sp = GetThread()->GetRegisterContext(); + auto process_sp = GetThread()->GetProcess(); + addr_t pc = reg_ctx_sp->GetPC(); + if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), + error)) { + // If this fails, we simply don't handle the step-over-break logic and + // log the failure + LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", + pc, error); + return; + } + auto &target = process_sp->GetTarget(); + auto platform_sp = target.GetPlatform(); + auto platform_opcode = + platform_sp->SoftwareTrapOpcodeTable(target.GetArchitecture()); + + if (auto *arch_plugin = target.GetArchitecturePlugin(); + arch_plugin && + arch_plugin->IsValidBreakpointInstruction( + platform_opcode, llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), + bytes_at_pc.size()))) { + LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", + pc + platform_opcode.size()); + reg_ctx_sp->SetPC(pc + platform_opcode.size()); + } + } + } + bool ShouldStop(Event *event_ptr) override { return IsShouldStopSignal(); } // If should stop returns false, check if we should notify of this event diff --git a/lldb/test/API/macosx/builtin-debugtrap/Makefile b/lldb/test/API/functionalities/builtin-debugtrap/Makefile similarity index 50% rename from lldb/test/API/macosx/builtin-debugtrap/Makefile rename to lldb/test/API/functionalities/builtin-debugtrap/Makefile index 99998b20bcb05..10495940055b6 100644 --- a/lldb/test/API/macosx/builtin-debugtrap/Makefile +++ b/lldb/test/API/functionalities/builtin-debugtrap/Makefile @@ -1,3 +1,3 @@ -CXX_SOURCES := main.cpp +C_SOURCES := main.c include Makefile.rules diff --git a/lldb/test/API/macosx/builtin-debugtrap/TestBuiltinDebugTrap.py b/lldb/test/API/functionalities/builtin-debugtrap/TestBuiltinDebugTrap.py similarity index 74% rename from lldb/test/API/macosx/builtin-debugtrap/TestBuiltinDebugTrap.py rename to lldb/test/API/functionalities/builtin-debugtrap/TestBuiltinDebugTrap.py index e66892b7ee87a..9e5a40036de07 100644 --- a/lldb/test/API/macosx/builtin-debugtrap/TestBuiltinDebugTrap.py +++ b/lldb/test/API/functionalities/builtin-debugtrap/TestBuiltinDebugTrap.py @@ -11,16 +11,15 @@ class BuiltinDebugTrapTestCase(TestBase): NO_DEBUG_INFO_TESTCASE = True - # Currently this depends on behavior in debugserver to - # advance the pc past __builtin_trap instructions so that - # continue works. Everyone is in agreement that this - # should be moved up into lldb instead of depending on the - # remote stub rewriting the pc values. - @skipUnlessDarwin def test(self): + platform_stop_reason = lldb.eStopReasonSignal + platform = self.getPlatform() + if platform == "darwin" or platform == "windows": + platform_stop_reason = lldb.eStopReasonException + self.build() (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( - self, "// Set a breakpoint here", lldb.SBFileSpec("main.cpp") + self, "// Set a breakpoint here", lldb.SBFileSpec("main.c") ) # Continue to __builtin_debugtrap() @@ -31,7 +30,7 @@ def test(self): self.runCmd("ta v global") self.assertEqual( - process.GetSelectedThread().GetStopReason(), lldb.eStopReasonException + process.GetSelectedThread().GetStopReason(), platform_stop_reason ) list = target.FindGlobalVariables("global", 1, lldb.eMatchTypeNormal) @@ -49,7 +48,7 @@ def test(self): self.runCmd("ta v global") self.assertEqual( - process.GetSelectedThread().GetStopReason(), lldb.eStopReasonException + process.GetSelectedThread().GetStopReason(), platform_stop_reason ) # "global" is now 10. @@ -63,8 +62,12 @@ def test(self): self.runCmd("bt") self.runCmd("ta v global") + # Some platforms might exit when seeing the trap instruction + if process.GetState() == lldb.eStateExited: + return + self.assertEqual( - process.GetSelectedThread().GetStopReason(), lldb.eStopReasonException + process.GetSelectedThread().GetStopReason(), platform_stop_reason ) # "global" is still 10. diff --git a/lldb/test/API/macosx/builtin-debugtrap/main.cpp b/lldb/test/API/functionalities/builtin-debugtrap/main.c similarity index 93% rename from lldb/test/API/macosx/builtin-debugtrap/main.cpp rename to lldb/test/API/functionalities/builtin-debugtrap/main.c index 84332d800148f..8010be4a0235e 100644 --- a/lldb/test/API/macosx/builtin-debugtrap/main.cpp +++ b/lldb/test/API/functionalities/builtin-debugtrap/main.c @@ -1,7 +1,6 @@ #include <stdio.h> int global = 0; -int main() -{ +int main() { global = 5; // Set a breakpoint here puts(""); __builtin_debugtrap(); >From a92bcc8dc43deeda46d1e9e5e70a9745086c153a Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Mon, 5 Jan 2026 18:39:20 +0000 Subject: [PATCH 2/8] Add review feedback * Function naming * Early exits where possible * comment rewording * reorder if statement condition for consistency * maintain default-case behaviour in breakpoint byte selection * mark parameter unused --- lldb/include/lldb/Core/Architecture.h | 4 +- lldb/include/lldb/Target/Platform.h | 2 +- .../AArch64/ArchitectureAArch64.cpp | 4 +- .../Architecture/Arm/ArchitectureArm.cpp | 5 +- lldb/source/Target/Platform.cpp | 14 +++-- lldb/source/Target/StopInfo.cpp | 62 ++++++++++--------- 6 files changed, 50 insertions(+), 41 deletions(-) diff --git a/lldb/include/lldb/Core/Architecture.h b/lldb/include/lldb/Core/Architecture.h index aaff8deb3582a..9c1381d7621a2 100644 --- a/lldb/include/lldb/Core/Architecture.h +++ b/lldb/include/lldb/Core/Architecture.h @@ -142,7 +142,9 @@ class Architecture : public PluginInterface { /// Returns whether a given byte sequence is a valid breakpoint for the /// architecture. Some architectures have breakpoint instructions that /// have immediates that can take on any value, resulting in a family - /// of valid byte sequences. Bases the size comparison on the reference. + /// of valid byte sequences. If the observed byte sequence is shorter + /// than the reference then they are considered not to match, even if + /// the initial bytes would match. virtual bool IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index bd1fdb9011569..98a2b0257e817 100644 --- a/lldb/include/lldb/Target/Platform.h +++ b/lldb/include/lldb/Target/Platform.h @@ -338,7 +338,7 @@ class Platform : public PluginInterface { /// A hint to disambiguate which instruction is used on platforms where /// there are multiple interrupts with different sizes in the ISA (e.g /// ARM Thumb, RISC-V) - llvm::ArrayRef<uint8_t> SoftwareTrapOpcodeTable(const ArchSpec &arch, + llvm::ArrayRef<uint8_t> SoftwareTrapOpcodeBytes(const ArchSpec &arch, size_t size_hint = 0); virtual size_t GetSoftwareBreakpointTrapOpcode(Target &target, diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp index 9dca0d9e3a265..61e5230ef63ba 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp @@ -150,8 +150,8 @@ bool ArchitectureAArch64::IsValidBreakpointInstruction( return false; auto ref_bytes = llvm::support::endian::read32le(reference.data()); auto bytes = llvm::support::endian::read32le(observed.data()); - // Only the 11 highest bits define the breakpoint, the others include an - // immediate which is stored to a CPU register. + // Only the 11 highest bits define the breakpoint instruction, the others + // include an immediate value that we will explicitly check against. uint32_t mask = 0xFFE00000; // Check that the masked bytes match the reference, but also check that the // immediate in the instruction is the default output by llvm.debugtrap diff --git a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp index 138d28b50988b..909d0d0a82e08 100644 --- a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp +++ b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp @@ -341,9 +341,10 @@ UnwindPlanSP ArchitectureArm::GetArchitectureUnwindPlan( bool ArchitectureArm::IsValidBreakpointInstruction( llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { - auto is_bkpt = false; - if (observed.size() < reference.size()) + if (reference.size() > observed.size()) return false; + + auto is_bkpt = false; if (reference.size() == 2) { auto ref_bytes = llvm::support::endian::read16le(reference.data()); auto obs_bytes = llvm::support::endian::read16le(observed.data()); diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index c7648911d6e2f..47c543f72398a 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -1947,7 +1947,7 @@ size_t Platform::ConnectToWaitingProcesses(lldb_private::Debugger &debugger, return 0; } -llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeTable(const ArchSpec &arch, +llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeBytes(const ArchSpec &arch, size_t size_hint) { llvm::ArrayRef<uint8_t> trap_opcode; @@ -2052,12 +2052,16 @@ llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeTable(const ArchSpec &arch, } break; // Unreachable (0x00) triggers an unconditional trap. - case llvm::Triple::wasm32: - // In the case of an unkown platform, return 0x00 too - default: { + case llvm::Triple::wasm32: { static const uint8_t g_wasm_opcode[] = {0x00}; trap_opcode = llvm::ArrayRef<uint8_t>(g_wasm_opcode, sizeof(g_wasm_opcode)); } break; + // The default case should not match against anything, so return a zero + // size array. + default: { + static const uint8_t g_no_opcode[] = {}; + trap_opcode = llvm::ArrayRef<uint8_t>(g_no_opcode, 0); + }; } return trap_opcode; } @@ -2083,7 +2087,7 @@ size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target, if ((addr_class == AddressClass::eCodeAlternateISA) || (arch.GetFlags() & ArchSpec::eRISCV_rvc)) size_hint = 2; - auto trap_opcode = SoftwareTrapOpcodeTable(arch, size_hint); + auto trap_opcode = SoftwareTrapOpcodeBytes(arch, size_hint); if (bp_site && bp_site->SetTrapOpcode(trap_opcode.begin(), trap_opcode.size())) diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 97d5276634a79..d56ae17bc0aec 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -1159,37 +1159,39 @@ class StopInfoUnixSignal : public StopInfo { return false; } - void PerformAction(Event *event_ptr) override { + void PerformAction(Event [[maybe_unused]] * event_ptr) override { // A signal of SIGTRAP indicates that a break instruction has been hit - if (m_value == SIGTRAP) { - Log *log = GetLog(LLDBLog::Process); - Status error; - std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; - auto reg_ctx_sp = GetThread()->GetRegisterContext(); - auto process_sp = GetThread()->GetProcess(); - addr_t pc = reg_ctx_sp->GetPC(); - if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), - error)) { - // If this fails, we simply don't handle the step-over-break logic and - // log the failure - LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", - pc, error); - return; - } - auto &target = process_sp->GetTarget(); - auto platform_sp = target.GetPlatform(); - auto platform_opcode = - platform_sp->SoftwareTrapOpcodeTable(target.GetArchitecture()); - - if (auto *arch_plugin = target.GetArchitecturePlugin(); - arch_plugin && - arch_plugin->IsValidBreakpointInstruction( - platform_opcode, llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), - bytes_at_pc.size()))) { - LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", - pc + platform_opcode.size()); - reg_ctx_sp->SetPC(pc + platform_opcode.size()); - } + if (m_value != SIGTRAP) + return; + Log *log = GetLog(LLDBLog::Process); + Status error; + // We don't expect to see byte sequences longer than four bytes long for + // any breakpoint instructions known to LLDB. + std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; + auto reg_ctx_sp = GetThread()->GetRegisterContext(); + auto process_sp = GetThread()->GetProcess(); + addr_t pc = reg_ctx_sp->GetPC(); + if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), + error)) { + // If this fails, we simply don't handle the step-over-break logic and + // log the failure + LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", + pc, error); + return; + } + auto &target = process_sp->GetTarget(); + auto platform_sp = target.GetPlatform(); + auto platform_opcode = + platform_sp->SoftwareTrapOpcodeBytes(target.GetArchitecture()); + + if (auto *arch_plugin = target.GetArchitecturePlugin(); + arch_plugin && + arch_plugin->IsValidBreakpointInstruction( + platform_opcode, + llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size()))) { + LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", + pc + platform_opcode.size()); + reg_ctx_sp->SetPC(pc + platform_opcode.size()); } } >From 11a25bcb465b48625f92c463354528beb5f7d0b0 Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Fri, 9 Jan 2026 16:33:39 +0000 Subject: [PATCH 3/8] Rename check valid function Breakpoints are debugger things, traps are the actual instructions --- lldb/include/lldb/Core/Architecture.h | 5 ++--- .../Plugins/Architecture/AArch64/ArchitectureAArch64.cpp | 2 +- .../Plugins/Architecture/AArch64/ArchitectureAArch64.h | 5 ++--- lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp | 2 +- lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h | 5 ++--- lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp | 2 +- lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h | 5 ++--- lldb/source/Target/StopInfo.cpp | 2 +- 8 files changed, 12 insertions(+), 16 deletions(-) diff --git a/lldb/include/lldb/Core/Architecture.h b/lldb/include/lldb/Core/Architecture.h index 9c1381d7621a2..5d312bef85d95 100644 --- a/lldb/include/lldb/Core/Architecture.h +++ b/lldb/include/lldb/Core/Architecture.h @@ -145,9 +145,8 @@ class Architecture : public PluginInterface { /// of valid byte sequences. If the observed byte sequence is shorter /// than the reference then they are considered not to match, even if /// the initial bytes would match. - virtual bool - IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, - llvm::ArrayRef<uint8_t> observed) const { + virtual bool IsValidTrapInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const { if (reference.size() > observed.size()) return false; return !std::memcmp(reference.data(), observed.data(), reference.size()); diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp index 61e5230ef63ba..6ccca2404e3c7 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp @@ -144,7 +144,7 @@ bool ArchitectureAArch64::ReconfigureRegisterInfo(DynamicRegisterInfo ®_info, return true; } -bool ArchitectureAArch64::IsValidBreakpointInstruction( +bool ArchitectureAArch64::IsValidTrapInstruction( llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { if (reference.size() < 4 || observed.size() < 4) return false; diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h index 502a55ba23a91..3bf4b66e28e0a 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.h @@ -39,9 +39,8 @@ class ArchitectureAArch64 : public Architecture { DataExtractor ®_data, RegisterContext ®_context) const override; - bool - IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, - llvm::ArrayRef<uint8_t> observed) const override; + bool IsValidTrapInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; private: static std::unique_ptr<Architecture> Create(const ArchSpec &arch); diff --git a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp index 909d0d0a82e08..6088b9c850653 100644 --- a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp +++ b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.cpp @@ -339,7 +339,7 @@ UnwindPlanSP ArchitectureArm::GetArchitectureUnwindPlan( return new_plan; } -bool ArchitectureArm::IsValidBreakpointInstruction( +bool ArchitectureArm::IsValidTrapInstruction( llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { if (reference.size() > observed.size()) return false; diff --git a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h index 071f756c96a0a..dfd7b7b651767 100644 --- a/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h +++ b/lldb/source/Plugins/Architecture/Arm/ArchitectureArm.h @@ -34,9 +34,8 @@ class ArchitectureArm : public Architecture { lldb_private::Thread &thread, lldb_private::RegisterContextUnwind *regctx, std::shared_ptr<const UnwindPlan> current_unwindplan) override; - bool - IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, - llvm::ArrayRef<uint8_t> observed) const override; + bool IsValidTrapInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; private: static std::unique_ptr<Architecture> Create(const ArchSpec &arch); diff --git a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp index 8d921149bb1b3..f4c7a19531ac3 100644 --- a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp +++ b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp @@ -232,7 +232,7 @@ Instruction *ArchitectureMips::GetInstructionAtAddress( return nullptr; } -bool ArchitectureMips::IsValidBreakpointInstruction( +bool ArchitectureMips::IsValidTrapInstruction( llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { // The middle twenty bits of BREAK can be anything, so zero them uint32_t mask = 0xFC00003F; diff --git a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h index 8d48eef61019f..d2475d21b8de6 100644 --- a/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h +++ b/lldb/source/Plugins/Architecture/Mips/ArchitectureMips.h @@ -33,9 +33,8 @@ class ArchitectureMips : public Architecture { lldb::addr_t GetOpcodeLoadAddress(lldb::addr_t load_addr, AddressClass addr_class) const override; - bool - IsValidBreakpointInstruction(llvm::ArrayRef<uint8_t> reference, - llvm::ArrayRef<uint8_t> observed) const override; + bool IsValidTrapInstruction(llvm::ArrayRef<uint8_t> reference, + llvm::ArrayRef<uint8_t> observed) const override; private: Instruction *GetInstructionAtAddress(Target &target, diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index d56ae17bc0aec..3b814e9a23f40 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -1186,7 +1186,7 @@ class StopInfoUnixSignal : public StopInfo { if (auto *arch_plugin = target.GetArchitecturePlugin(); arch_plugin && - arch_plugin->IsValidBreakpointInstruction( + arch_plugin->IsValidTrapInstruction( platform_opcode, llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size()))) { LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", >From 4812e7046058b41074f682c93914513029064cae Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Fri, 9 Jan 2026 20:18:56 +0000 Subject: [PATCH 4/8] Stop using invalid array --- lldb/source/Target/Platform.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index 47c543f72398a..26364080513b5 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -2056,11 +2056,9 @@ llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeBytes(const ArchSpec &arch, static const uint8_t g_wasm_opcode[] = {0x00}; trap_opcode = llvm::ArrayRef<uint8_t>(g_wasm_opcode, sizeof(g_wasm_opcode)); } break; - // The default case should not match against anything, so return a zero - // size array. + // The default case should not match against anything, so return a nullptr default: { - static const uint8_t g_no_opcode[] = {}; - trap_opcode = llvm::ArrayRef<uint8_t>(g_no_opcode, 0); + trap_opcode = llvm::ArrayRef<uint8_t>(nullptr, 0); }; } return trap_opcode; >From 3c2ee20fb5f1f4dea56630250951218887f6468d Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Fri, 9 Jan 2026 21:00:11 +0000 Subject: [PATCH 5/8] Default construct default case trap opcode --- lldb/source/Target/Platform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index 26364080513b5..c2706406c5f17 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -2058,7 +2058,7 @@ llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeBytes(const ArchSpec &arch, } break; // The default case should not match against anything, so return a nullptr default: { - trap_opcode = llvm::ArrayRef<uint8_t>(nullptr, 0); + trap_opcode = llvm::ArrayRef<uint8_t>{}}; }; } return trap_opcode; >From aad7580963b710db1f1bf84a19f215163a1eb600 Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Thu, 15 Jan 2026 16:48:56 +0000 Subject: [PATCH 6/8] Change 2/4 byte logic handling and support mac The implementation has been hoisted to a helper function that actually does the work, because everyone does the same thing, just guarded on a different condition in a different subclass. The handling of byte sequences of 2 or 4 bytes has been improved, to make sure that we always capture the right sequence length. --- lldb/include/lldb/Core/Architecture.h | 11 ++- lldb/include/lldb/Target/Platform.h | 11 +++ lldb/include/lldb/Target/StopInfo.h | 4 ++ .../Process/Utility/StopInfoMachException.cpp | 7 ++ .../Process/Utility/StopInfoMachException.h | 4 ++ lldb/source/Target/Platform.cpp | 24 ++++++- lldb/source/Target/StopInfo.cpp | 70 ++++++++++--------- .../source/MacOSX/arm64/DNBArchImplARM64.cpp | 22 ------ 8 files changed, 91 insertions(+), 62 deletions(-) diff --git a/lldb/include/lldb/Core/Architecture.h b/lldb/include/lldb/Core/Architecture.h index 5d312bef85d95..f31f23347fb54 100644 --- a/lldb/include/lldb/Core/Architecture.h +++ b/lldb/include/lldb/Core/Architecture.h @@ -139,12 +139,11 @@ class Architecture : public PluginInterface { return lldb::UnwindPlanSP(); } - /// Returns whether a given byte sequence is a valid breakpoint for the - /// architecture. Some architectures have breakpoint instructions that - /// have immediates that can take on any value, resulting in a family - /// of valid byte sequences. If the observed byte sequence is shorter - /// than the reference then they are considered not to match, even if - /// the initial bytes would match. + /// Returns whether a given byte sequence is a valid trap instruction for the + /// architecture. Some architectures feature instructions that have immediates + /// that can take on any value, resulting in a family of valid byte sequences. + /// If the observed byte sequence is shorter than the reference then they are + /// considered not to match, even if the initial bytes would match. virtual bool IsValidTrapInstruction(llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const { if (reference.size() > observed.size()) diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index 98a2b0257e817..4fbb2467dfe8f 100644 --- a/lldb/include/lldb/Target/Platform.h +++ b/lldb/include/lldb/Target/Platform.h @@ -341,6 +341,17 @@ class Platform : public PluginInterface { llvm::ArrayRef<uint8_t> SoftwareTrapOpcodeBytes(const ArchSpec &arch, size_t size_hint = 0); + /// Get the suggested size hint for a trap instruction on the given target. + /// + /// \param[in] target + /// The target of the inferior + /// \param addr + /// The address of the instruction + /// \param bytes + /// The raw bytes of the instruction + size_t GetTrapOpcodeSizeHint(Target &target, Address addr, + llvm::ArrayRef<uint8_t> bytes); + virtual size_t GetSoftwareBreakpointTrapOpcode(Target &target, BreakpointSite *bp_site); diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h index cdd6a6fbe6aa4..495ca526d1721 100644 --- a/lldb/include/lldb/Target/StopInfo.h +++ b/lldb/include/lldb/Target/StopInfo.h @@ -218,6 +218,10 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> { // to consult this later on. virtual bool ShouldStop(Event *event_ptr) { return true; } + // Shared implementation for when a trap instruction leaves the CPU PC at + // the trap instruction instead of just after it. + void SkipOverTrapInstruction(); + // Classes that inherit from StackID can see and modify these lldb::ThreadWP m_thread_wp; // The thread corresponding to the stop reason. uint32_t m_stop_id; // The process stop ID for which this stop info is valid diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp index d374742b5722c..f523dc80248a0 100644 --- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp +++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp @@ -827,6 +827,13 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( not_stepping_but_got_singlestep_exception); } +void StopInfoMachException::PerformAction([[maybe_unused]] Event *event_ptr) { + if (!(m_value == 6 /*EXC_BREAKPOINT*/ && + m_exc_code == 1 /*EXC_ARM_BREAKPOINT*/)) + return; + SkipOverTrapInstruction(); +} + // Detect an unusual situation on Darwin where: // // 0. We did an instruction-step before this. diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.h b/lldb/source/Plugins/Process/Utility/StopInfoMachException.h index 26f93e0bb348f..2a186c41b366b 100644 --- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.h +++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.h @@ -58,6 +58,10 @@ class StopInfoMachException : public StopInfo { }; #endif + /// Allow this plugin to respond to stop events to enable skip-over-trap + /// behaviour on AArch64. + void PerformAction(Event *event_ptr) override; + // Since some mach exceptions will be reported as breakpoints, signals, // or trace, we use this static accessor which will translate the mach // exception into the correct StopInfo. diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index c2706406c5f17..24c535c1adfca 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -2056,14 +2056,34 @@ llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeBytes(const ArchSpec &arch, static const uint8_t g_wasm_opcode[] = {0x00}; trap_opcode = llvm::ArrayRef<uint8_t>(g_wasm_opcode, sizeof(g_wasm_opcode)); } break; - // The default case should not match against anything, so return a nullptr + // The default case should not match against anything, so return empty Array default: { - trap_opcode = llvm::ArrayRef<uint8_t>{}}; + trap_opcode = llvm::ArrayRef<uint8_t>{}; }; } return trap_opcode; } +size_t Platform::GetTrapOpcodeSizeHint(Target &target, Address addr, + llvm::ArrayRef<uint8_t> bytes) { + ArchSpec arch = target.GetArchitecture(); + assert(arch.IsValid()); + const auto &triple = arch.GetTriple(); + + if (bytes.size() && triple.isRISCV()) { + // RISC-V instructions have the two LSB as 0b11 if they are four-byte + return (bytes[0] & 0b11) == 0b11 ? 4 : 2; + } + + if (triple.isARM()) { + if (auto addr_class = addr.GetAddressClass(); + addr_class == AddressClass::eCodeAlternateISA) { + return 2; + } + } + return 0; +} + size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target, BreakpointSite *bp_site) { ArchSpec arch = target.GetArchitecture(); diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 3b814e9a23f40..8ef55bdf6a3a5 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include <array> #include <string> #include "lldb/Breakpoint/Breakpoint.h" @@ -82,6 +83,40 @@ bool StopInfo::HasTargetRunSinceMe() { return false; } +void StopInfo::SkipOverTrapInstruction() { + Status error; + Log *log = GetLog(LLDBLog::Process); + // We don't expect to see byte sequences longer than four bytes long for + // any breakpoint instructions known to LLDB. + std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; + auto reg_ctx_sp = GetThread()->GetRegisterContext(); + auto process_sp = GetThread()->GetProcess(); + addr_t pc = reg_ctx_sp->GetPC(); + if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), + error)) { + // If this fails, we simply don't handle the step-over-break logic and + // log the failure + LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", pc, + error); + return; + } + auto &target = process_sp->GetTarget(); + auto platform_sp = target.GetPlatform(); + auto size_hint = platform_sp->GetTrapOpcodeSizeHint(target, pc, bytes_at_pc); + auto platform_opcode = + platform_sp->SoftwareTrapOpcodeBytes(target.GetArchitecture(), size_hint); + + if (auto *arch_plugin = target.GetArchitecturePlugin(); + arch_plugin && + arch_plugin->IsValidTrapInstruction( + platform_opcode, + llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size()))) { + LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", + pc + platform_opcode.size()); + reg_ctx_sp->SetPC(pc + platform_opcode.size()); + } +} + // StopInfoBreakpoint namespace lldb_private { @@ -1159,40 +1194,11 @@ class StopInfoUnixSignal : public StopInfo { return false; } - void PerformAction(Event [[maybe_unused]] * event_ptr) override { - // A signal of SIGTRAP indicates that a break instruction has been hit + void PerformAction([[maybe_unused]] Event *event_ptr) override { + // A signal of SIGTRAP indicates that a trap instruction has been hit if (m_value != SIGTRAP) return; - Log *log = GetLog(LLDBLog::Process); - Status error; - // We don't expect to see byte sequences longer than four bytes long for - // any breakpoint instructions known to LLDB. - std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; - auto reg_ctx_sp = GetThread()->GetRegisterContext(); - auto process_sp = GetThread()->GetProcess(); - addr_t pc = reg_ctx_sp->GetPC(); - if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), - error)) { - // If this fails, we simply don't handle the step-over-break logic and - // log the failure - LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", - pc, error); - return; - } - auto &target = process_sp->GetTarget(); - auto platform_sp = target.GetPlatform(); - auto platform_opcode = - platform_sp->SoftwareTrapOpcodeBytes(target.GetArchitecture()); - - if (auto *arch_plugin = target.GetArchitecturePlugin(); - arch_plugin && - arch_plugin->IsValidTrapInstruction( - platform_opcode, - llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size()))) { - LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", - pc + platform_opcode.size()); - reg_ctx_sp->SetPC(pc + platform_opcode.size()); - } + SkipOverTrapInstruction(); } bool ShouldStop(Event *event_ptr) override { return IsShouldStopSignal(); } diff --git a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp index e30e02a911529..83da00d3de370 100644 --- a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp +++ b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp @@ -842,28 +842,6 @@ bool DNBArchMachARM64::NotifyException(MachException::Data &exc) { return true; } - // detect a __builtin_debugtrap instruction pattern ("brk #0xf000") - // and advance the $pc past it, so that the user can continue execution. - // Generally speaking, this knowledge should be centralized in lldb, - // recognizing the builtin_trap instruction and knowing how to advance - // the pc past it, so that continue etc work. - if (exc.exc_data.size() == 2 && exc.exc_data[0] == EXC_ARM_BREAKPOINT) { - nub_addr_t pc = GetPC(INVALID_NUB_ADDRESS); - if (pc != INVALID_NUB_ADDRESS && pc > 0) { - DNBBreakpoint *bp = - m_thread->Process()->Breakpoints().FindByAddress(pc); - if (bp == nullptr) { - uint8_t insnbuf[4]; - if (m_thread->Process()->ReadMemory(pc, 4, insnbuf) == 4) { - uint8_t builtin_debugtrap_insn[4] = {0x00, 0x00, 0x3e, - 0xd4}; // brk #0xf000 - if (memcmp(insnbuf, builtin_debugtrap_insn, 4) == 0) { - SetPC(pc + 4); - } - } - } - } - } break; } return false; >From 25ea41f192027ebb9eef2509bdcbd5639e6cf804 Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Fri, 30 Jan 2026 15:15:54 +0000 Subject: [PATCH 7/8] Add review feedback Largely improve comment quantity and quality, plus add a fallback case for arm size hints. Also, keep the debugserver pc jump handling for now, with an eye to remove it later should this patch work. --- lldb/include/lldb/Target/Platform.h | 10 ++++++++- lldb/include/lldb/Target/StopInfo.h | 4 +++- lldb/source/Target/Platform.cpp | 6 +++-- lldb/source/Target/StopInfo.cpp | 12 ++++++---- .../source/MacOSX/arm64/DNBArchImplARM64.cpp | 22 +++++++++++++++++++ 5 files changed, 46 insertions(+), 8 deletions(-) diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h index 4fbb2467dfe8f..ecfd514a26b31 100644 --- a/lldb/include/lldb/Target/Platform.h +++ b/lldb/include/lldb/Target/Platform.h @@ -330,7 +330,10 @@ class Platform : public PluginInterface { virtual std::vector<ArchSpec> GetSupportedArchitectures(const ArchSpec &process_host_arch) = 0; - /// Get the bytes of the platform's software interrupt instruction. + /// Get the bytes of the platform's software interrupt instruction. If there + /// are multiple possible encodings, for example where there are immediate + /// values encoded in the instruction, this will return the instruction with + /// those bits set as 0. /// /// \param[in] arch /// The architecture of the inferior @@ -342,6 +345,11 @@ class Platform : public PluginInterface { size_t size_hint = 0); /// Get the suggested size hint for a trap instruction on the given target. + /// Some platforms have a compressed instruction set which can be used + /// instead of the "normal" encoding. This function attempts to determine + /// a size hint for the size of the instruction at address \a addr, and + /// return 0, 2 or 4, with 2 and 4 corresponding to the estimated size + /// and zero meaning no applicable hint. /// /// \param[in] target /// The target of the inferior diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h index 495ca526d1721..fbbad7c91fba5 100644 --- a/lldb/include/lldb/Target/StopInfo.h +++ b/lldb/include/lldb/Target/StopInfo.h @@ -219,7 +219,9 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> { virtual bool ShouldStop(Event *event_ptr) { return true; } // Shared implementation for when a trap instruction leaves the CPU PC at - // the trap instruction instead of just after it. + // the trap instruction, instead of just after it. Currently the subclasses + // StopInfoMachException and StopInfoUnixSignal use this to skip over the + // instruction at the PC if it is a matching trap instruction. void SkipOverTrapInstruction(); // Classes that inherit from StackID can see and modify these diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index 24c535c1adfca..096c1b97113ee 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -2056,7 +2056,7 @@ llvm::ArrayRef<uint8_t> Platform::SoftwareTrapOpcodeBytes(const ArchSpec &arch, static const uint8_t g_wasm_opcode[] = {0x00}; trap_opcode = llvm::ArrayRef<uint8_t>(g_wasm_opcode, sizeof(g_wasm_opcode)); } break; - // The default case should not match against anything, so return empty Array + // The default case should not match against anything, so return empty Array. default: { trap_opcode = llvm::ArrayRef<uint8_t>{}; }; @@ -2071,7 +2071,7 @@ size_t Platform::GetTrapOpcodeSizeHint(Target &target, Address addr, const auto &triple = arch.GetTriple(); if (bytes.size() && triple.isRISCV()) { - // RISC-V instructions have the two LSB as 0b11 if they are four-byte + // RISC-V instructions have the two LSB as 0b11 if they are four-byte. return (bytes[0] & 0b11) == 0b11 ? 4 : 2; } @@ -2079,6 +2079,8 @@ size_t Platform::GetTrapOpcodeSizeHint(Target &target, Address addr, if (auto addr_class = addr.GetAddressClass(); addr_class == AddressClass::eCodeAlternateISA) { return 2; + } else { + return 4; } } return 0; diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 8ef55bdf6a3a5..232e01ae732c2 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -86,6 +86,7 @@ bool StopInfo::HasTargetRunSinceMe() { void StopInfo::SkipOverTrapInstruction() { Status error; Log *log = GetLog(LLDBLog::Process); + // We don't expect to see byte sequences longer than four bytes long for // any breakpoint instructions known to LLDB. std::array<uint8_t, 4> bytes_at_pc = {0, 0, 0, 0}; @@ -94,24 +95,27 @@ void StopInfo::SkipOverTrapInstruction() { addr_t pc = reg_ctx_sp->GetPC(); if (!process_sp->ReadMemory(pc, bytes_at_pc.data(), bytes_at_pc.size(), error)) { - // If this fails, we simply don't handle the step-over-break logic and - // log the failure + // If this fails, we simply don't handle the step-over-break logic. LLDB_LOG(log, "failed to read program bytes at pc address {}, error {}", pc, error); return; } + auto &target = process_sp->GetTarget(); auto platform_sp = target.GetPlatform(); auto size_hint = platform_sp->GetTrapOpcodeSizeHint(target, pc, bytes_at_pc); auto platform_opcode = platform_sp->SoftwareTrapOpcodeBytes(target.GetArchitecture(), size_hint); + auto is_valid_trap = arch_plugin->IsValidTrapInstruction(platform_opcode, + llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size() + if (auto *arch_plugin = target.GetArchitecturePlugin(); arch_plugin && arch_plugin->IsValidTrapInstruction( platform_opcode, llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size()))) { - LLDB_LOG(log, "stepping over breakpoint in debuggee to new pc: {}", + LLDB_LOG(log, "stepping over breakpoint in inferior to new pc: {}", pc + platform_opcode.size()); reg_ctx_sp->SetPC(pc + platform_opcode.size()); } @@ -1195,7 +1199,7 @@ class StopInfoUnixSignal : public StopInfo { } void PerformAction([[maybe_unused]] Event *event_ptr) override { - // A signal of SIGTRAP indicates that a trap instruction has been hit + // A signal of SIGTRAP indicates that a trap instruction has been hit. if (m_value != SIGTRAP) return; SkipOverTrapInstruction(); diff --git a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp index 83da00d3de370..e30e02a911529 100644 --- a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp +++ b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp @@ -842,6 +842,28 @@ bool DNBArchMachARM64::NotifyException(MachException::Data &exc) { return true; } + // detect a __builtin_debugtrap instruction pattern ("brk #0xf000") + // and advance the $pc past it, so that the user can continue execution. + // Generally speaking, this knowledge should be centralized in lldb, + // recognizing the builtin_trap instruction and knowing how to advance + // the pc past it, so that continue etc work. + if (exc.exc_data.size() == 2 && exc.exc_data[0] == EXC_ARM_BREAKPOINT) { + nub_addr_t pc = GetPC(INVALID_NUB_ADDRESS); + if (pc != INVALID_NUB_ADDRESS && pc > 0) { + DNBBreakpoint *bp = + m_thread->Process()->Breakpoints().FindByAddress(pc); + if (bp == nullptr) { + uint8_t insnbuf[4]; + if (m_thread->Process()->ReadMemory(pc, 4, insnbuf) == 4) { + uint8_t builtin_debugtrap_insn[4] = {0x00, 0x00, 0x3e, + 0xd4}; // brk #0xf000 + if (memcmp(insnbuf, builtin_debugtrap_insn, 4) == 0) { + SetPC(pc + 4); + } + } + } + } + } break; } return false; >From 52583b6f77fbaa2ed21da4de0a6179d8400b66f1 Mon Sep 17 00:00:00 2001 From: Duncan McBain <[email protected]> Date: Fri, 30 Jan 2026 17:24:54 +0000 Subject: [PATCH 8/8] Remove unused temporary bool variable --- .../Plugins/Architecture/AArch64/ArchitectureAArch64.cpp | 4 ++-- lldb/source/Target/StopInfo.cpp | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp index 6ccca2404e3c7..29fb9ce65ab0b 100644 --- a/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp +++ b/lldb/source/Plugins/Architecture/AArch64/ArchitectureAArch64.cpp @@ -154,7 +154,7 @@ bool ArchitectureAArch64::IsValidTrapInstruction( // include an immediate value that we will explicitly check against. uint32_t mask = 0xFFE00000; // Check that the masked bytes match the reference, but also check that the - // immediate in the instruction is the default output by llvm.debugtrap - // The reference has the immediate set as all-zero, so mask and check here + // immediate in the instruction is the default output by llvm.debugtrap. + // The reference has the immediate set as all-zero, so mask and check here. return (ref_bytes == (bytes & mask)) && ((bytes & ~mask) >> 5 == 0xF000); } diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 232e01ae732c2..2ec21f7d4560c 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -107,9 +107,6 @@ void StopInfo::SkipOverTrapInstruction() { auto platform_opcode = platform_sp->SoftwareTrapOpcodeBytes(target.GetArchitecture(), size_hint); - auto is_valid_trap = arch_plugin->IsValidTrapInstruction(platform_opcode, - llvm::ArrayRef<uint8_t>(bytes_at_pc.data(), bytes_at_pc.size() - if (auto *arch_plugin = target.GetArchitecturePlugin(); arch_plugin && arch_plugin->IsValidTrapInstruction( _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
