[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.

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" : :);
+}