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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
