[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Lu Weining via Phabricator via lldb-commits]

SixWeining added a comment.

I'm not sure whether lldb should follow llvm coding standard.



================
Comment at: lldb/source/Plugins/Instruction/LoongArch/CMakeLists.txt:7-8
+    lldbInterpreter
+    lldbSymbol
+    lldbPluginProcessUtility
+  LINK_COMPONENTS
----------------
It's better to sort alphabetically.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:37
+  static EmulateInstructionLoongArch::Opcode g_opcodes[] = {
+      {0x00000000, 0x00000000, &EmulateInstructionLoongArch::EmulateNonJMP,
+       "NonJMP"}};
----------------
Will the mask be changed in future? If so, better to leave a `FIXME` or `TODO`. If not, the following `for` loop always return the `NonJMP` opcode?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:39
+       "NonJMP"}};
+  static const size_t num_ppc_opcodes = std::size(g_opcodes);
+
----------------
loongarch?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:41
+
+  for (size_t i = 0; i < num_ppc_opcodes; ++i) {
+    if ((g_opcodes[i].mask & inst) == g_opcodes[i].value)
----------------
useless `{`


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:44
+      return &g_opcodes[i];
+  }
+  return nullptr;
----------------
Ditto.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:49
+bool EmulateInstructionLoongArch::EvaluateInstruction(uint32_t options) {
+  uint32_t inst_size = m_opcode.GetByteSize();
+  uint32_t inst = m_opcode.GetOpcode32();
----------------
Could it be `InstSize`? https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:66-68
+  success = (this->*opcode_data->callback)(inst);
+  if (!success)
+    return false;
----------------
  if (!(this->*opcode_data->callback)(inst))
    return false;


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:75-78
+    if (new_pc == old_pc) {
+      if (!WritePC(old_pc + inst_size))
+        return false;
+    }
----------------
```
    if (new_pc == old_pc && !WritePC(old_pc + inst_size)
        return false;
```


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:84
+bool EmulateInstructionLoongArch::ReadInstruction() {
+
+  bool success = false;
----------------
Remove useless blank line.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:174
+  if (EmulateInstructionLoongArch::SupportsThisInstructionType(inst_type) &&
+      SupportsThisArch(arch)) {
+    return new EmulateInstructionLoongArch(arch);
----------------
useless `{`


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:176
+    return new EmulateInstructionLoongArch(arch);
+  }
+
----------------
Ditto


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:28-36
+    switch (inst_type) {
+    case eInstructionTypePCModifying:
+      return true;
+    case eInstructionTypeAny:
+    case eInstructionTypePrologueEpilogue:
+    case eInstructionTypeAll:
+      return false;
----------------
Do you plan to support `eInstructionTypeAny` `eInstructionTypePrologueEpilogue` `eInstructionTypeAll` in future? If so, had better leave a `TODO` here. Otherwise this function can be simplied to:
```
  return inst_type == eInstructionTypePCModifying;
```


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:66
+                                               uint32_t reg_num) override;
+  lldb::addr_t ReadPC(bool *success);
+  bool WritePC(lldb::addr_t pc);
----------------
Could it be a private function?


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:79
+
+  bool EmulateNonJMP(uint32_t inst);
+};
----------------
Is it a override function?


================
Comment at: lldb/tools/lldb-server/SystemInitializerLLGS.cpp:51
+#define LLDB_TARGET_LoongArch
+#include "Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h"
+#endif
----------------
Sort alphabetically?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139158