[Lldb-commits] [PATCH] D140615: [LLDB][LoongArch] Delete the s9 register alias definition
xen0n added a comment. BTW do we have a way of fixing the bug while preserving the alias? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140615/new/ https://reviews.llvm.org/D140615 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D138407: [LLDB] Add LoongArch register definitions and operations
xen0n accepted this revision. xen0n added a comment. Also LGTM for the LoongArch bits. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138407/new/ https://reviews.llvm.org/D138407 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D138407: [LLDB] Add LoongArch register definitions and operations
xen0n added inline comments. Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h:98 +DEFINE_GPR64_ALT(r20, t8, LLDB_INVALID_REGNUM), +DEFINE_GPR64_ALT(r21, u0, LLDB_INVALID_REGNUM), +DEFINE_GPR64_ALT(r22, fp, LLDB_REGNUM_GENERIC_FP), SixWeining wrote: > `u0` is a unknown alias. Could we just use `DEFINE_GPR64`? FYI the `u0` name is [[ https://www.kernel.org/doc/html/latest/loongarch/introduction.html | a non-standard alias only seen in the Linux kernel ]]. It should be harmless to just support `r21` but not `u0`, much like how we don't support `v0/v1` any more. While at it, `s9` in addition to `fp` may be supported too. (Arguably `s9` is a better description of `r22` than `fp`, because FP usage can be disabled while generating code, in which case it's just another ordinary callee-saved register. But it seems some people believe so deeply that this usage is acceptable that the name persisted into the final ABI document...) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138407/new/ https://reviews.llvm.org/D138407 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137519: [LLDB] Add LoongArch software breakpoint trap opcode
xen0n accepted this revision. xen0n added a comment. This revision is now accepted and ready to land. You removed the formatting in the commit message and added an extra space in front of the patch title, all fixed for you. The changes look good LoongArch-wise now. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137519/new/ https://reviews.llvm.org/D137519 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137519: [LLDB][LoongArch] Add LoongArch software breakpoint trap code
xen0n added a comment.
I think eventually the upstream Linux would gain support for hardware
single-stepping, before the architecture is widely adopted (to be frank, if at
all), so it's probably not necessary to mention this point in the commit
message. Focus on the code changes being made right here.
Also the commit message is a bit unnatural (the English "take" feels very
different than Chinese "取" in this context). Let me tweak it a little bit and
please check if it's still conveying the message you intended.
Comment at: lldb/source/Host/common/NativeProcessProtocol.cpp:510
static const uint8_t g_riscv_opcode_c[] = {0x02, 0x90}; // c.ebreak
+ static const uint8_t g_loongarch_opcode[] = {0x05, 0x00, 0x2a, 0x00}; //
break
This is not a plain `break`, but rather `break 0x5` or `break BRK_SSTEPBP` as
the constant/magic 5 comes from Linux/LoongArch `asm/break.h`. Better reflect
this in the comment.
Comment at: lldb/source/Target/Platform.cpp:1945
+static const uint8_t g_loongarch_opcode[] = {0x05, 0x00, 0x2a,
+ 0x00}; // break
+trap_opcode = g_loongarch_opcode;
Same here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137519/new/
https://reviews.llvm.org/D137519
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137312: [LLDB] [LoongArch] Add loongarch64 case in ComputeHostArchitectureSupport()
xen0n accepted this revision. xen0n added a comment. This revision is now accepted and ready to land. Trivially correct. `CONFIG_COMPAT` isn't yet supported by upstream Linux though, but preemptively adding LoongArch here won't do any harm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137312/new/ https://reviews.llvm.org/D137312 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137057: [LLDB][LoongArch] Add LoongArch ArchSpec and subtype detection
xen0n added inline comments.
Comment at: lldb/include/lldb/Utility/ArchSpec.h:111
+ enum LOONGARCHSubType {
+eLOONGARCHSubType_unknown,
Should be `LoongArch`. It's that way everywhere else in LLVM land.
Comment at: lldb/source/Utility/ArchSpec.cpp:223
+{eByteOrderLittle, 4, 1, 4, llvm::Triple::loongarch32,
+ ArchSpec::eCore_loongarch32, "loongarch32"},
`min_opcode_byte_size` should be 4 too, all LoongArch insns are 32 bits long.
Same for `loongarch64`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137057/new/
https://reviews.llvm.org/D137057
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D136578: [LLDB] [LoongArch] Add minimal LoongArch support
xen0n added a comment. Hi, I've edited the summary and patch title for you. It's generally not necessary to add //that// much "politeness" when most of it is obvious from context (e.g. the fact that you're new face here, that there's obviously no LoongArch support in LLDB, and most of the methods being stubs). It didn't help that much of the text was in Chinglish either. As for the changes, they look reasonable to me, but as I haven't tested it out myself yet (and unable to, due to it being stub-only), I'll not give the approval myself this time. Thanks for your contribution and welcome! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136578/new/ https://reviews.llvm.org/D136578 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
