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

[dlav-sc]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (dlav-sc)

Changes

lldb-server didn't consider that LR instruction may be followed not only by BNE instruction (takes 4 bytes), but by BNEZ too, which is a compressed instruction (takes 2 bytes). As a result, after BNEZ lldb-server reseived an incorrect PC value and couldn't parse the next instruction opcode.

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp (+11-6)

diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index badc7ba36f011..e4de28d97d1ff 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -349,9 +349,10 @@ bool AtomicSequence(EmulateInstructionRISCV &emulator) {
   if (!inst || (!std::holds_alternative<LR_W>(inst->decoded) &&
                 !std::holds_alternative<LR_D>(inst->decoded)))
     return false;
+  current_pc += 4;
 
-  // The second instruction should be BNE to exit address
-  inst = emulator.ReadInstructionAt(current_pc += 4);
+      // The second instruction should be BNE or C.BNEZ to exit address
+      inst = emulator.ReadInstructionAt(current_pc);
   if (!inst || !std::holds_alternative<B>(inst->decoded))
     return false;
   auto bne_exit = std::get<B>(inst->decoded);
@@ -359,15 +360,18 @@ bool AtomicSequence(EmulateInstructionRISCV &emulator) {
     return false;
   // save the exit address to check later
   const auto exit_pc = current_pc + SextW(bne_exit.imm);
+  // Can be C.BNEZ (2 bytes) or BNE (4 bytes)
+  current_pc += inst->is_rvc ? 2 : 4;
 
   // The third instruction should be SC.W or SC.D
-  inst = emulator.ReadInstructionAt(current_pc += 4);
+  inst = emulator.ReadInstructionAt(current_pc);
   if (!inst || (!std::holds_alternative<SC_W>(inst->decoded) &&
                 !std::holds_alternative<SC_D>(inst->decoded)))
     return false;
+  current_pc += 4;
 
-  // The fourth instruction should be BNE to entry address
-  inst = emulator.ReadInstructionAt(current_pc += 4);
+      // The fourth instruction should be BNE or C.BNEZ to entry address
+      inst = emulator.ReadInstructionAt(current_pc);
   if (!inst || !std::holds_alternative<B>(inst->decoded))
     return false;
   auto bne_start = std::get<B>(inst->decoded);
@@ -375,8 +379,9 @@ bool AtomicSequence(EmulateInstructionRISCV &emulator) {
     return false;
   if (entry_pc != current_pc + SextW(bne_start.imm))
     return false;
+  // Can be C.BNEZ (2 bytes) or BNE (4 bytes)
+  current_pc += inst->is_rvc ? 2 : 4;
 
-  current_pc += 4;
   // check the exit address and jump to it
   return exit_pc == current_pc && emulator.WritePC(current_pc);
 }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JDevlieghere]

CC @SEmmmer

[DavidSpickett]

Please update the title/description to mention that this is within atomic sequences. I thought this was some kind of fused instruction pair/delay slot type deal but it's not that.

Looks fine at a high level, but I always like to get an architecture expert in just in case. @asb you or someone else you know of? Confirming the claims about the format of the sequence would be enough I think.

[dlav-sc]

I'm going to land this by the end of the week. If you have any concerns, please let me know.

[asb]

My apologies for missing this before. It is indeed completely valid to have a compressed branch immediately after the lr.w or sc.w and remain compliant with the forward progress guarantee. I feel I'm lacking a bit of higher level understanding of what this code is trying to do it, because:

• It matches only one of many possible lr.[wd]/sc.[wd] sequences. For instance we'll have sequences of other integer instructions between the lr and sc for things like narrower than 32-bit cmpxchg (see llvm/test/CodeGen/RISCV/atomic-cmpxchg.ll)
• Why isn't there a a call to m_emu.WriteMem in the implementation?

[dlav-sc]

It matches only one of many possible lr.[wd]/sc.[wd] sequences.

I was confused too when I saw this for the first time. I expected to see a processing a sequence of integer instructions between lr and sc. This patch was enough to fix my issue, so I decided to bring it here as is. However, I'm now thinking I could try to do something more comprehensive and support any possible sequence of instructions.

Why isn't there a a call to m_emu.WriteMem in the implementation?

From what I understand, it's not a real simulator. This is used in lldb for software stepping to determine where to set a breakpoint. lldb needs to emulate the instruction to obtain next PC.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[dlav-sc]

@DavidSpickett, @asb could you take another look at this patch, please?

I've added support for instruction step for any possible riscv atomic sequence (or at least I believe so). Additionally, I've made some minor refactoring to the software single-step logic.

[DavidSpickett]

If I'm someone looking to find out if a particular atomic sequence is valid, where do I look, what's the ABI / ISA doc that tells us that?

I don't literally want to do that now, but someone in future might, so if there is one, please add links to it in the PR message and the top of the test file.

(if there is no stable link, the doc's name is enough)

[DavidSpickett]

LGTM

[dlav-sc]

LGTM

Thank you for the review! I'm going to land it by the end of the day.

[slydiman]

lldb-remote-linux-ubuntu is broken after this patch.
Please update the test conditions to skip riscv tests on non-riscv platforms.
Note r matches in aarch64 for rv* regex. https://regex101.com/r/qDLwI2/1
Update rv* with ^rv.* must be enough.

[JDevlieghere]

GreenDragon too: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/28268/

[DavidSpickett]

Yeah to my surprise it's re.search not re.match. Makes sense in hindsight though.