add comments

Author: dlav-sc

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>
 
@@ -44,12 +46,18 @@ class SingleStepBreakpointLocationsPredictor {
 
   virtual BreakpointLocations GetBreakpointLocations(Status &status);
 
-  virtual unsigned GetBreakpointSize(lldb::addr_t, Status &) { return 4; }
+  virtual llvm::Expected<unsigned> GetBreakpointSize(lldb::addr_t bp_addr) {
+    return 4;
+  }
 
   virtual ~SingleStepBreakpointLocationsPredictor() = default;
 
 protected:
-  lldb::addr_t GetSequentiallyNextInstructionPC(Status &error);
+  // 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);
@@ -547,6 +555,13 @@ class EmulateInstruction : public PluginInterface {
 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));

lldb/source/Core/EmulateInstruction.cpp

@@ -620,7 +620,7 @@ 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 = GetSequentiallyNextInstructionPC(status);
+    lldb::addr_t next_pc = GetNextInstructionAddress(status);
     return BreakpointLocations{next_pc};
   }
 
@@ -637,8 +637,7 @@ SingleStepBreakpointLocationsPredictor::GetBreakpointLocations(Status &status) {
   return BreakpointLocations{next_pc};
 }
 
-lldb::addr_t
-SingleStepBreakpointLocationsPredictor::GetSequentiallyNextInstructionPC(
+lldb::addr_t SingleStepBreakpointLocationsPredictor::GetNextInstructionAddress(
     Status &error) {
   auto instr_size = m_emulator_up->GetLastInstrSize();
   if (!instr_size) {
@@ -672,10 +671,10 @@ SingleStepBreakpointLocationsPredictor::GetBreakpointLocationAddress(
   }
 
   if (entry_pc == pc) {
-    // Emulate instruction failed and it haven't changed PC. Advance PC with
+    // 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 a not supported instruction which don't modify PC.
+    // likely caused by an unsupported instruction which does not modify PC.
     return pc + m_emulator_up->GetOpcode().GetByteSize();
   }
 

lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp

@@ -14472,13 +14472,15 @@ bool EmulateInstructionARM::CreateFunctionEntryUnwind(UnwindPlan &unwind_plan) {
   return true;
 }
 
-unsigned ARMSingleStepBreakpointLocationsPredictor::GetBreakpointSize(
-    lldb::addr_t bp_addr, Status &error) {
+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)
-    error = Status("Reading flags failed!");
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Reading flags failed!");
 
   return (flags & 0x20) ? /* Thumb mode */ 2 : /* Arm mode */ 4;
 }

lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h

@@ -23,7 +23,7 @@ class ARMSingleStepBreakpointLocationsPredictor
       std::unique_ptr<EmulateInstruction> emulator_up)
       : SingleStepBreakpointLocationsPredictor{std::move(emulator_up)} {}
 
-  unsigned GetBreakpointSize(lldb::addr_t bp_addr, Status &error) override;
+  llvm::Expected<unsigned> GetBreakpointSize(lldb::addr_t bp_addr) override;
 };
 
 // ITSession - Keep track of the IT Block progression.

lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp

@@ -1809,14 +1809,15 @@ RISCVSingleStepBreakpointLocationsPredictor::GetBreakpointLocations(
               "RISCVSingleStepBreakpointLocationsPredictor::%s: can't find "
               "corresponding load reserve insturuction",
               __FUNCTION__);
-    return {*pc + 4};
+    return {*pc + inst->is_rvc ? 2u : 4u};
   }
 
   return SingleStepBreakpointLocationsPredictor::GetBreakpointLocations(status);
 }
 
-unsigned RISCVSingleStepBreakpointLocationsPredictor::GetBreakpointSize(
-    lldb::addr_t bp_addr, Status &error) {
+llvm::Expected<unsigned>
+RISCVSingleStepBreakpointLocationsPredictor::GetBreakpointSize(
+    lldb::addr_t bp_addr) {
   EmulateInstructionRISCV *riscv_emulator =
       static_cast<EmulateInstructionRISCV *>(m_emulator_up.get());
 

lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h

@@ -29,7 +29,7 @@ class RISCVSingleStepBreakpointLocationsPredictor
 
   BreakpointLocations GetBreakpointLocations(Status &status) override;
 
-  unsigned GetBreakpointSize(lldb::addr_t bp_addr, Status &error) override;
+  llvm::Expected<unsigned> GetBreakpointSize(lldb::addr_t bp_addr) override;
 
 private:
   static bool FoundLoadReserve(const RISCVInst &inst) {

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

@@ -131,11 +131,11 @@ Status NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping(
     return error;
 
   for (auto &&bp_addr : bp_locations) {
-    unsigned bp_size = bp_locaions_predictor->GetBreakpointSize(bp_addr, error);
-    if (error.Fail())
-      return error;
+    auto bp_size = bp_locaions_predictor->GetBreakpointSize(bp_addr);
+    if (auto err = bp_size.takeError())
+      return Status(toString(std::move(err)));
 
-    error = SetSoftwareBreakpoint(bp_addr, bp_size, process);
+    error = SetSoftwareBreakpoint(bp_addr, *bp_size, process);
     if (error.Fail())
       return error;
 

lldb/test/API/riscv/step/TestSoftwareStep.py

@@ -1,5 +1,5 @@
 """
-Test software step-inst
+Test software step-inst, also known as instruction level single step, in risc-v atomic sequence.
 """
 
 import lldb
@@ -9,11 +9,13 @@
 
 
 class TestSoftwareStep(TestBase):
-    @skipIf(archs=no_match(re.compile("rv*")))
-    def test_cas(self):
-        self.build()
+    def do_sequence_test(filename, bkpt_name):
+        source_file = filename + ".c"
+        exe_file = filename + ".x"
+
+        self.build(dictionary={"C_SOURCES": source_file, "EXE": exe_file})
         (target, process, cur_thread, bkpt) = lldbutil.run_to_name_breakpoint(
-            self, "cas"
+            self, bkpt_name, exe_name=exe_file
         )
         entry_pc = cur_thread.GetFrameAtIndex(0).GetPC()
 
@@ -24,67 +26,49 @@ def test_cas(self):
         )
 
         pc = cur_thread.GetFrameAtIndex(0).GetPC()
-        self.assertTrue((pc - entry_pc) > 0x10)
 
-    @skipIf(archs=no_match(re.compile("rv*")))
-    def test_branch_cas(self):
-        self.build(dictionary={"C_SOURCES": "branch.c", "EXE": "branch.x"})
-        (target, process, cur_thread, bkpt) = lldbutil.run_to_name_breakpoint(
-            self, "branch_cas", exe_name="branch.x"
-        )
-        entry_pc = cur_thread.GetFrameAtIndex(0).GetPC()
+        return pc - entry_pc
 
-        self.runCmd("thread step-inst")
-        self.expect(
-            "thread list",
-            substrs=["stopped", "stop reason = instruction step into"],
-        )
+    @skipIf(archs=no_match(re.compile("rv*")))
+    def test_cas(self):
+        """
+        This test verifies LLDB instruction step handling of a proper lr/sc pair.
+        """
+        difference = do_sequence_test("main", "cas")
+        self.assertTrue(difference > 0x10)
 
-        pc = cur_thread.GetFrameAtIndex(0).GetPC()
-        self.assertTrue((pc - entry_pc) > 0x10)
+    @skipIf(archs=no_match(re.compile("rv*")))
+    def test_branch_cas(self):
+        """
+        LLDB cannot predict the actual state of registers within a critical section (i.e., inside an atomic
+        sequence). Therefore, it should identify all forward branches inside the atomic sequence and set
+        breakpoints at every jump address that lies beyond the end of the sequence (after the sc instruction).
+        This ensures that if any such branch is taken, execution will pause at its target address.
+
+        This test includes an lr/sc sequence containing an active forward branch with a jump address located
+        after the end of the atomic section. LLDB should correctly stop at this branch's target address. The
+        test is nearly identical to the previous one, except for the branch condition, which is inverted and
+        will result in a taken jump.
+        """
+        difference = do_sequence_test("branch", "branch_cas")
+        self.assertTrue(difference > 0x10)
 
     @skipIf(archs=no_match(re.compile("rv*")))
     def test_incomplete_sequence_without_lr(self):
-        self.build(
-            dictionary={
-                "C_SOURCES": "incomplete_sequence_without_lr.c",
-                "EXE": "incomplete_lr.x",
-            }
-        )
-        (target, process, cur_thread, bkpt) = lldbutil.run_to_name_breakpoint(
-            self, "incomplete_cas", exe_name="incomplete_lr.x"
-        )
-        entry_pc = cur_thread.GetFrameAtIndex(0).GetPC()
-
-        self.runCmd("thread step-inst")
-
-        self.expect(
-            "thread list",
-            substrs=["stopped", "stop reason = instruction step into"],
-        )
-
-        pc = cur_thread.GetFrameAtIndex(0).GetPC()
-        self.assertTrue((pc - entry_pc) == 0x4)
+        """
+        This test verifies the behavior of a standalone sc instruction without a preceding lr. Since the sc
+        lacks the required lr pairing, LLDB should treat it as a non-atomic store rather than part of an
+        atomic sequence.
+        """
+        difference = do_sequence_test("incomplete_sequence_without_lr", "incomplete_cas")
+        self.assertTrue(difference == 0x4)
 
     @skipIf(archs=no_match(re.compile("rv*")))
     def test_incomplete_sequence_without_sc(self):
-        self.build(
-            dictionary={
-                "C_SOURCES": "incomplete_sequence_without_sc.c",
-                "EXE": "incomplete_sc.x",
-            }
-        )
-        (target, process, cur_thread, bkpt) = lldbutil.run_to_name_breakpoint(
-            self, "incomplete_cas", exe_name="incomplete_sc.x"
-        )
-        entry_pc = cur_thread.GetFrameAtIndex(0).GetPC()
-
-        self.runCmd("thread step-inst")
-
-        self.expect(
-            "thread list",
-            substrs=["stopped", "stop reason = instruction step into"],
-        )
-
-        pc = cur_thread.GetFrameAtIndex(0).GetPC()
-        self.assertTrue((pc - entry_pc) == 0x4)
+        """
+        This test checks the behavior of a standalone lr instruction without a subsequent sc. Since the lr
+        lacks its required sc counterpart, LLDB should treat it as a non-atomic load rather than part of an
+        atomic sequence.
+        """
+        difference = do_sequence_test("incomplete_sequence_without_sc", "incomplete_cas")
+        self.assertTrue(difference == 0x4)