[Lldb-commits] [PATCH] D136578: [LLDB] [LoongArch] Add minimal LoongArch support

Mon Oct 24 02:41:45 PDT 2022

SixWeining added a comment.

In D136578#3878744 <https://reviews.llvm.org/D136578#3878744>, @DavidSpickett wrote:

> Always good to see another architecture in lldb.
>
> A few points up front.
>
> Changes like this are fine and we can review them, but can only be landed once we see that they'll be built on. So please stack changes so that we can see what is upcoming. https://llvm.org/docs/Phabricator.html#creating-a-patch-series
>
> Do you have a plan to test this configuration? I don't see any public buildbots for Loongson (please correct me if I am wrong) but that is not a requirement as long as you have some testing plan (RISCV doesn't have a public bot, as one example).
>
> Do you have community members who can review these changes for architectural correctness? Speaking just for myself I am happy to review bits of lldb machinery but when it comes to architecture details I am not, and will not, be an expert in Loongson.
>
> I'm ok accepting patches without a second reviewer, but I advise for your own benefit to try to find someone who already knows the details of Loongson.

Hi @DavidSpickett, thanks for your advice. I (as the LoongArch backend code owner) can review these changes for architectural correctness. And I think other LoongArch community members (non Loongson employees) also can take it. Especially @xen0n and @xry111 who are quite familar with LoongArch.

What's more, AFAIK, @seehearfeel (yangtiezhu at loongson.cn) is the LoongArch port maintainer of gdb <https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/MAINTAINERS;h=52db8cb8f70f0f67d8bced5ac02f5fde88690bba;hb=HEAD#l274>.

Regarding the public buildbot, I'm trying to set it up and maybe we can see it one or two weeks.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.h:24
+
+class NativeRegisterContextLinux_loongarch64 : public NativeRegisterContextLinux {
+public:
----------------
Pls limit the columns count to 80 which can be done by `clang-format`. See: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.h:9
+
+#if defined(__loongarch__) && __loongarch_grlen == 64
+
----------------
Seems `#if __loongarch_grlen == 64` is enough? But I'm fine with both.

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

https://reviews.llvm.org/D136578