[Lldb-commits] [lldb] [lldb][riscv] Fix setting breakpoint for undecoded instruction (PR #90075)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Jun 7 01:29:17 PDT 2024
https://github.com/ita-sc updated https://github.com/llvm/llvm-project/pull/90075
>From 11ecb22c9473a2eee135ba0373e3890b9389851a Mon Sep 17 00:00:00 2001
From: Ivan Tetyushkin <ivan.tetyushkin at syntacore.com>
Date: Fri, 7 Jun 2024 11:23:24 +0300
Subject: [PATCH] [lldb][riscv] Fix setting breakpoint for undecoded
instruction
Copy gdb behaviour:
For RISCV we can set breakpoint even for unknown instruction, as
RISCV encoding have information about size of instruction.
---
lldb/include/lldb/Core/EmulateInstruction.h | 2 +
.../RISCV/EmulateInstructionRISCV.cpp | 23 +++++-
.../RISCV/EmulateInstructionRISCV.h | 3 +
.../NativeProcessSoftwareSingleStep.cpp | 78 ++++++++++++-------
lldb/test/API/riscv/break-undecoded/Makefile | 3 +
.../break-undecoded/TestBreakpointIllegal.py | 45 +++++++++++
.../API/riscv/break-undecoded/compressed.c | 7 ++
lldb/test/API/riscv/break-undecoded/main.c | 7 ++
8 files changed, 139 insertions(+), 29 deletions(-)
create mode 100644 lldb/test/API/riscv/break-undecoded/Makefile
create mode 100644 lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py
create mode 100644 lldb/test/API/riscv/break-undecoded/compressed.c
create mode 100644 lldb/test/API/riscv/break-undecoded/main.c
diff --git a/lldb/include/lldb/Core/EmulateInstruction.h b/lldb/include/lldb/Core/EmulateInstruction.h
index 93c16537adba1..b459476883fc5 100644
--- a/lldb/include/lldb/Core/EmulateInstruction.h
+++ b/lldb/include/lldb/Core/EmulateInstruction.h
@@ -369,6 +369,8 @@ class EmulateInstruction : public PluginInterface {
virtual bool ReadInstruction() = 0;
+ virtual std::optional<uint32_t> GetLastInstrSize() { return std::nullopt; }
+
virtual bool EvaluateInstruction(uint32_t evaluate_options) = 0;
virtual InstructionCondition GetInstructionCondition() {
diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 6c46618b337c2..3f61e011d620a 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -624,9 +624,26 @@ std::optional<DecodeResult> EmulateInstructionRISCV::Decode(uint32_t inst) {
uint16_t try_rvc = uint16_t(inst & 0x0000ffff);
// check whether the compressed encode could be valid
uint16_t mask = try_rvc & 0b11;
- bool is_rvc = try_rvc != 0 && mask != 3;
uint8_t inst_type = RV64;
+ // Try to get size of RISCV instruction.
+ // 1.2 Instruction Length Encoding
+ bool is_16b = (inst & 0b11) != 0b11;
+ bool is_32b = (inst & 0x1f) != 0x1f;
+ bool is_48b = (inst & 0x3f) != 0x1f;
+ bool is_64b = (inst & 0x7f) != 0x3f;
+ if (is_16b)
+ m_last_size = 2;
+ else if (is_32b)
+ m_last_size = 4;
+ else if (is_48b)
+ m_last_size = 6;
+ else if (is_64b)
+ m_last_size = 8;
+ else
+ // Not Valid
+ m_last_size = std::nullopt;
+
// if we have ArchSpec::eCore_riscv128 in the future,
// we also need to check it here
if (m_arch.GetCore() == ArchSpec::eCore_riscv32)
@@ -638,8 +655,8 @@ std::optional<DecodeResult> EmulateInstructionRISCV::Decode(uint32_t inst) {
LLDB_LOGF(
log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was decoded to %s",
__FUNCTION__, inst, m_addr, pat.name);
- auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
- return DecodeResult{decoded, inst, is_rvc, pat};
+ auto decoded = is_16b ? pat.decode(try_rvc) : pat.decode(inst);
+ return DecodeResult{decoded, inst, is_16b, pat};
}
}
LLDB_LOGF(log, "EmulateInstructionRISCV::%s: inst(0x%x) was unsupported",
diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
index 8bca73a7f589d..53ac11c2e1102 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
@@ -60,6 +60,7 @@ class EmulateInstructionRISCV : public EmulateInstruction {
bool SetTargetTriple(const ArchSpec &arch) override;
bool ReadInstruction() override;
+ std::optional<uint32_t> GetLastInstrSize() override { return m_last_size; }
bool EvaluateInstruction(uint32_t options) override;
bool TestEmulation(Stream &out_stream, ArchSpec &arch,
OptionValueDictionary *test_data) override;
@@ -99,6 +100,8 @@ class EmulateInstructionRISCV : public EmulateInstruction {
private:
/// Last decoded instruction from m_opcode
DecodeResult m_decoded;
+ /// Last decoded instruction size estimate.
+ std::optional<uint32_t> m_last_size;
};
} // namespace lldb_private
diff --git a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
index 6bf8a0dc28b22..fa7d7407bb720 100644
--- a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
@@ -94,6 +94,39 @@ static lldb::addr_t ReadFlags(NativeRegisterContext ®siter_context) {
LLDB_INVALID_ADDRESS);
}
+static int GetSoftwareBreakpointSize(const ArchSpec &arch,
+ lldb::addr_t next_flags) {
+ if (arch.GetMachine() == llvm::Triple::arm) {
+ if (next_flags & 0x20)
+ // Thumb mode
+ return 2;
+ else
+ // Arm mode
+ return 4;
+ }
+ if (arch.IsMIPS() || arch.GetTriple().isPPC64() ||
+ arch.GetTriple().isRISCV() || arch.GetTriple().isLoongArch())
+ return 4;
+ return 0;
+}
+
+static Status SetSoftwareBreakpointOnPC(const ArchSpec &arch, lldb::addr_t pc,
+ lldb::addr_t next_flags,
+ NativeProcessProtocol &process) {
+ int size_hint = GetSoftwareBreakpointSize(arch, next_flags);
+ Status error;
+ error = process.SetBreakpoint(pc, size_hint, /*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;
@@ -115,8 +148,23 @@ Status NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping(
emulator_up->SetWriteMemCallback(&WriteMemoryCallback);
emulator_up->SetWriteRegCallback(&WriteRegisterCallback);
- if (!emulator_up->ReadInstruction())
- return Status("Read instruction failed!");
+ if (!emulator_up->ReadInstruction()) {
+ // try to get at least the size of next instruction to set breakpoint.
+ auto instr_size = emulator_up->GetLastInstrSize();
+ if (!instr_size)
+ return Status("Read instruction failed!");
+ bool success = false;
+ auto pc = emulator_up->ReadRegisterUnsigned(eRegisterKindGeneric,
+ LLDB_REGNUM_GENERIC_PC,
+ LLDB_INVALID_ADDRESS, &success);
+ if (!success)
+ return Status("Reading pc failed!");
+ lldb::addr_t next_pc = pc + *instr_size;
+ auto result =
+ SetSoftwareBreakpointOnPC(arch, next_pc, /* next_flags */ 0x0, process);
+ m_threads_stepping_with_breakpoint.insert({thread.GetID(), next_pc});
+ return result;
+ }
bool emulation_result =
emulator_up->EvaluateInstruction(eEmulateInstructionOptionAutoAdvancePC);
@@ -157,29 +205,7 @@ Status NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping(
// modifying the PC but we don't know how.
return Status("Instruction emulation failed unexpectedly.");
}
-
- int size_hint = 0;
- if (arch.GetMachine() == llvm::Triple::arm) {
- if (next_flags & 0x20) {
- // Thumb mode
- size_hint = 2;
- } else {
- // Arm mode
- size_hint = 4;
- }
- } else if (arch.IsMIPS() || arch.GetTriple().isPPC64() ||
- arch.GetTriple().isRISCV() || arch.GetTriple().isLoongArch())
- size_hint = 4;
- error = process.SetBreakpoint(next_pc, size_hint, /*hardware=*/false);
-
- // If setting the breakpoint fails because next_pc is out of the address
- // space, ignore it and let the debugee segfault.
- if (error.GetError() == EIO || error.GetError() == EFAULT) {
- return Status();
- } else if (error.Fail())
- return error;
-
+ auto result = SetSoftwareBreakpointOnPC(arch, next_pc, next_flags, process);
m_threads_stepping_with_breakpoint.insert({thread.GetID(), next_pc});
-
- return Status();
+ return result;
}
diff --git a/lldb/test/API/riscv/break-undecoded/Makefile b/lldb/test/API/riscv/break-undecoded/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/riscv/break-undecoded/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py b/lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py
new file mode 100644
index 0000000000000..b11b83e6cefce
--- /dev/null
+++ b/lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py
@@ -0,0 +1,45 @@
+"""
+Test that we can set up software breakpoint even if we failed to decode and execute instruction
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestBreakpointIllegal(TestBase):
+ @skipIf(archs=no_match(["rv64gc"]))
+ def test_4(self):
+ self.build()
+ (target, process, cur_thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "main", lldb.SBFileSpec("main.c")
+ )
+ self.runCmd("thread step-inst")
+ # we need to step more, as some compilers do not set appropriate debug info.
+ while cur_thread.GetStopDescription(256) == "instruction step into":
+ self.runCmd("thread step-inst")
+ # The stop reason of the thread should be illegal opcode.
+ self.expect(
+ "thread list",
+ STOPPED_DUE_TO_SIGNAL,
+ substrs=["stopped", "stop reason = signal SIGILL: illegal opcode"],
+ )
+
+ @skipIf(archs=no_match(["rv64gc"]))
+ def test_2(self):
+ self.build(dictionary={"C_SOURCES": "compressed.c", "EXE": "compressed.x"})
+ (target, process, cur_thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "main", lldb.SBFileSpec("compressed.c"),
+ exe_name="compressed.x"
+ )
+ self.runCmd("thread step-inst")
+ # we need to step more, as some compilers do not set appropriate debug info.
+ while cur_thread.GetStopDescription(256) == "instruction step into":
+ self.runCmd("thread step-inst")
+ # The stop reason of the thread should be illegal opcode.
+ self.expect(
+ "thread list",
+ STOPPED_DUE_TO_SIGNAL,
+ substrs=["stopped", "stop reason = signal SIGILL: illegal opcode"],
+ )
diff --git a/lldb/test/API/riscv/break-undecoded/compressed.c b/lldb/test/API/riscv/break-undecoded/compressed.c
new file mode 100644
index 0000000000000..4c9435d734bf6
--- /dev/null
+++ b/lldb/test/API/riscv/break-undecoded/compressed.c
@@ -0,0 +1,7 @@
+int main() {
+ // This instruction is not valid, but we have an ability to set
+ // software breakpoint.
+ // This results illegal instruction during execution, not fail to set
+ // breakpoint
+ asm volatile(".2byte 0xaf");
+}
diff --git a/lldb/test/API/riscv/break-undecoded/main.c b/lldb/test/API/riscv/break-undecoded/main.c
new file mode 100644
index 0000000000000..45e6e547ad316
--- /dev/null
+++ b/lldb/test/API/riscv/break-undecoded/main.c
@@ -0,0 +1,7 @@
+int main() {
+ // This instruction is not valid, but we have an ability to set
+ // software breakpoint.
+ // This results illegal instruction during execution, not fail to set
+ // breakpoint
+ asm volatile(".4byte 0xc58573" : :);
+}
More information about the lldb-commits
mailing list