[Lldb-commits] [lldb] [lldb][RISCV] fix LR/SC atomic sequence handling in lldb-server (PR #127505)

[David Spickett via lldb-commits]

================
@@ -1792,4 +1778,127 @@ bool EmulateInstructionRISCV::SupportsThisArch(const ArchSpec &arch) {
   return arch.GetTriple().isRISCV();
 }
 
+BreakpointLocations
+RISCVSingleStepBreakpointLocationsPredictor::GetBreakpointLocations(
+    Status &status) {
+  EmulateInstructionRISCV *riscv_emulator =
+      static_cast<EmulateInstructionRISCV *>(m_emulator_up.get());
+
+  auto pc = riscv_emulator->ReadPC();
+  if (!pc) {
+    status = Status("Can't read PC");
+    return {};
+  }
+
+  auto inst = riscv_emulator->ReadInstructionAt(*pc);
+  if (!inst) {
+    // Can't read instruction. Try default handler.
+    return SingleStepBreakpointLocationsPredictor::GetBreakpointLocations(
+        status);
+  }
+
+  if (FoundLoadReserve(inst->decoded))
+    return HandleAtomicSequence(*pc, status);
+
+  if (FoundStoreConditional(inst->decoded)) {
+    // Ill-formed atomic sequence (SC doesn't have corresponding LR
+    // instruction). Consider SC instruction like a non-atomic store and set a
+    // breakpoint at the next instruction.
+    Log *log = GetLog(LLDBLog::Unwind);
+    LLDB_LOGF(log,
+              "RISCVSingleStepBreakpointLocationsPredictor::%s: can't find "
+              "corresponding load reserve insturuction",
+              __FUNCTION__);
+    return {*pc + inst->is_rvc ? 2u : 4u};
+  }
+
+  return SingleStepBreakpointLocationsPredictor::GetBreakpointLocations(status);
+}
+
+llvm::Expected<unsigned>
+RISCVSingleStepBreakpointLocationsPredictor::GetBreakpointSize(
+    lldb::addr_t bp_addr) {
+  EmulateInstructionRISCV *riscv_emulator =
+      static_cast<EmulateInstructionRISCV *>(m_emulator_up.get());
+
+  if (auto inst = riscv_emulator->ReadInstructionAt(bp_addr); inst)
+    return inst->is_rvc ? 2 : 4;
+
+  // Try last instruction size.
+  if (auto last_instr_size = riscv_emulator->GetLastInstrSize();
+      last_instr_size)
+    return *last_instr_size;
+
+  // Just place non-compressed software trap.
+  return 4;
+}
+
+BreakpointLocations
+RISCVSingleStepBreakpointLocationsPredictor::HandleAtomicSequence(
+    lldb::addr_t pc, Status &error) {
+  EmulateInstructionRISCV *riscv_emulator =
+      static_cast<EmulateInstructionRISCV *>(m_emulator_up.get());
+
+  // Handle instructions between LR and SC. According to unprivilleged
+  // RISC-V ISA there can be at most 16 instructions in the sequence.
+
+  lldb::addr_t entry_pc = pc; // LR instruction address
+  pc += 4;                    // add LR_W, LR_D instruction size
+
+  size_t atomic_length = 0;
+  std::optional<DecodeResult> inst;
+  std::vector<lldb::addr_t> bp_addrs;
+  do {
+    inst = riscv_emulator->ReadInstructionAt(pc);
+    if (!inst) {
+      error = Status("Can't read instruction");
+      return {};
+    }
+
+    if (B *branch = std::get_if<B>(&inst->decoded))
+      bp_addrs.push_back(pc + SignExt(branch->imm));
+
+    unsigned addent = inst->is_rvc ? 2 : 4;
+    pc += addent;
+    atomic_length += addent;
+  } while ((atomic_length < s_max_atomic_sequence_length) &&
+           !FoundStoreConditional(inst->decoded));
+
+  if (atomic_length >= s_max_atomic_sequence_length) {
+    // Ill-formed atomic sequence (LR doesn't have corresponding SC
+    // instruction). In this case consider LR like a non-atomic load instruction
+    // and set a breakpoint at the next after LR instruction.
+    Log *log = GetLog(LLDBLog::Unwind);
+    LLDB_LOGF(log,
+              "RISCVSingleStepBreakpointLocationsPredictor::%s: can't find "
+              "corresponding store conditional insturuction",
+              __FUNCTION__);
+    return {entry_pc + 4};
----------------
DavidSpickett wrote:

Asking this again because I think GitHub hid the comment, is +4 safe here? Could it be +2 if the instruction is compressed, or do you already know that it won't be?

https://github.com/llvm/llvm-project/pull/127505