Skip to content

[lldb][RISCV] fix LR/SC atomic sequence handling in lldb-server #146072

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dlav-sc
Copy link
Contributor

@dlav-sc dlav-sc commented Jun 27, 2025

lldb-server had limited support for single-stepping through the lr/sc atomic sequence. This patch enhances that support for all possible atomic sequences.

dlav-sc added 2 commits June 27, 2025 12:47
…#127505)

lldb-server had limited support for single-stepping through the lr/sc
atomic sequence. This patch enhances that support for all possible
atomic sequences.
@dlav-sc dlav-sc requested a review from JDevlieghere as a code owner June 27, 2025 13:07
@llvmbot llvmbot added the lldb label Jun 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-lldb

Author: None (dlav-sc)

Changes

lldb-server had limited support for single-stepping through the lr/sc atomic sequence. This patch enhances that support for all possible atomic sequences.


Patch is 47.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146072.diff

19 Files Affected:

  • (modified) lldb/include/lldb/Core/EmulateInstruction.h (+61)
  • (modified) lldb/source/Core/EmulateInstruction.cpp (+92)
  • (modified) lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp (+13)
  • (modified) lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h (+18)
  • (modified) lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp (+73-49)
  • (modified) lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h (-2)
  • (modified) lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp (+128-18)
  • (modified) lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h (+34-3)
  • (modified) lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp (+7-5)
  • (modified) lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp (+7-5)
  • (modified) lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp (+21-87)
  • (modified) lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h (+2-1)
  • (added) lldb/test/API/riscv/step/Makefile (+3)
  • (added) lldb/test/API/riscv/step/TestSoftwareStep.py (+79)
  • (added) lldb/test/API/riscv/step/branch.c (+22)
  • (added) lldb/test/API/riscv/step/incomplete_sequence_without_lr.c (+22)
  • (added) lldb/test/API/riscv/step/incomplete_sequence_without_sc.c (+21)
  • (added) lldb/test/API/riscv/step/main.c (+22)
  • (modified) lldb/unittests/Instruction/LoongArch/TestLoongArchEmulator.cpp (+18-18)
diff --git a/lldb/include/lldb/Core/EmulateInstruction.h b/lldb/include/lldb/Core/EmulateInstruction.h
index b459476883fc5..a9fd4543cbbcb 100644
--- a/lldb/include/lldb/Core/EmulateInstruction.h
+++ b/lldb/include/lldb/Core/EmulateInstruction.h
@@ -22,6 +22,8 @@
 #include "lldb/lldb-private-types.h"
 #include "lldb/lldb-types.h"
 
+#include "llvm/Support/Error.h"
+
 #include <cstddef>
 #include <cstdint>
 
@@ -32,6 +34,38 @@ class RegisterValue;
 class Stream;
 class Target;
 class UnwindPlan;
+class EmulateInstruction;
+
+using BreakpointLocations = std::vector<lldb::addr_t>;
+
+class SingleStepBreakpointLocationsPredictor {
+public:
+  SingleStepBreakpointLocationsPredictor(
+      std::unique_ptr<EmulateInstruction> emulator_up)
+      : m_emulator_up{std::move(emulator_up)} {}
+
+  virtual BreakpointLocations GetBreakpointLocations(Status &status);
+
+  virtual llvm::Expected<unsigned>
+  GetBreakpointSize([[maybe_unused]] lldb::addr_t bp_addr) {
+    return 4;
+  }
+
+  virtual ~SingleStepBreakpointLocationsPredictor() = default;
+
+protected:
+  // This function retrieves the address of the next instruction as it appears
+  // in the binary file. Essentially, it reads the value of the PC register,
+  // determines the size of the current instruction (where the PC is pointing),
+  // and returns the sum of these two values.
+  lldb::addr_t GetNextInstructionAddress(Status &error);
+
+  lldb::addr_t GetBreakpointLocationAddress(lldb::addr_t entry_pc,
+                                            Status &error);
+
+  std::unique_ptr<EmulateInstruction> m_emulator_up;
+  bool m_emulation_result = false;
+};
 
 /// \class EmulateInstruction EmulateInstruction.h
 /// "lldb/Core/EmulateInstruction.h"
@@ -497,7 +531,19 @@ class EmulateInstruction : public PluginInterface {
   static uint32_t GetInternalRegisterNumber(RegisterContext *reg_ctx,
                                             const RegisterInfo &reg_info);
 
+  static std::unique_ptr<SingleStepBreakpointLocationsPredictor>
+  CreateBreakpointLocationPredictor(
+      std::unique_ptr<EmulateInstruction> emulator_up);
+
+  // Helper functions
+  std::optional<lldb::addr_t> ReadPC();
+  bool WritePC(lldb::addr_t addr);
+
 protected:
+  using BreakpointLocationsPredictorCreator =
+      std::function<std::unique_ptr<SingleStepBreakpointLocationsPredictor>(
+          std::unique_ptr<EmulateInstruction>)>;
+
   ArchSpec m_arch;
   void *m_baton = nullptr;
   ReadMemoryCallback m_read_mem_callback = &ReadMemoryDefault;
@@ -508,6 +554,21 @@ class EmulateInstruction : public PluginInterface {
   Opcode m_opcode;
 
 private:
+  virtual BreakpointLocationsPredictorCreator
+  GetSingleStepBreakpointLocationsPredictorCreator() {
+    if (!m_arch.IsMIPS() && !m_arch.GetTriple().isPPC64() &&
+        !m_arch.GetTriple().isLoongArch()) {
+      // Unsupported architecture
+      return [](std::unique_ptr<EmulateInstruction> emulator_up) {
+        return nullptr;
+      };
+    }
+    return [](std::unique_ptr<EmulateInstruction> emulator_up) {
+      return std::make_unique<SingleStepBreakpointLocationsPredictor>(
+          std::move(emulator_up));
+    };
+  }
+
   // For EmulateInstruction only
   EmulateInstruction(const EmulateInstruction &) = delete;
   const EmulateInstruction &operator=(const EmulateInstruction &) = delete;
diff --git a/lldb/source/Core/EmulateInstruction.cpp b/lldb/source/Core/EmulateInstruction.cpp
index d240b4d3b3310..634ce1075d54b 100644
--- a/lldb/source/Core/EmulateInstruction.cpp
+++ b/lldb/source/Core/EmulateInstruction.cpp
@@ -588,7 +588,99 @@ EmulateInstruction::GetInternalRegisterNumber(RegisterContext *reg_ctx,
   return LLDB_INVALID_REGNUM;
 }
 
+std::unique_ptr<SingleStepBreakpointLocationsPredictor>
+EmulateInstruction::CreateBreakpointLocationPredictor(
+    std::unique_ptr<EmulateInstruction> emulator_up) {
+  auto creator =
+      emulator_up->GetSingleStepBreakpointLocationsPredictorCreator();
+  return creator(std::move(emulator_up));
+}
+
+std::optional<lldb::addr_t> EmulateInstruction::ReadPC() {
+  bool success = false;
+  auto addr = ReadRegisterUnsigned(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC,
+                                   LLDB_INVALID_ADDRESS, &success);
+  return success ? std::optional<addr_t>(addr) : std::nullopt;
+}
+
+bool EmulateInstruction::WritePC(lldb::addr_t addr) {
+  EmulateInstruction::Context ctx;
+  ctx.type = eContextAdvancePC;
+  ctx.SetNoArgs();
+  return WriteRegisterUnsigned(ctx, eRegisterKindGeneric,
+                               LLDB_REGNUM_GENERIC_PC, addr);
+}
+
 bool EmulateInstruction::CreateFunctionEntryUnwind(UnwindPlan &unwind_plan) {
   unwind_plan.Clear();
   return false;
 }
+
+BreakpointLocations
+SingleStepBreakpointLocationsPredictor::GetBreakpointLocations(Status &status) {
+  if (!m_emulator_up->ReadInstruction()) {
+    // try to get at least the size of next instruction to set breakpoint.
+    lldb::addr_t next_pc = GetNextInstructionAddress(status);
+    return BreakpointLocations{next_pc};
+  }
+
+  auto entry_pc = m_emulator_up->ReadPC();
+  if (!entry_pc) {
+    status = Status("Can't read PC");
+    return {};
+  }
+
+  m_emulation_result = m_emulator_up->EvaluateInstruction(
+      eEmulateInstructionOptionAutoAdvancePC);
+
+  lldb::addr_t next_pc = GetBreakpointLocationAddress(*entry_pc, status);
+  return BreakpointLocations{next_pc};
+}
+
+lldb::addr_t SingleStepBreakpointLocationsPredictor::GetNextInstructionAddress(
+    Status &error) {
+  auto instr_size = m_emulator_up->GetLastInstrSize();
+  if (!instr_size) {
+    error = Status("Read instruction failed!");
+    return LLDB_INVALID_ADDRESS;
+  }
+
+  auto pc = m_emulator_up->ReadPC();
+  if (!pc) {
+    error = Status("Can't read PC");
+    return LLDB_INVALID_ADDRESS;
+  }
+
+  lldb::addr_t next_pc = *pc + *instr_size;
+  return next_pc;
+}
+
+lldb::addr_t
+SingleStepBreakpointLocationsPredictor::GetBreakpointLocationAddress(
+    lldb::addr_t entry_pc, Status &error) {
+  auto addr = m_emulator_up->ReadPC();
+  if (!addr) {
+    error = Status("Can't read PC");
+    return LLDB_INVALID_ADDRESS;
+  }
+  lldb::addr_t pc = *addr;
+
+  if (m_emulation_result) {
+    assert(entry_pc != pc && "Emulation was successfull but PC wasn't updated");
+    return pc;
+  }
+
+  if (entry_pc == pc) {
+    // Emulate instruction failed and it hasn't changed PC. Advance PC with
+    // the size of the current opcode because the emulation of all
+    // PC modifying instruction should be successful. The failure most
+    // likely caused by an unsupported instruction which does not modify PC.
+    return pc + m_emulator_up->GetOpcode().GetByteSize();
+  }
+
+  // The instruction emulation failed after it modified the PC. It is an
+  // unknown error where we can't continue because the next instruction is
+  // modifying the PC but we don't  know how.
+  error = Status("Instruction emulation failed unexpectedly.");
+  return LLDB_INVALID_ADDRESS;
+}
diff --git a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
index 32975d88af46e..89da4d200699f 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -14471,3 +14471,16 @@ bool EmulateInstructionARM::CreateFunctionEntryUnwind(UnwindPlan &unwind_plan) {
   unwind_plan.SetReturnAddressRegister(dwarf_lr);
   return true;
 }
+
+llvm::Expected<unsigned>
+ARMSingleStepBreakpointLocationsPredictor::GetBreakpointSize(
+    lldb::addr_t bp_addr) {
+  auto flags = m_emulator_up->ReadRegisterUnsigned(
+      eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FLAGS, LLDB_INVALID_ADDRESS,
+      nullptr);
+  if (flags == LLDB_INVALID_ADDRESS)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Reading flags failed!");
+
+  return (flags & 0x20) ? /* Thumb mode */ 2 : /* Arm mode */ 4;
+}
diff --git a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
index 9eca0288607c1..7931e0e648fb0 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
+++ b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
@@ -16,6 +16,16 @@
 
 namespace lldb_private {
 
+class ARMSingleStepBreakpointLocationsPredictor
+    : public SingleStepBreakpointLocationsPredictor {
+public:
+  ARMSingleStepBreakpointLocationsPredictor(
+      std::unique_ptr<EmulateInstruction> emulator_up)
+      : SingleStepBreakpointLocationsPredictor{std::move(emulator_up)} {}
+
+  llvm::Expected<unsigned> GetBreakpointSize(lldb::addr_t bp_addr) override;
+};
+
 // ITSession - Keep track of the IT Block progression.
 class ITSession {
 public:
@@ -770,6 +780,14 @@ class EmulateInstructionARM : public EmulateInstruction {
   // B6.2.13 SUBS PC, LR and related instructions
   bool EmulateSUBSPcLrEtc(const uint32_t opcode, const ARMEncoding encoding);
 
+  BreakpointLocationsPredictorCreator
+  GetSingleStepBreakpointLocationsPredictorCreator() override {
+    return [](std::unique_ptr<EmulateInstruction> emulator_up) {
+      return std::make_unique<ARMSingleStepBreakpointLocationsPredictor>(
+          std::move(emulator_up));
+    };
+  }
+
   uint32_t m_arm_isa;
   Mode m_opcode_mode;
   uint32_t m_opcode_cpsr;
diff --git a/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp b/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp
index 37f08592d62b9..db32bf8a4dd51 100644
--- a/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp
+++ b/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp
@@ -86,7 +86,6 @@ bool EmulateInstructionLoongArch::EvaluateInstruction(uint32_t options) {
   uint32_t inst_size = m_opcode.GetByteSize();
   uint32_t inst = m_opcode.GetOpcode32();
   bool increase_pc = options & eEmulateInstructionOptionAutoAdvancePC;
-  bool success = false;
 
   Opcode *opcode_data = GetOpcodeForInstruction(inst);
   if (!opcode_data)
@@ -94,9 +93,10 @@ bool EmulateInstructionLoongArch::EvaluateInstruction(uint32_t options) {
 
   lldb::addr_t old_pc = 0;
   if (increase_pc) {
-    old_pc = ReadPC(&success);
-    if (!success)
+    auto addr = ReadPC();
+    if (!addr)
       return false;
+    old_pc = *addr;
   }
 
   // Call the Emulate... function.
@@ -104,9 +104,10 @@ bool EmulateInstructionLoongArch::EvaluateInstruction(uint32_t options) {
     return false;
 
   if (increase_pc) {
-    lldb::addr_t new_pc = ReadPC(&success);
-    if (!success)
+    auto addr = ReadPC();
+    if (!addr)
       return false;
+    lldb::addr_t new_pc = *addr;
 
     if (new_pc == old_pc && !WritePC(old_pc + inst_size))
       return false;
@@ -115,13 +116,14 @@ bool EmulateInstructionLoongArch::EvaluateInstruction(uint32_t options) {
 }
 
 bool EmulateInstructionLoongArch::ReadInstruction() {
-  bool success = false;
-  m_addr = ReadPC(&success);
-  if (!success) {
+  auto addr = ReadPC();
+  if (!addr) {
     m_addr = LLDB_INVALID_ADDRESS;
     return false;
   }
+  m_addr = *addr;
 
+  bool success = false;
   Context ctx;
   ctx.type = eContextReadOpcode;
   ctx.SetNoArgs();
@@ -131,19 +133,6 @@ bool EmulateInstructionLoongArch::ReadInstruction() {
   return true;
 }
 
-lldb::addr_t EmulateInstructionLoongArch::ReadPC(bool *success) {
-  return ReadRegisterUnsigned(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC,
-                              LLDB_INVALID_ADDRESS, success);
-}
-
-bool EmulateInstructionLoongArch::WritePC(lldb::addr_t pc) {
-  EmulateInstruction::Context ctx;
-  ctx.type = eContextAdvancePC;
-  ctx.SetNoArgs();
-  return WriteRegisterUnsigned(ctx, eRegisterKindGeneric,
-                               LLDB_REGNUM_GENERIC_PC, pc);
-}
-
 std::optional<RegisterInfo>
 EmulateInstructionLoongArch::GetRegisterInfo(lldb::RegisterKind reg_kind,
                                              uint32_t reg_index) {
@@ -273,9 +262,12 @@ bool EmulateInstructionLoongArch::EmulateNonJMP(uint32_t inst) { return false; }
 bool EmulateInstructionLoongArch::EmulateBEQZ64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint32_t offs21 = Bits32(inst, 25, 10) + (Bits32(inst, 4, 0) << 16);
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
@@ -293,9 +285,12 @@ bool EmulateInstructionLoongArch::EmulateBEQZ64(uint32_t inst) {
 bool EmulateInstructionLoongArch::EmulateBNEZ64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint32_t offs21 = Bits32(inst, 25, 10) + (Bits32(inst, 4, 0) << 16);
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
@@ -313,9 +308,12 @@ bool EmulateInstructionLoongArch::EmulateBNEZ64(uint32_t inst) {
 bool EmulateInstructionLoongArch::EmulateBCEQZ64(uint32_t inst) {
   bool success = false;
   uint32_t cj = Bits32(inst, 7, 5) + fpr_fcc0_loongarch;
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint32_t offs21 = Bits32(inst, 25, 10) + (Bits32(inst, 4, 0) << 16);
   uint8_t cj_val =
       (uint8_t)ReadRegisterUnsigned(eRegisterKindLLDB, cj, 0, &success);
@@ -335,9 +333,12 @@ bool EmulateInstructionLoongArch::EmulateBCEQZ64(uint32_t inst) {
 bool EmulateInstructionLoongArch::EmulateBCNEZ64(uint32_t inst) {
   bool success = false;
   uint32_t cj = Bits32(inst, 7, 5) + fpr_fcc0_loongarch;
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint32_t offs21 = Bits32(inst, 25, 10) + (Bits32(inst, 4, 0) << 16);
   uint8_t cj_val =
       (uint8_t)ReadRegisterUnsigned(eRegisterKindLLDB, cj, 0, &success);
@@ -358,9 +359,12 @@ bool EmulateInstructionLoongArch::EmulateJIRL64(uint32_t inst) {
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
   bool success = false;
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   EmulateInstruction::Context ctx;
   if (!WriteRegisterUnsigned(ctx, eRegisterKindLLDB, rd, pc + 4))
     return false;
@@ -374,10 +378,11 @@ bool EmulateInstructionLoongArch::EmulateJIRL64(uint32_t inst) {
 // b offs26
 // PC = PC + SignExtend({offs26, 2' b0}, GRLEN)
 bool EmulateInstructionLoongArch::EmulateB64(uint32_t inst) {
-  bool success = false;
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint32_t offs26 = Bits32(inst, 25, 10) + (Bits32(inst, 9, 0) << 16);
   uint64_t next_pc = pc + llvm::SignExtend64<28>(offs26 << 2);
   return WritePC(next_pc);
@@ -387,10 +392,11 @@ bool EmulateInstructionLoongArch::EmulateB64(uint32_t inst) {
 // GR[1] = PC + 4
 // PC = PC + SignExtend({offs26, 2'b0}, GRLEN)
 bool EmulateInstructionLoongArch::EmulateBL64(uint32_t inst) {
-  bool success = false;
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   EmulateInstruction::Context ctx;
   if (!WriteRegisterUnsigned(ctx, eRegisterKindLLDB, gpr_r1_loongarch, pc + 4))
     return false;
@@ -406,9 +412,12 @@ bool EmulateInstructionLoongArch::EmulateBEQ64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
     return false;
@@ -429,9 +438,12 @@ bool EmulateInstructionLoongArch::EmulateBNE64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
     return false;
@@ -452,9 +464,12 @@ bool EmulateInstructionLoongArch::EmulateBLT64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   int64_t rj_val =
       (int64_t)ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
@@ -477,9 +492,12 @@ bool EmulateInstructionLoongArch::EmulateBGE64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   int64_t rj_val =
       (int64_t)ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
@@ -502,9 +520,12 @@ bool EmulateInstructionLoongArch::EmulateBLTU64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
     return false;
@@ -525,9 +546,12 @@ bool EmulateInstructionLoongArch::EmulateBGEU64(uint32_t inst) {
   bool success = false;
   uint32_t rj = Bits32(inst, 9, 5);
   uint32_t rd = Bits32(inst, 4, 0);
-  uint64_t pc = ReadPC(&success);
-  if (!success)
+
+  auto addr = ReadPC();
+  if (!addr)
     return false;
+  uint64_t pc = *addr;
+
   uint64_t rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
   if (!success)
     return false;
diff --git a/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h b/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
index 47fe454fcd88c..2a8acba42f49e 100644
--- a/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
+++ b/lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
@@ -57,8 +57,6 @@ class EmulateInstructionLoongArch : public EmulateInstruction {
 
   std::optional<RegisterInfo> GetRegisterInfo(lldb::RegisterKind reg_kind,
                                               uint32_t reg_num) override;
-  lldb::addr_t ReadPC(bool *success);
-  bool WritePC(lldb::addr_t pc);
   bool IsLoongArch64() { return m_arch_subtype == llvm::Triple::loongarch64; }
   bool TestExecute(uint32_t inst);
 
diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index badc7ba36f011..c22d5bbdb6924 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -650,9 +650,10 @@ std::optional<DecodeResult> EmulateInstructionRISCV::Decode(uint32_t inst) {
   for (const InstrPattern &pat : PATTERNS) {
     if ((inst & pat.type_mask) == pat.eigen &&
         (inst_type & pat.inst_type) != 0) {
-      LLDB_LOGF(
-          log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was decoded to %s",
-          __FUNCTION__, inst, m_addr, pat.name);
+      LLDB_LOGF(log,
+                "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64
+                ") was decoded to %s",
+                __FUNCTION__, inst, m_addr, pat.name);
       auto decoded = is_16b ? pat.decode(try_rvc) : pat.decode(inst);
       return DecodeResult{decoded, inst, is_16b, pat};
     }
@@ -1649,21 +1650,6 @@ bool EmulateInstructionRISCV::ReadInstruction() {
   return true;
 }
 
-std::optional<addr_t> EmulateInstructionRISCV::ReadPC() {
-  bool success = false;
-  auto addr = ReadRegisterUnsigned(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC,
-                                   LLDB_INVALID_ADDRESS, &success);
-  return success ? std::optional<addr_t>(addr) : std::nullopt;
-}
...
[truncated]

@dlav-sc
Copy link
Contributor Author

dlav-sc commented Jun 27, 2025

@DavidSpickett, @JDevlieghere TestSoftwareStep should only run on riscv targets. I've updated the test's regex (see d265e6f) , so I hope it won't fail anymore.

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

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

Please describe in the PR description the changes made since it was reverted (even as trivial as they are here).

This helps git archaeology a bit, and users who might want to check the state of a feature later and might be confused by a revert/reland sequence.

Otherwise, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants