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

[WÁNG Xuěruì via Phabricator via lldb-commits]

xen0n added a comment.

I think you can remove the example from your commit message altogether, it's giving essentially no information while making the text very lengthy. Only keeping the first paragraph should be enough.



================
Comment at: lldb/source/Plugins/Instruction/CMakeLists.txt:7
 add_subdirectory(RISCV)
+add_subdirectory(LoongArch)
----------------
The list is sorted alphabetically so this should come before MIPS.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:10
+#ifndef LLDB_SOURCE_PLUGINS_INSTRUCTION_LoongArch_EMULATEINSTRUCTIONLoongArch_H
+#define LLDB_SOURCE_PLUGINS_INSTRUCTION_LoongArch_EMULATEINSTRUCTIONLoongArch_H
+
----------------
Use `ALL_CAPS` for this include guard symbol.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:24
+  static llvm::StringRef GetPluginDescriptionStatic() {
+    return "Emulate instructions for the loongarch architecture.";
+  }
----------------
Use "LoongArch" here because it's intended to be a human-readable string.


================
Comment at: lldb/tools/lldb-server/CMakeLists.txt:57
       lldbPluginInstructionRISCV
+      lldbPluginInstructionLoongArch
       ${LLDB_SYSTEM_LIBS}
----------------
Put before MIPS for alphabetical order too.


================
Comment at: lldb/tools/lldb-server/SystemInitializerLLGS.cpp:49
 
+#if defined(__loongarch) || defined(__loongarch64)
+#define LLDB_TARGET_LoongArch
----------------
There's no `__loongarch`, only `__loongarch__` (we're more classical than RISC-V in this regard). Instead simply use `#if defined(__loongarch__)` as the code enabled by this condition isn't really caring its bitness.


================
Comment at: lldb/tools/lldb-server/SystemInitializerLLGS.cpp:74-76
+#if defined(LLDB_TARGET_LoongArch)
+  EmulateInstructionLoongArch::Initialize();
+#endif
----------------
Alphabetize this chunk too.


================
Comment at: lldb/tools/lldb-server/SystemInitializerLLGS.cpp:96-98
+#if defined(LLDB_TARGET_LoongArch)
+  EmulateInstructionLoongArch::Terminate();
+#endif
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139158