[lldb][RISCV] Fix float load and stores in RISC-V emulator

[sga-sc]

This patch fixes 2 fundamental problems in emulating FLW, FSW, FLD and FSD instructions.

1. Instructions immediate wasn't sign extended
2. Store instructions always wrote for 64 bits to memory

Also this patch fixes 2 lldb tests for RISC-V: TestThreadJump.py and TestBreakpointHitCount.py

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-lldb

Author: Georgiy Samoylov (sga-sc)

Changes

This patch fixes 2 fundamental problems in emulating FLW, FSW, FLD and FSD instructions.

1. Instructions immediate wasn't sign extended
2. Store instructions always wrote for 64 bits to memory

Also this patch fixes 2 lldb tests for RISC-V: TestThreadJump.py and TestBreakpointHitCount.py

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp (+18-12)

diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 5c1b7d4943b3f..2957cb716041d 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -1328,32 +1328,36 @@ class Executor {
         m_emu, inst, 8, ZextD,
         [](uint64_t a, uint64_t b) { return std::max(a, b); });
   }
-  template <typename T>
-  bool F_Load(T inst, const fltSemantics &(*semantics)(),
-              unsigned int numBits) {
+  template <typename I, typename T>
+  bool F_Load(I inst, const fltSemantics &(*semantics)()) {
     return transformOptional(inst.rs1.Read(m_emu),
                              [&](auto &&rs1) {
-                               uint64_t addr = rs1 + uint64_t(inst.imm);
-                               uint64_t bits = *m_emu.ReadMem<uint64_t>(addr);
+                               uint64_t addr =
+                                   rs1 + uint64_t(SignExt(inst.imm));
+                               uint64_t bits = *m_emu.ReadMem<T>(addr);
+                               unsigned numBits = sizeof(T) * 8;
                                APFloat f(semantics(), APInt(numBits, bits));
                                return inst.rd.WriteAPFloat(m_emu, f);
                              })
         .value_or(false);
   }
-  bool operator()(FLW inst) { return F_Load(inst, &APFloat::IEEEsingle, 32); }
-  template <typename T> bool F_Store(T inst, bool isDouble) {
+  bool operator()(FLW inst) {
+    return F_Load<FLW, uint32_t>(inst, &APFloat::IEEEsingle);
+  }
+  template <typename I, typename T> bool F_Store(I inst, bool isDouble) {
     return transformOptional(zipOpt(inst.rs1.Read(m_emu),
                                     inst.rs2.ReadAPFloat(m_emu, isDouble)),
                              [&](auto &&tup) {
                                auto [rs1, rs2] = tup;
-                               uint64_t addr = rs1 + uint64_t(inst.imm);
+                               uint64_t addr =
+                                   rs1 + uint64_t(SignExt(inst.imm));
                                uint64_t bits =
                                    rs2.bitcastToAPInt().getZExtValue();
-                               return m_emu.WriteMem<uint64_t>(addr, bits);
+                               return m_emu.WriteMem<T>(addr, bits);
                              })
         .value_or(false);
   }
-  bool operator()(FSW inst) { return F_Store(inst, false); }
+  bool operator()(FSW inst) { return F_Store<FSW, uint32_t>(inst, false); }
   std::tuple<bool, APFloat> FusedMultiplyAdd(APFloat rs1, APFloat rs2,
                                              APFloat rs3) {
     auto opStatus = rs1.fusedMultiplyAdd(rs2, rs3, m_emu.GetRoundingMode());
@@ -1616,8 +1620,10 @@ class Executor {
   bool operator()(FCVT_S_LU inst) {
     return FCVT_f2i(inst, &Rs::Read, APFloat::IEEEsingle());
   }
-  bool operator()(FLD inst) { return F_Load(inst, &APFloat::IEEEdouble, 64); }
-  bool operator()(FSD inst) { return F_Store(inst, true); }
+  bool operator()(FLD inst) {
+    return F_Load<FLD, uint64_t>(inst, &APFloat::IEEEdouble);
+  }
+  bool operator()(FSD inst) { return F_Store<FSD, uint64_t>(inst, true); }
   bool operator()(FMADD_D inst) { return FMA(inst, true, 1.0f, 1.0f); }
   bool operator()(FMSUB_D inst) { return FMA(inst, true, 1.0f, -1.0f); }
   bool operator()(FNMSUB_D inst) { return FMA(inst, true, -1.0f, 1.0f); }

[sga-sc]

@DavidSpickett, please, take a look

[DavidSpickett]

Load/store size changing with precision makes sense. That's how I'd expect them to act.

Can you cite some part of the spec that says the immediate should be sign extended?

(maybe this is second nature to RISC-V experts but I keep my distance from the details usually)

[sga-sc]

Quote from RISC-V ISA, 2.6. Load and Store Instructions:

Load and store instructions transfer a value between the registers and memory. Loads are encoded in the Itype format and stores are S-type. The effective address is obtained by adding register rs1 to the signextended 12-bit offset. Loads copy a value from memory to register rd. Stores copy the value in register rs2 to memory.

[DavidSpickett]

LGTM

[sga-sc]

Could you merge it, please? I don't have merge rights yet.

[lenary]

Is this the right width of sign extension?

The function only seems to cast from a uint32_t to an int32_t, which iirc will sign extend from the 32nd bit, not the 12th bit? Nowhere between decode and emulation do I see something specifically calling out 12, but I'm not familiar with LLDB, so I might have missed somewhere.

I'm not even sure that LoadStoreAddr(EmulateInstructionRISCV &emulator, I inst) is correct either.

[DavidSpickett]

ping @sga-sc

[DavidSpickett]

There is SignExtend32 in llvm/include/llvm/Support/MathExtras.h that could handle this.

[sga-sc]

Should I create a new PR which fixes sign extention here and in LoadStoreAddr(EmulateInstructionRISCV &emulator, I inst)?

[DavidSpickett]

Assuming you agree that this is an issue, yes please. From this snippet, it does seem incorrect but I haven't looked at the supporting code myself.