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

Summary:
This patch adds an interface GetLastInstrSize to get information about
the size of last tried to be decoded instruction and uses it to set
software breakpoint if the memory can be decoded as instruction.

RISC-V architecture instruction format specifies the length of
instruction in first bits, so we can set a breakpoint for these cases.
This is needed as RISCV have a lot of extensions, that are not supported
by `EmulateInstructionRISCV`.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251663

Author: ita-sc

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() {

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",

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

lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp

@@ -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,
+                                        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 +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()) {
+    // 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 +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;
 }

lldb/test/API/riscv/break-undecoded/Makefile

@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py

@@ -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"],
+        )

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 in illegal instruction during execution, not fail to set
+  // breakpoint
+  asm volatile(".2byte 0xaf");
+}

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 in illegal instruction during execution, not fail to set
+  // breakpoint
+  asm volatile(".4byte 0xc58573" : :);
+}