delve: add ppc64le support #2963

alexsaezm · 2022-04-06T09:32:09Z

This adds support to PPC64LE architecture. This is still in an early stage of development but I'm opening this draft pull request to keep track of suggestions about the port and other things that might happen.

Currently runtime.Breakpoint is broken for ppc64le so I'm using a patched version of Go.

derekparker · 2022-07-05T17:15:33Z

Adding the v1.9.1 milestone as I'd like to get this in for the next release. Of course this also depends on CI availability so we might slip a bit, but I wanted to at least aim for it to be included.

aarzilli · 2022-07-14T09:01:08Z

Is this marked as Draft by accident or is it still a draft?

alexsaezm · 2022-07-14T11:10:31Z

Is this marked as Draft by accident or is it still a draft?

It's still not 100% functional so I guess it's still a draft...?

aarzilli · 2022-07-14T11:35:35Z

No problem, I just thought I'd remind you.

alexsaezm · 2022-11-23T17:28:21Z

Removed the draft status. Some tests are still failing but it is functional.

Related issue #1564

derekparker · 2022-11-23T18:14:18Z

@alexsaezm thanks for this! Any test that is still failing please just skip explicitly and provide some explanation.

We can work on adding CI for this as well.

aarzilli

If there's going to be a builder for teamcity there should be a builder for teamcity before this is merged. If there isn't going to be a builder for teamcity this should be gated behind a build flag in support_sentinel_linux.go much like the windows/arm64 backend is gated behind a build flag in support_sentinel_window.go.

pkg/proc/ppc64le_arch.go

pkg/dwarf/regnum/ppc64le.go

aarzilli · 2022-11-24T09:04:21Z

pkg/proc/bininfo.go

@@ -365,8 +366,19 @@ func FindFunctionLocation(p Process, funcName string, lineOffset int) ([]uint64,
 // If sameline is set FirstPCAfterPrologue will always return an
 // address associated with the same line as fn.Entry.
 func FirstPCAfterPrologue(p Process, fn *Function, sameline bool) (uint64, error) {
+ // The DWARF prologue_end statement is incorrect in ppc64le and


I'm very skeptical that this code can be correct. How does this skip the prologue? Not skipping the prologue will cause a number of problems. If the prologue_end position is wrong then:

there should be a PR to fix it against cmd/link

the disassembly method should be used

I agree, we should be using the disassembly method as with other arches. Looks like we just need to update the disassemble patterns for matching against.

Also, I'll look into the upstream patch to fix the prologue_end position.

I've taken a second look at this, the patch to add prologue_end markers (63fd764502e08d067293a93d6d1a566951255ce5) still looks correct to me, even when observing where the prologue_end marker is placed in the executables.

pkg/proc/bininfo.go

pkg/proc/target_exec.go

pkg/dwarf/regnum/ppc64le.go

derekparker · 2022-11-29T16:58:15Z

pkg/proc/bininfo.go

@@ -365,8 +366,19 @@ func FindFunctionLocation(p Process, funcName string, lineOffset int) ([]uint64,
 // If sameline is set FirstPCAfterPrologue will always return an
 // address associated with the same line as fn.Entry.
 func FirstPCAfterPrologue(p Process, fn *Function, sameline bool) (uint64, error) {
+ // The DWARF prologue_end statement is incorrect in ppc64le and


I agree, we should be using the disassembly method as with other arches. Looks like we just need to update the disassemble patterns for matching against.

Also, I'll look into the upstream patch to fix the prologue_end position.

pkg/proc/bininfo.go

pkg/proc/linutil/regs_ppc64le_arch.go

pkg/proc/native/threads_linux_ppc64le.go

derekparker · 2022-11-29T17:05:28Z

pkg/proc/proc_test.go

@@ -3324,6 +3324,11 @@ func logStacktrace(t *testing.T, p *proc.Target, frames []proc.Stackframe) {
 name = fmt.Sprintf("%s inlined in %s", frames[j].Call.Fn.Name, frames[j].Current.Fn.Name)
 }

+ t.Logf("\t%#x %#x %#x %s at %s:%d\n",


Probably a rebase error but I agree this should be removed.

derekparker · 2022-11-29T17:07:04Z

pkg/proc/stack.go

@@ -158,6 +158,9 @@ func (g *G) Stacktrace(depth int, opts StacktraceOptions) ([]Stackframe, error)
 if err != nil {
 return nil, err
 }
+ if it.err != nil {


This was done to not hide there error when the full stack could not be unwound, because otherwise it gets hidden. Can likely be removed now.

derekparker · 2022-11-29T17:07:35Z

pkg/proc/stack.go

 switch rule.Rule {
 default:
 fallthrough
 case frame.RuleUndefined:
 return nil, nil
 case frame.RuleSameVal:
 if it.regs.Reg(regnum) == nil {
- return nil, nil
+ return nil, fmt.Errorf("register %d undefined", regnum)


Yeah, we can just return nil here.

alexsaezm · 2023-03-08T18:29:18Z

I updated the PR with several modifications according to your comments @aarzilli & @derekparker

aarzilli · 2023-03-08T18:50:27Z

I'll take a better look tomorrow but also there are several failing tests on existing platforms.

aarzilli

I marked a bunch of things as merge errors but it could also be that github is being weird about the diff because of the merge conflict in proc_test.go.

aarzilli · 2023-03-09T14:13:39Z

pkg/dwarf/regnum/ppc64le.go

+ case num <= 31:
+ return fmt.Sprintf("R%d", num)
+ case num <= 62:
+ return fmt.Sprintf("F%d", num)


Judging from the reference document this should be num-32

aarzilli · 2023-03-09T14:13:47Z

pkg/dwarf/regnum/ppc64le.go

+ case num == PPC64LE_PC:
+ return "PC"
+ case num == PPC64LE_LR:
+ return "Link"


aarzilli · 2023-03-09T14:15:58Z

pkg/dwarf/regnum/ppc64le.go

+const (
+ PPC64LE_R0 = 0 // General Purpose Registers: from R0 to R31
+ PPC64LE_F0 = 0 // Floating point registers: from F0 to F31
+ PPC64LE_V0 = 0 // Vector (Altivec/VMX) registers: from V0 to V31


Is this the same as VR0? Shouldn't it be 77?

aarzilli · 2023-03-09T14:16:26Z

pkg/dwarf/regnum/ppc64le.go

+ case num == PPC64LE_LR:
+ return "Link"
+ case num >= PPC64LE_V0 && num <= 108:
+ return fmt.Sprintf("V%d", num-64)


Should 64 be PPC64LE_V0?

aarzilli · 2023-03-09T14:16:42Z

pkg/dwarf/regnum/ppc64le.go

+
+const (
+ PPC64LE_R0 = 0 // General Purpose Registers: from R0 to R31
+ PPC64LE_F0 = 0 // Floating point registers: from F0 to F31


Shouldn't this be 32?

aarzilli · 2023-03-09T14:22:54Z

pkg/proc/native/proc.go

+ // 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 == "freebsd" || (runtime.GOOS == "linux" && runtime.GOARCH == "arm64") || (runtime.GOOS == "linux" && runtime.GOARCH == "ppc64le"),


Readding freebsd here is also a merge error.

aarzilli · 2023-03-09T14:26:03Z

pkg/proc/native/support_sentinel_linux.go

@@ -1,6 +1,6 @@
 // This file is used to detect build on unsupported GOOS/GOARCH combinations.

-//go:build linux && !amd64 && !arm64 && !386
-// +build linux,!amd64,!arm64,!386
+//go:build linux && !amd64 && !arm64 && !386 && !ppc64le


What's the story going to be with CI? Are we going to have a ppc64 builder? Otherwise we should do the same thing we did with windows/arm64 and put it behind a build tag I think.

I requested a PPC64LE machine to put it into the CI system. I already have it.

What's the status on getting TeamCity agent installed there?

aarzilli · 2023-03-09T14:28:00Z

pkg/proc/ppc64le_disasm.go

+var prologuesPPC64LE []opcodeSeq
+
+func init() {
+ // Note: these will be the gnu opcodes and not the Go opcodes. Verify the sequences are as expected.


I still don't think these should be here unless the comment in FirstPCAfterPrologue is wrong.

aarzilli · 2023-03-09T14:28:43Z

pkg/proc/proc_test.go

@@ -586,6 +586,7 @@ func TestNextGeneral(t *testing.T) {
 }

 func TestNextConcurrent(t *testing.T) {
+ skipOn(t, "broken", "freebsd")


A bunch of these have been reintroduced due to merge errors. In fact a lot of changes in this file seem to be merge errors and should be rolled back.

Unless maybe there are no merge errors and github UI is just lying to me because of the merge conflict.

There was a merge error as you said. I think is now fixed.

aarzilli · 2023-03-09T14:32:33Z

pkg/proc/stack.go

@@ -211,6 +226,9 @@ func (it *stackIterator) Next() bool {
 }

 callFrameRegs, ret, retaddr := it.advanceRegs()
+ if it.err != nil {


Still wondering about this.

aarzilli · 2023-04-27T12:43:27Z

pkg/proc/bininfo.go

@@ -365,8 +366,19 @@ func FindFunctionLocation(p Process, funcName string, lineOffset int) ([]uint64,
 // If sameline is set FirstPCAfterPrologue will always return an
 // address associated with the same line as fn.Entry.
 func FirstPCAfterPrologue(p Process, fn *Function, sameline bool) (uint64, error) {
+ // The DWARF prologue_end statement is incorrect in ppc64le and


I've taken a second look at this, the patch to add prologue_end markers (63fd764502e08d067293a93d6d1a566951255ce5) still looks correct to me, even when observing where the prologue_end marker is placed in the executables.

aarzilli · 2023-04-27T12:46:11Z

pkg/proc/proc_test.go

@@ -3337,7 +3337,7 @@ func TestCgoStacktrace(t *testing.T) {
 }

 skipOn(t, "broken - cgo stacktraces", "386")
- skipOn(t, "broken - cgo stacktraces", "windows", "arm64")


merge problem

I think it's now fixed

aarzilli · 2023-04-27T12:46:59Z

pkg/proc/proc_test.go

@@ -3900,6 +3903,9 @@ func TestInlinedStacktraceAndVariables(t *testing.T) {
 }

 func TestInlineStep(t *testing.T) {
+ if runtime.GOARCH == "ppc64le" {


skipOn(t, "broken", "ppc64le")

aarzilli · 2023-04-27T12:49:57Z

service/dap/server_test.go

@@ -2465,6 +2465,7 @@ func TestGlobalScopeAndVariables(t *testing.T) {
 // got loaded. It then steps into a function in another package and tests that
 // the registers were updated by checking PC.
 func TestRegistersScopeAndVariables(t *testing.T) {
+ t.Skip("skipped on ppc64le: broken")


Should be guarded by an if

aarzilli · 2023-04-27T12:50:28Z

service/test/integration1_test.go

@@ -858,6 +858,7 @@ func Test1Issue355(t *testing.T) {
 }

 func Test1Disasm(t *testing.T) {
+ t.Skip("skipped on ppc64le: broken")


should be guarded by an if

aarzilli · 2023-04-27T12:50:41Z

service/test/integration2_test.go

@@ -1359,6 +1359,7 @@ func TestIssue355(t *testing.T) {
 }

 func TestDisasm(t *testing.T) {
+ t.Skip("skipped on ppc64le: broken")


should be guarded by an if

aarzilli · 2023-04-27T12:50:46Z

service/test/integration2_test.go

@@ -2855,6 +2856,7 @@ func assertLine(t *testing.T, state *api.DebuggerState, file string, lineno int)
 }

 func TestPluginSuspendedBreakpoint(t *testing.T) {
+ t.Skip("skipped on ppc64le: broken")


should be guarded by an if

pkg/proc/ppc64le_arch.go

derekparker · 2023-06-12T21:43:31Z

cmd/dlv/cmds/commands.go

@@ -690,6 +690,7 @@ func traceCmd(cmd *cobra.Command, args []string) {
 fmt.Fprintf(os.Stderr, "unable to set tracepoint on function %s: %#v\n", funcs[i], err)
 continue
 }
+ fmt.Fprintf(os.Stderr, "Addrs %#v\n", addrs)


Leftover print.

pkg/dwarf/regnum/ppc64le.go

derekparker · 2023-06-12T21:49:03Z

pkg/proc/native/support_sentinel_linux.go

@@ -1,6 +1,6 @@
 // This file is used to detect build on unsupported GOOS/GOARCH combinations.

-//go:build linux && !amd64 && !arm64 && !386
-// +build linux,!amd64,!arm64,!386
+//go:build linux && !amd64 && !arm64 && !386 && !ppc64le


What's the status on getting TeamCity agent installed there?

pkg/proc/bininfo.go

pkg/proc/ppc64le_arch.go

derekparker · 2023-06-12T21:53:37Z

pkg/proc/proc_test.go

@@ -2863,6 +2873,7 @@ func TestNextInDeferReturn(t *testing.T) {
 }

 func TestAttachDetach(t *testing.T) {
+ skipOn(t, "broken", "ppc64le")


This test shouldn't be arch specfic, unless it's failing on PIE on ppc.

derekparker · 2023-06-12T21:54:11Z

pkg/proc/proc_test.go

@@ -3920,6 +3935,7 @@ func TestInlinedStacktraceAndVariables(t *testing.T) {
 }

 func TestInlineStep(t *testing.T) {
+ skipOn(t, "broken", "ppc64le")


This shouldn't be arch specific, what's the failure in ppc?

derekparker · 2023-06-12T21:54:38Z

pkg/proc/proc_test.go

@@ -4080,6 +4096,7 @@ func TestIssue951(t *testing.T) {
 }

 func TestDWZCompression(t *testing.T) {
+ skipOn(t, "broken", "ppc64le")


This shouldn't be broken based on arch, what's the failure?

derekparker · 2023-06-12T21:55:29Z

pkg/proc/proc_test.go

@@ -5838,6 +5864,9 @@ func TestNilPtrDerefInBreakInstr(t *testing.T) {
 func TestStepIntoAutogeneratedSkip(t *testing.T) {
 // Tests that autogenerated functions are skipped with the new naming
 // scheme for autogenerated functions (issue #2948).
+ if runtime.GOARCH == "ppc64le" {


nit: use skipOn.

derekparker · 2023-06-12T21:57:31Z

service/dap/server_test.go

@@ -2457,6 +2457,9 @@ func TestGlobalScopeAndVariables(t *testing.T) {
 // got loaded. It then steps into a function in another package and tests that
 // the registers were updated by checking PC.
 func TestRegistersScopeAndVariables(t *testing.T) {
+ if runtime.GOARCH == "ppc64le" {


What's the failure on ppc?

aarzilli · 2023-07-06T12:12:00Z

pkg/proc/bininfo.go

- if entryLine == line {
- return pc, nil
- }
+ pc, _, line, ok := fn.cu.lineInfo.PrologueEndPC(fn.Entry, fn.End)


Is this undoing the changes for the no-dwarf stuff?

aarzilli · 2023-07-06T12:13:48Z

_scripts/test_linux.sh

@@ -76,3 +76,11 @@ else
 exit $x
 fi

+export GOARCH=ppc64le
+go run _scripts/make.go --tags exp.linuxppc64le


There's a bunch of test failures in CI, I think this might be what's causing them.

I think it was the TestGeneratedDoc. It should be fix now

You are right: this isn't even running currently in CI because other failures (TestStaticcheck) are causing the script to exit before this is even executed. However I don't think it's going to work. The export GOARCH=ppc64le makes go run produce a ppc64le binary for make.go, which won't run.

I think the way to do this is to do GOARCH=ppc64le go build -tags exp.linuxppc64le github.com/go-delve/delve/cmd/dlv directly.

The tests now pass, this line however is never executed because of the exit above.

aarzilli · 2023-07-07T08:55:56Z

_scripts/test_linux.sh

@@ -76,3 +76,11 @@ else
 exit $x
 fi

+export GOARCH=ppc64le
+go run _scripts/make.go --tags exp.linuxppc64le


You are right: this isn't even running currently in CI because other failures (TestStaticcheck) are causing the script to exit before this is even executed. However I don't think it's going to work. The export GOARCH=ppc64le makes go run produce a ppc64le binary for make.go, which won't run.

I think the way to do this is to do GOARCH=ppc64le go build -tags exp.linuxppc64le github.com/go-delve/delve/cmd/dlv directly.

aarzilli · 2023-07-07T08:57:07Z

pkg/dwarf/regnum/ppc64le.go

+ }
+}
+
+// According to LLDB's documentation, there is 172 registers in total, across 4 categories:


This comment needs to start with PPC64LEMaxRegNum or there needs to be an exclusion directive for staticcheck.

aarzilli · 2023-07-07T09:04:01Z

I'm not sure what's causing the new test failures on macOS, the only thing that sticks out is the if err return in the stacktracing code.

aarzilli · 2023-07-07T09:06:25Z

PS. builds of master pass the test normally, so it doesn't look like it's something broken in CI but something in this PR. Other possibility is that it's screwing up something with the automatic merge (it's 60 commits), in which case squashing should fix it.

alexsaezm · 2023-07-07T09:07:28Z

For some reason ( I think it might be the latest rebase) in linux amd64 several tests fail but they pass on ppc64le. @derekparker suggested me yesterday a change that make it pass again locally but new tests fails on ppc64le.

PS. builds of master pass the test normally, so it doesn't look like it's something broken in CI but something in this PR. Other possibility is that it's screwing up something with the automatic merge (it's 60 commits), in which case squashing should fix it.

I'll squash the whole thing because there's a lot of commits that means nothing.

alexsaezm · 2023-07-07T09:34:28Z

Squashed and rebased, hopefully now we can find out what is going on.

aarzilli

LGTM

alexsaezm · 2023-07-07T12:07:30Z

Looks like TestFindReturnAddressTopOfStackFn now fails (on ppc64le)... working on it 😞

aarzilli · 2023-07-07T12:29:08Z

As far as I am concerned it's fine to merge this with TestFindReturnAddressTopOfStackFn failing on ppc64le, especially since it's behind a build tag. That test isn't that important either.

derekparker · 2023-07-07T14:38:13Z

Looks like all that's needed now is to fix TestGeneratedDoc and then this is good to merge.

alexsaezm · 2023-07-07T14:56:19Z

Looks like all that's needed now is to fix TestGeneratedDoc and then this is good to merge.

Done

pkg/proc/ppc64le_arch.go

pkg/proc/proc_test.go

derekparker

LGTM

alexsaezm force-pushed the ppc64le branch from ea56638 to 0a79c04 Compare June 3, 2022 11:55

derekparker added this to the v1.9.1 milestone Jul 5, 2022

alexsaezm force-pushed the ppc64le branch from ce96b10 to ec2763c Compare September 7, 2022 10:35

alexsaezm force-pushed the ppc64le branch from dbaec74 to a292468 Compare October 13, 2022 16:30

alexsaezm force-pushed the ppc64le branch from a292468 to 9d1ed2e Compare October 24, 2022 18:05

alexsaezm force-pushed the ppc64le branch from 8ad6df1 to 9d08fab Compare November 8, 2022 17:49

alexsaezm force-pushed the ppc64le branch 3 times, most recently from cdb245e to 3e42216 Compare November 23, 2022 17:26

alexsaezm marked this pull request as ready for review November 23, 2022 17:27

derekparker requested review from aarzilli and derekparker November 23, 2022 18:12

aarzilli reviewed Nov 24, 2022

View reviewed changes

derekparker requested changes Nov 29, 2022

View reviewed changes

alexsaezm force-pushed the ppc64le branch from e6a0f78 to 0a83877 Compare February 16, 2023 17:15

alexsaezm requested review from aarzilli and derekparker and removed request for aarzilli March 8, 2023 18:27

aarzilli reviewed Mar 9, 2023

View reviewed changes

alexsaezm force-pushed the ppc64le branch from f93efa0 to 7d783c3 Compare April 20, 2023 12:55

aarzilli reviewed Apr 27, 2023

View reviewed changes

alexsaezm force-pushed the ppc64le branch from 65147af to 6f8b24e Compare April 28, 2023 12:07

alexsaezm force-pushed the ppc64le branch from 8eddaf1 to c3d4ad5 Compare May 4, 2023 18:50

alexsaezm commented May 4, 2023

View reviewed changes

pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved

derekparker requested changes Jun 12, 2023

View reviewed changes

alexsaezm force-pushed the ppc64le branch from 8adcb6a to 66718c4 Compare July 5, 2023 18:37

aarzilli reviewed Jul 6, 2023

View reviewed changes

aarzilli reviewed Jul 7, 2023

View reviewed changes

alexsaezm force-pushed the ppc64le branch from ef3f539 to d09ce5f Compare July 7, 2023 09:32

Add vendor/golang.org/x/arch/ppc64

1a20020

alexsaezm force-pushed the ppc64le branch from d09ce5f to 6c0d083 Compare July 7, 2023 09:33

aarzilli approved these changes Jul 7, 2023

View reviewed changes

alexsaezm force-pushed the ppc64le branch from 6c0d083 to f40e6a1 Compare July 7, 2023 13:01

alexsaezm force-pushed the ppc64le branch from f40e6a1 to 277425f Compare July 7, 2023 14:55

derekparker requested changes Jul 7, 2023

View reviewed changes

pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved

pkg/proc/ppc64le_arch.go Outdated Show resolved Hide resolved

pkg/proc/proc_test.go Outdated Show resolved Hide resolved

pkg/proc/proc_test.go Outdated Show resolved Hide resolved

Add ppc64le support

2b06d3f

alexsaezm force-pushed the ppc64le branch from 277425f to 2b06d3f Compare July 7, 2023 15:58

derekparker approved these changes Jul 7, 2023

View reviewed changes

derekparker merged commit 71f1220 into go-delve:master Jul 7, 2023
1 check passed

delve: add ppc64le support #2963

delve: add ppc64le support #2963

Conversation

alexsaezm commented Apr 6, 2022

derekparker commented Jul 5, 2022 • edited Loading

aarzilli commented Jul 14, 2022

alexsaezm commented Jul 14, 2022

aarzilli commented Jul 14, 2022

alexsaezm commented Nov 23, 2022

derekparker commented Nov 23, 2022

aarzilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsaezm commented Mar 8, 2023

aarzilli commented Mar 8, 2023

aarzilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli commented Jul 7, 2023

aarzilli commented Jul 7, 2023

alexsaezm commented Jul 7, 2023

alexsaezm commented Jul 7, 2023 • edited Loading

aarzilli left a comment

Choose a reason for hiding this comment

alexsaezm commented Jul 7, 2023 • edited Loading

aarzilli commented Jul 7, 2023

derekparker commented Jul 7, 2023

alexsaezm commented Jul 7, 2023

derekparker left a comment

Choose a reason for hiding this comment

derekparker commented Jul 5, 2022 •

edited

Loading

alexsaezm commented Jul 7, 2023 •

edited

Loading

alexsaezm commented Jul 7, 2023 •

edited

Loading