Skip to content

[lldb][riscv] Fix setting breakpoint for undecoded instruction #90075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 16, 2024
Merged

[lldb][riscv] Fix setting breakpoint for undecoded instruction #90075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lldb/include/lldb/Core/EmulateInstruction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
23 changes: 20 additions & 3 deletions lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
77 changes: 51 additions & 26 deletions lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,38 @@ static lldb::addr_t ReadFlags(NativeRegisterContext &regsiter_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;
// 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but in recent years we've been preferring using llvm::Error over lldb::Status where possible. The benefit of Error is that it must be checked and it's trivial to convert between the two. It would be nice if this function would return an llvm::Error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep Status in this MR, as from my point of view, this MR will do more that it should -- it will add a new functionality and change the way we work with errors here.

Moreover, changing this will result in unnecessary casts from Status to Error (process.SetBreakpoint) and from Error to Status(SetupSoftwareSingleStepping).

Note: I've checked that we have Status::ToError and Status(llvm::Error error) ctor, but I'm not sure these functions will suffice (AFAIU m_code will be saved only if m_type == ErrorType::eErrorTypePOSIX).

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;
Expand All @@ -115,8 +147,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()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this block now be if (emulator_up->ReadInstruction()) now? We're going to get the size of the last decoded instruction here.

Copy link
Collaborator

@jasonmolenda jasonmolenda May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, I see. This method is trying to decode where the next instruction will go, with branches and jumps decoded, so we can put a breakpoint there. And you're handling the case where we can't decode the current instruction (I now understand why you used that in your test case). It seems harmless to call GetLastInstrSize() if the instruction that couldn't be decoded, and add the length of the instruction to pc. We can assume the emulation engine will emulate all branching instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are right: I'm adding an interface to get instruction size, as if it is not a jump or branch, actually, we do not need to emulate it on engine. As RISCV have a lot of extensions, even vendor-specific, we can not simply emulate all of them. I've added a common interface, as it may be helpful for other architectures, as for new implementations later user will need to implement only branches and jumps.

// 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!");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've defined the new GetLastInstrSize() method for the RISCV EmulateInstruction plugin, but others like AArch64 won't have that, so this will error out on them, won't it? What we really want to express is "if arch.GetTriple().isRISCV() and we couldn't decode the length of the last instruction, then error out" isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in this MR this is a common code, and every other architecture now not implemented GetLastInstrSize, so it will fail with the same error, as it was before. For RISC-V we will generate the same error if we can not get the size of instruction (if we can not read memory or if an instruction is bigger than expected)

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);
Expand Down Expand Up @@ -157,29 +204,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;
}
3 changes: 3 additions & 0 deletions lldb/test/API/riscv/break-undecoded/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
C_SOURCES := main.c

include Makefile.rules
44 changes: 44 additions & 0 deletions lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
"""
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"],
)
7 changes: 7 additions & 0 deletions lldb/test/API/riscv/break-undecoded/compressed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
int main() {
// This instruction is not valid, but we have an ability to set
// software breakpoint.
// This results in illegal instruction during execution, not fail to set
// breakpoint
asm volatile(".2byte 0xaf");
}
7 changes: 7 additions & 0 deletions lldb/test/API/riscv/break-undecoded/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
int main() {
// This instruction is not valid, but we have an ability to set
// software breakpoint.
// This results in illegal instruction during execution, not fail to set
// breakpoint
asm volatile(".4byte 0xc58573" : :);
}
Loading