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

Add arm-linux support to delve #2122

Closed
wants to merge 30 commits into from
Closed

Conversation

puppywang
Copy link

This is the intial support of arm arch for linux os, to address the problem in this issue #20, The arm-linux's ptrace is lacking support of PTRACE_SINGLESTEP, so we need to emulate it by setup breakpoint at any possible next instructions, and then continue execution. I try to fix all the bug that I found, but there may still have plenty of them. Any code review comments is welcome, thanks.

@chainhelen
Copy link
Contributor

So great.

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.

How do we test this?

pkg/proc/native/threads_linux_arm.go Outdated Show resolved Hide resolved
pkg/proc/native/threads_linux_arm.go Outdated Show resolved Hide resolved
pkg/proc/native/threads_single_step_linux.go Outdated Show resolved Hide resolved
pkg/proc/target_exec.go Outdated Show resolved Hide resolved
pkg/proc/native/threads_linux_arm.go Outdated Show resolved Hide resolved
pkg/proc/target_exec.go Outdated Show resolved Hide resolved
WangYi added 2 commits September 8, 2020 20:56
…p break instruction on arm.

2. Move breakpointInstr calc outside of for loop.
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.

In a previous comment I was asking "How do we test this?". This is a question we need to find an answer to. We like to have continuous integration for the architectures we support.

nextInstrLen := t.BinInfo().Arch.MaxInstructionLength()
nextInstrBytes := make([]byte, nextInstrLen)
var err error
t.dbp.execPtraceFunc(func() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use t.ReadMemory (here and in the other places in this function where PtracePeekData is called)?

Copy link
Author

Choose a reason for hiding this comment

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

As this is inside low level emulation of ptrace singlestep, I want to make sure it first enter the context of ptrace channel, then do the peek and poke work, so I skip the upper Read/WriteMemory function, and use ptrace function directly.

@puppywang
Copy link
Author

In a previous comment I was asking "How do we test this?". This is a question we need to find an answer to. We like to have continuous integration for the architectures we support.

Yes, currently I do only a little test on raspberry pi4, and I am looking for help to make a more coverage test, do we have any guideline to make such test on different platform? Thanks.

@aarzilli
Copy link
Member

Yes, currently I do only a little test on raspberry pi4, and I am looking for help to make a more coverage test, do we have any guideline to make such test on different platform? Thanks.

So far we've gotten along using free services like travis, see .travis.yml. It doesn't have 32bit arm but I don't know if it's possible to do the same thing we do with linux/386.

@aarzilli
Copy link
Member

aarzilli commented Nov 3, 2020

Regarding testing this, I tried just adding an export GOARCH=arm to the build script. This works on my raspberry pi but it doesn't on Travis-CI, presumably because they didn't enable the appropriate kernel option.
Result of the experiment here: #2218

Additionally the tests hang randomly on my raspberrypi inside native.(*nativeThread).singleStep.

@puppywang
Copy link
Author

puppywang commented Nov 5, 2020

Additionally the tests hang randomly on my raspberrypi inside native.(*nativeThread).singleStep.

Got, let me check with my arm device.

@ghost
Copy link

ghost commented Nov 28, 2020

Can confirm d75633f works on my Raspberry Pi 3 with an armhf type processor.

@daresan
Copy link

daresan commented Dec 4, 2020

So no success, to make delve work on a 32bit raspberry Pi OS yet? Just asking, because I just spent more than 2 hours trying to find a 32bit version for raspbian. It should be mentioned in the documentation.

@xcaptain
Copy link

xcaptain commented Jan 3, 2021

Also got a raspberry pi and can't run dlv
image
would take some time to try this branch and see if it works on raspberry pi 4b+

@puppywang
Copy link
Author

puppywang commented Jan 6, 2021

Also got a raspberry pi and can't run dlv
image
would take some time to try this branch and see if it works on raspberry pi 4b+

Before this merge is accept, you could try my working branch, try following cmd.

git clone https://github.com/puppywang/delve
cd delve
go install
``
And try debug with delve.

@krasi-georgiev
Copy link

magic I just tested it and it works!

@krasi-georgiev
Copy link

krasi-georgiev commented Jan 20, 2021

spoke too soon, stepping in almost never works.

@derekparker
Copy link
Member

I've been holding off on a deeper review of this since we've never been able to answer the question of how do we have CI for this? I agree 32-bit ARM support would be great, but we need CI to verify this (I don't even have a Pi or other 32-bit ARM system to test this on, unfortunately).

@aarzilli
Copy link
Member

@derekparker we could probably do the same thing we do on 386 and run it inside docker. If the kernel is compiled with compatibility with arm32 binaries the tests can be run directly on arm64. See my comment #2122 (comment). The problem however is that it randomly hangs inside singleStep.

@derekparker
Copy link
Member

@derekparker we could probably do the same thing we do on 386 and run it inside docker. If the kernel is compiled with compatibility with arm32 binaries the tests can be run directly on arm64. See my comment #2122 (comment). The problem however is that it randomly hangs inside singleStep.

Ack, alright that makes sense.

@puppywang will you be able to investigate this outstanding issue?

@chrisdutz
Copy link

Another option I discovered recently would be to use a GitHub Action and to define a "local runner" ... there a RPi could possibly be setup as local runner.

@chrisdutz
Copy link

And to the cross compiling on a non-arm machine. Was having similar issues with cross compiling some Go application using CGO to 32bit mips and solved that by using a toolchain from the OpenWRT project and setting CC to a gcc version built for building mips. Currently I'm doing the same for an OpenWRT system intended for arm 32bit systems, so this could be used ... however building the OpenWRT toolchain does consume quite a bit of CI-time and might drain the resources an open-source project has available.

@chrisdutz
Copy link

So yesterday I successfully used what we use for building our application for arm32 devices and was able to compile the arm version on my normal ubuntu laptop (So it should be buildable on a normal CI agent or in a Docker container). I was able to run the Delve binary on my edge device, but yes ... I also did encounter these lockups.

Is there anything I can help with?

@jochen42
Copy link
Contributor

i am using a patched dlv-binary (the code of the mr) and go 1.17 on linux-arm7 and remote-debugging. work's like charm until now.
do you plan to integrate this into the stable binanry? Can i support?

@aarzilli
Copy link
Member

i am using a patched dlv-binary (the code of the mr) and go 1.17 on linux-arm7 and remote-debugging. work's like charm until now.
do you plan to integrate this into the stable binanry? Can i support?

Last time I looked at this it didn't pass most of the tests reliably, so there are no plans at the moment to merge this. If you want to rebase it and fix the problems yourself see: https://github.com/go-delve/delve/blob/master/Documentation/internal/portnotes.md

@adelgado0723 adelgado0723 mentioned this pull request Nov 22, 2021
@derekparker
Copy link
Member

Closing stale PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants