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

[Hui Li via Phabricator via lldb-commits]

lh03061238 added inline comments.


================
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;
----------------
SixWeining wrote:
> On LoongArch32, it should be `uint32_t`, right?
will modify to distinguish between LoongArch64 and LoongArch32 operations.


================
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) {
----------------
SixWeining wrote:
> Should `success` be checked after call ?
yes, will modify it .
thank you.


================
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);
----------------
SixWeining wrote:
> Return value should be checked ?
yes, will modify it .
thank you.


================
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);
----------------
SixWeining wrote:
> Should it be checked ?
yes, will modify it .
thank you.


================
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);
----------------
SixWeining wrote:
> Ditto.
will modify all .
thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139833