[Lldb-commits] [PATCH] D139833: [LLDB][LoongArch] Add branch instructions for EmulateInstructionLoongArch

[Lu Weining via Phabricator via lldb-commits]

SixWeining added a comment.

1. Does the directory `lldb/source/Plugins/Instruction/LoongArch/` target both `LoongArch64` and `LoongArch32`?
2. Will you handle floating pointer branching instructions `bceqz` and `bcnez` in future?



================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:205
+bool EmulateInstructionLoongArch::EmulateBEQZ(uint32_t inst) {
+  uint64_t next_pc, imm_sign_extend;
+  bool success = false;
----------------
On LoongArch32, it should be `uint32_t`, right?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:208
+  uint32_t rj = Bits32(inst, 9, 5);
+  uint64_t rj_val;
+  uint64_t pc = ReadPC(&success);
----------------
On LoongArch32, it should be `uint32_t`, right?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:213
+  uint32_t offs21 = Bits32(inst, 25, 10) + (Bits32(inst, 4, 0) << 16);
+  rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
+  if (rj_val == 0) {
----------------
Should `success` be checked after call ?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:215
+  if (rj_val == 0) {
+    imm_sign_extend = llvm::SignExtend64<23>(offs21 << 2);
+    next_pc = pc + imm_sign_extend;
----------------
On LoongArch32, it should be `llvm::SignExtend32`, right?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:258
+    return false;
+  ReadRegister(eRegisterKindLLDB, rj, value);
+  imm_sign_extend = llvm::SignExtend64<18>(Bits32(inst, 25, 10) << 2);
----------------
Return value should be checked ?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:269
+  bool success = false;
+  uint64_t pc = ReadPC(&success);
+  uint32_t offs26 = Bits32(inst, 25, 10) + (Bits32(inst, 9, 0) << 16);
----------------
Should it be checked ?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:306
+    return false;
+  rj_val = ReadRegisterUnsigned(eRegisterKindLLDB, rj, 0, &success);
+  rd_val = ReadRegisterUnsigned(eRegisterKindLLDB, rd, 0, &success);
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139833/new/

https://reviews.llvm.org/D139833