This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] [LoongArch] Add minimal LoongArch support
ClosedPublic

Authored by seehearfeel on Oct 23 2022, 11:24 PM.

Details

Summary

Add as little code as possible to allow compiling lldb on LoongArch.
Actual functionality will be implemented later.

Diff Detail

Event Timeline

seehearfeel created this revision.Oct 23 2022, 11:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2022, 11:24 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
seehearfeel requested review of this revision.Oct 23 2022, 11:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2022, 11:24 PM
seehearfeel edited the summary of this revision. (Show Details)Oct 23 2022, 11:30 PM
SixWeining added inline comments.Oct 23 2022, 11:30 PM
lldb/source/Plugins/Process/Linux/CMakeLists.txt
11

Had better use git diff -U999999 to generate the patch so that we can get more context.
See: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Use git diff -U999999 to generate the patch so that we can get more context suggested by Weining.

xen0n retitled this revision from [LLDB] [LoongArch] Fix build errors to [LLDB] [LoongArch] Add minimal LoongArch support.Oct 24 2022, 12:03 AM
xen0n edited the summary of this revision. (Show Details)

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!

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.

tschuett added inline comments.
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp
44

Stupid question: why is the register count 0?

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.h
27

Firstly, I prefer ènum class. Second, it is unused?

seehearfeel marked an inline comment as done.Oct 24 2022, 2:37 AM

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!

OK, thank you.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp
44

It is just a stub function here to build successfully, actually it should return k_num_gpr_registers
after the following header file is added in the later patch.

lldb/source/Plugins/Process/Utility/lldb-loongarch-register-enums.h

SixWeining added a comment.EditedOct 24 2022, 2:41 AM

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@loongson.cn) is the LoongArch port maintainer of gdb.

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

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.h
9

Seems #if __loongarch_grlen == 64 is enough? But I'm fine with both.

25

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

seehearfeel added inline comments.Oct 24 2022, 2:45 AM
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.h
27

Now, GPRegSet and FPRegSet are not used,
I will remove them and update the diff in this commit,
and use
enum class { GPRegSet = 0, FPRegSet };
in the later patch, thank you.

What's more, AFAIK, @seehearfeel (yangtiezhu@loongson.cn) is the LoongArch port maintainer of gdb.
Regarding the public buildbot, I'm trying to set it up and maybe we can see it in one or two weeks.

Great!

seehearfeel added inline comments.Oct 24 2022, 4:34 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.h
9

Let us leave it as it is like riscv does, thank you.

25

OK, will do it and then update.

SixWeining added a comment.EditedOct 24 2022, 5:44 PM

Build is OK on my local LoongArch machine with 299 failed tests and 2 timed out tests. Just as the summary says, I think this is expected.

seehearfeel edited the summary of this revision. (Show Details)
seehearfeel set the repository for this revision to rG LLVM Github Monorepo.

Remove GPRegSet and FPRegSet definitions which are not used in this commit.
Use git clang-format to format the patch properly.

DavidSpickett accepted this revision.Oct 25 2022, 2:19 AM

This LGTM from the lldb side.

Feel free to upload a series of patches in future if you want. You don't have to do one at a time, just update the whole stack as needed. Plus it gives the reviewers some context of what the changes are working towards.

Do you have commit access? If not it's fine to wait until you have (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access), or someone else can land on your behalf. Just provide a name and email address you'd like to be on the commit.

This revision is now accepted and ready to land.Oct 25 2022, 2:19 AM
SixWeining added inline comments.Oct 25 2022, 3:14 AM
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.h
27

Why is this structure and below FPR defined like this? Do you intent to keep the layout same as some structures of Linux? If yes, is it necessary?

xry111 added inline comments.Oct 25 2022, 3:42 AM
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.h
27

It seems necessary because lldb invokes ptrace system call which directly stores into the pointer returned by GetGPRBuffer.

This LGTM from the lldb side.

OK, thank you.

Feel free to upload a series of patches in future if you want. You don't have to do one at a time, just update the whole stack as needed. Plus it gives the reviewers some context of what the changes are working towards.

I am a newcomer, here are some of my thoughts:
(1) Add the minimal changes to fix the build errors.
(2) Submit other more patches step by step to make
the basic command "run", "breakpoint", "next" ...
can be used to debug, single patch or patch series.
(3) Add more code to make more commands work well.

Do you have commit access? If not it's fine to wait until you have (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access), or someone else can land on your behalf. Just provide a name and email address you'd like to be on the commit.

I have no commit access now, @SixWeining luweining@loongson.cn
can land on my behalf, here are my name and email:
Tiezhu Yang
yangtiezhu@loongson.cn

SixWeining accepted this revision.Oct 25 2022, 4:30 AM

LGTM for LoongArch related changes.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.h
27

Make sense. Thanks.

I am a newcomer, here are some of my thoughts:
(1) Add the minimal changes to fix the build errors.
(2) Submit other more patches step by step to make
the basic command "run", "breakpoint", "next" ...
can be used to debug, single patch or patch series.
(3) Add more code to make more commands work well.

Sounds like a good approach to me.

I believe the riscv effort focused on parts of the test suite in turn, sounds like you'll do the same.

can land on my behalf

Will do.

This revision was automatically updated to reflect the committed changes.

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

FYI. D138672 is the review for adding LoongArch buildbot.

Nice, glad to see that.