Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support linux-loong64 native debug #3685

Closed
wants to merge 2 commits into from
Closed

Conversation

yelvens
Copy link
Contributor

@yelvens yelvens commented Mar 12, 2024

LoongArch is a new RISC ISA, which is independently designed by Loongson Technology.

LoongArch includes a reduced 32-bit version (LA32R), a standard 32-bit version (LA32S) and a 64-bit version (LA64), and loong64 is the 64-bit version of LoongArch.

LoongArch documentation: https://github.com/loongson/LoongArch-Documentation.git

Since version 1.19, golang upstream support for the LA architecture has been maintained.

@yelvens
Copy link
Contributor Author

yelvens commented Mar 12, 2024

The arch project hasn't been integrated into the upstream yet, so I duplicated it to the vendor and submitted it.

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dumping a bunch of changes in vendor that don't exist upstream is unacceptable, the x/arch changes need to be merged there first.

pkg/gobuild/gobuild.go Outdated Show resolved Hide resolved
pkg/proc/native/support_sentinel_linux.go Outdated Show resolved Hide resolved
@aarzilli
Copy link
Member

PS. also see test failures.

@derekparker
Copy link
Member

@ihuang77 any updates on this based on @aarzilli feedback?

@yelvens
Copy link
Contributor Author

yelvens commented Apr 23, 2024

Hi @aarzilli @derekparker

The x/arch has submitted a PR to support loong64, but it has not yet been merged into the upstream repository.

I can provide machines running CI at any time.

Thanks.

@aarzilli
Copy link
Member

That's cool, we'll have to wait for that PR to be merged for this.

@derekparker
Copy link
Member

Hi @aarzilli @derekparker

The x/arch has submitted a PR to support loong64, but it has not yet been merged into the upstream repository.

I can provide machines running CI at any time.

Thanks.

Looks like this has merged. Are you available to pick this work back up?

@abner-chenc
Copy link

@ihuang77 any updates on this based on @aarzilli feedback?

Yes, the x/arch has been implemented and we are ready to update the Pr as soon as possible. We can also provide CI machines at any time.

@yelvens
Copy link
Contributor Author

yelvens commented Oct 9, 2024

Looks like this has merged. Are you available to pick this work back up?

Hi @derekparker @derekparker
The PR I submitted in x/arch has been merged. I just updated the PR I submitted here, and I would appreciate it if you could review the updated code. Thank you for your hard work.

Copy link

@abner-chenc abner-chenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go get -u golang.org/x/arch@master --> go get -u golang.org/x/[email protected]

@yelvens
Copy link
Contributor Author

yelvens commented Oct 9, 2024

go get -u golang.org/x/arch@master --> go get -u golang.org/x/[email protected]

Thank you. I have completed that task and have created a new PR specifically for submitting the arch update.

@yelvens yelvens force-pushed the master branch 2 times, most recently from 804124e to 4cb37fa Compare October 11, 2024 07:56
@yelvens
Copy link
Contributor Author

yelvens commented Oct 11, 2024

Hi @aarzilli @derekparker
As I mentioned earlier:

I can provide machines running CI at any time.

The machines are always ready. If possible, could you kindly provide the runners configured as needed? Thank you.

@aarzilli
Copy link
Member

Hi @aarzilli @derekparker As I mentioned earlier:

I can provide machines running CI at any time.

The machines are always ready. If possible, could you kindly provide the runners configured as needed? Thank you.

You can download the agent from https://delve.teamcity.com/agents/overview (also note that there currently are test failures).

@abner-chenc
Copy link

Hi @aarzilli @derekparker As I mentioned earlier:

I can provide machines running CI at any time.

The machines are always ready. If possible, could you kindly provide the runners configured as needed? Thank you.

You can download the agent from https://delve.teamcity.com/agents/overview (also note that there currently are test failures).
Hi, the loong64 machine and agent are ready, but before executing bin/agent.sh start, some configurations should be modified in conf/buildAgent.properties, such as what is the serverUrl value?

@aarzilli
Copy link
Member

Hi @aarzilli @derekparker As I mentioned earlier:

I can provide machines running CI at any time.

The machines are always ready. If possible, could you kindly provide the runners configured as needed? Thank you.

You can download the agent from https://delve.teamcity.com/agents/overview (also note that there currently are test failures).
Hi, the loong64 machine and agent are ready, but before executing bin/agent.sh start, some configurations should be modified in conf/buildAgent.properties, such as what is the serverUrl value?

Yes, see: https://www.jetbrains.com/help/teamcity/configure-agent-installation.html I have never installed an agent myself so I can not provide any specific guidance there.
Also, note the merge conflicts and test failures.

@abner-chenc
Copy link

abner-chenc commented Oct 28, 2024

Hi @aarzilli @derekparker As I mentioned earlier:

I can provide machines running CI at any time.

The machines are always ready. If possible, could you kindly provide the runners configured as needed? Thank you.

You can download the agent from https://delve.teamcity.com/agents/overview (also note that there currently are test failures).
Hi, the loong64 machine and agent are ready, but before executing bin/agent.sh start, some configurations should be modified in conf/buildAgent.properties, such as what is the serverUrl value?

Yes, see: https://www.jetbrains.com/help/teamcity/configure-agent-installation.html I have never installed an agent myself so I can not provide any specific guidance there. Also, note the merge conflicts and test failures.

I read the instructions here, but there are no detailed steps. I still don't know how to deal with it. I configured serverUrl=https://delve.teamcity.com/app/dsl-plugins-repository and started the agent, but an error occurred.

@yelvens yelvens force-pushed the master branch 3 times, most recently from 4bf8dcc to c9573df Compare November 26, 2024 06:51
@yelvens
Copy link
Contributor Author

yelvens commented Nov 26, 2024

@aarzilli @derekparker @abner-chenc

I'm sorry that I haven't worked here for a long time because I've been working on other things recently. I modified the build and test failures today and pushed the code here. Please review the code when your time permits. Thank you.

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several merge conflicts.

pkg/gobuild/gobuild.go Outdated Show resolved Hide resolved
pkg/proc/bininfo.go Outdated Show resolved Hide resolved
}
}

func loong64FixFrameUnwindContext(fctxt *frame.FrameContext, pc uint64, bi *BinaryInfo) *frame.FrameContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this function, and loong64SwitchStack I'd rather have them be empty than mindlessly copypasted from the amd64 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for patiently reviewing the code in this PR. Delve is a powerful, user-friendly, yet complex debugger. My knowledge in this area is relatively limited, and some parts of the implementation, such as loong64FixFrameUnwindContext, are not done well. However, I will continue to deepen my understanding in this area and work to improve it as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You marked this comment as resolved but did nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aarzilli

I modified the loong64SwitchStack function.
The loong64FixFrameUnwindContext function is implemented with reference to arm64 and based on the LoongArch specific DWARF definition.
As I said before, delve is a complex and powerful debugging tool. Although I am working hard to learn its related knowledge, there are still some blind spots in my knowledge. I hope you can point out my mistakes more specifically and I will correct them in time. Thank you very much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is: why is there anything at all here? This function is about stack unwinding with cgo and with exceptions, I see that many (all?) tests relating to this piece of code are disabled so why is this here at all? Why is this function not just empty? Is there any code here that actually works?

The same goes for loong64SwitchStack below. Is there anything besides the first if switch, in that function, that works? Can we delete all of it except the first if and the default branch of the first switch?

pkg/proc/loong64_arch.go Outdated Show resolved Hide resolved
pkg/proc/loong64_arch.go Outdated Show resolved Hide resolved
pkg/proc/loong64_disasm.go Outdated Show resolved Hide resolved
pkg/proc/native/ptrace_linux_64bit.go Outdated Show resolved Hide resolved
pkg/proc/native/registers_linux_loong64.go Show resolved Hide resolved
pkg/proc/pe.go Outdated Show resolved Hide resolved
@yelvens yelvens force-pushed the master branch 2 times, most recently from 29ec15f to 9a6f0f1 Compare December 3, 2024 09:19
@yelvens
Copy link
Contributor Author

yelvens commented Dec 9, 2024

@aarzilli Can you please take another look at this patch? Thanks a lot.

@yelvens yelvens force-pushed the master branch 3 times, most recently from 14e9755 to 1a72c85 Compare December 11, 2024 01:25
@abner-chenc
Copy link

Hi, @aarzilli Can this Pr be merged? When I use delve on different machines to debug Go programs on loong64 machines, I need to download this Pr to the local machine and compile it separately each time. I think if this Pr can be merged, I will have a better experience when using delve.

Thanks

}
}

func loong64FixFrameUnwindContext(fctxt *frame.FrameContext, pc uint64, bi *BinaryInfo) *frame.FrameContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is: why is there anything at all here? This function is about stack unwinding with cgo and with exceptions, I see that many (all?) tests relating to this piece of code are disabled so why is this here at all? Why is this function not just empty? Is there any code here that actually works?

The same goes for loong64SwitchStack below. Is there anything besides the first if switch, in that function, that works? Can we delete all of it except the first if and the default branch of the first switch?

@@ -0,0 +1,180 @@
// TODO: disassembler support should be compiled in unconditionally,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment.

@@ -372,7 +372,7 @@ func (dbp *nativeProcess) initialize(path string, debugInfoDirs []string) (*proc
// with gdb once AsyncPreempt was enabled. While implementing the port,
// few tests failed while it was enabled, but cannot be warrantied that
// disabling it fixed the issues.
DisableAsyncPreempt: runtime.GOOS == "windows" || (runtime.GOOS == "linux" && runtime.GOARCH == "arm64") || (runtime.GOOS == "linux" && runtime.GOARCH == "ppc64le"),
DisableAsyncPreempt: runtime.GOOS == "windows" || (runtime.GOOS == "linux" && runtime.GOARCH == "arm64") || (runtime.GOOS == "linux" && runtime.GOARCH == "ppc64le") || (runtime.GOOS == "linux" && runtime.GOARCH == "loong64"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other architecture has an explanation for why asyncpreempt is disabled in the preceding comment, loong64 should also do this. I imagine the reason is the same as linux/arm64.

@@ -382,7 +382,7 @@ func (dbp *nativeProcess) initialize(path string, debugInfoDirs []string) (*proc
if err != nil {
return nil, err
}
if dbp.bi.Arch.Name == "arm64" || dbp.bi.Arch.Name == "ppc64le" || dbp.bi.Arch.Name == "riscv64" {
if dbp.bi.Arch.Name == "arm64" || dbp.bi.Arch.Name == "ppc64le" || dbp.bi.Arch.Name == "riscv64" || dbp.bi.Arch.Name == "loong64"{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run gofmt on the files you have changed.

Huang Qiqi added 2 commits December 31, 2024 11:19
LoongArch is a new RISC ISA, which is independently designed by Loongson Technology.

LoongArch includes a reduced 32-bit version (LA32R), a standard 32-bit version (LA32S)
and a 64-bit version (LA64), and loong64 is the 64-bit version of LoongArch.

LoongArch documentation: https://github.com/loongson/LoongArch-Documentation.git
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants