Add support for windows/arm64 #3063

qmuntal · 2022-07-13T16:01:25Z

This PR adds support for debugging on windows/arm64. There are some tests failing, but with these changes I can use dlv to debug big codebases without problems.

I could reuse all work done to support arm64 on linux. The only difference is that windows use BRK 0xF000 as breakpoint instruction instead of just BRK.

On the windows syscall side, I've redefined CONTEXT so it matches Windows ARM64 expectations.

There are a couple of things that are still not supported:

Core dumps
Hardware breakpoints
Some tests using runtime.Breakpoint() fail due to runtime: runtime.Breakpoint() generates illegal instruction on windows/arm64 golang/go#53837

Fixes #2129

aarzilli

Aside from the minidump problem and the comment on the CONTEXT interface I think there are three questions two answer:

I think we should wait to merge this until we have a version of Go that we can test it on. It doesn't have to be a released version, it could be go built at tip.
You say that some tests don't pass but made no changes to _test.go files, except core_test.go. Tests that don't pass should be skipped.
We would need to have some way to run CI on windows/arm64 otherwise it is going to break very soon

Regarding function call injection, I would expect it to work effortlessly, I suspect the problem you are encountering is that the runtime uses BRK extensively to communicate with delve for this, all of those BRK instruction will have the same problem runtime.Breakpoint() has.

pkg/proc/native/syscall_windows_arm64.go

pkg/proc/native/threads_windows.go

qmuntal · 2022-07-20T10:56:13Z

I think we should wait to merge this until we have a version of Go that we can test it on. It doesn't have to be a released version, it could be go built at tip.

Go provides official windows/arm64 since go1.17, so this should be a blocker.

You say that some tests don't pass but made no changes to _test.go files, except core_test.go. Tests that don't pass should be skipped.

Will do that soon.

We would need to have some way to run CI on windows/arm64 otherwise it is going to break very soon

Agree. Could we at least merge the part of this PR that refactors windows to be more friendly to non-amd64 architectures? This will pave the path for when we have windows/arm64 CI and it will allow me, and other people wanting to debug arm64 using this branch, to patch arm64 support with minimal effort.

pkg/proc/native/proc_windows.go

aarzilli · 2022-07-21T08:24:25Z

I think we should wait to merge this until we have a version of Go that we can test it on. It doesn't have to be a released version, it could be go built at tip.

Go provides official windows/arm64 since go1.17, so this should be a blocker.

I don't understand what you mean by this.

We would need to have some way to run CI on windows/arm64 otherwise it is going to break very soon

Agree. Could we at least merge the part of this PR that refactors windows to be more friendly to non-amd64 architectures? This will pave the path for when we have windows/arm64 CI and it will allow me, and other people wanting to debug arm64 using this branch, to patch arm64 support with minimal effort.

I think that's fine.

qmuntal · 2022-07-21T09:33:24Z

I think we should wait to merge this until we have a version of Go that we can test it on. It doesn't have to be a released version, it could be go built at tip.

Go provides official windows/arm64 since go1.17, so this should be a blocker.

I don't understand what you mean by this.

Then I don't understand your initial concern. Go already supports windows/arm64, so there is a version of Go that we can test it on. What's missing other than the CI?

aarzilli · 2022-07-21T09:52:08Z

Then I don't understand your initial concern. Go already supports windows/arm64, so there is a version of Go that we can test it on. What's missing other than the CI?

I was thinking about the problem with runtime.Breakpoint, I think we should wait until there's a version of go with a fixed runtime.Breakpoint, a large number of tests depends on runtime.Breakpoint being handled correctly.

qmuntal · 2022-07-28T14:03:31Z

@aarzilli I've rebased this PR and gated windows/arm64 behind the exp.winarm64 tag. What do you think?

I'm also struggling to run the test suite locally either on amd64 and arm64 because proc.FindFileLocation always returns ErrCouldNotFindLine. There seems to be a mishandling of the file name casing on windows.

derekparker · 2022-07-28T21:38:34Z

@aarzilli I've rebased this PR and gated windows/arm64 behind the exp.winarm64 tag. What do you think?

I'm also struggling to run the test suite locally either on amd64 and arm64 because proc.FindFileLocation always returns ErrCouldNotFindLine. There seems to be a mishandling of the file name casing on windows.

That's an odd one, our Windows builders seem to be running fine. Is there anything different about your test / development systems that could cause such an error? I don't have a Windows machine handy to test on right now.

qmuntal · 2022-07-29T09:15:51Z

That's an odd one, our Windows builders seem to be running fine. Is there anything different about your test / development systems that could cause such an error? I don't have a Windows machine handy to test on right now.

Found the culprit. I'm running the tests inside vscode, and for historical reasons they normalize the drive letter to lower-case. This shouldn't be a problem, because Windows file paths are case-insensitive, but proc.FindFileLocation is case sensitive, so it can't find the file even if it is there.

aarzilli · 2022-07-29T10:06:09Z

.teamcity/settings.kts

@@ -201,7 +204,11 @@ class TestBuild(val os: String, val arch: String, version: String, buildId: Abso
                    scriptMode = file {
                        path = "_scripts/test_windows.ps1"
                    }
-                    param("jetbrains_powershell_scriptArguments", "-version ${"go$version"} -arch $arch")
+                    if (arch == "arm64") {
+                        param("jetbrains_powershell_scriptArguments", "-version ${"go$version"} -arch amd64 -winarm64")


I don't understand how this change works. You are making test_windows.ps1 download the amd64 version of the toolchain and then run the tests by setting GOARCH manually to arm64? Why?

We still don't have a windows arm64 builder to run tests, but at least we can cross-compile from the windows amd64 builder to check for compilation errors.

But is this going to work given that there are no arm64 agents? And when there will be (if there will be any) it definitely won't work because they will try to execute the amd64 version of the go toolchain. I'd just add a build command at the end of test_windows.ps1.

I'll add the build command at the end of the ps1 script.

.teamcity/settings.kts

aarzilli · 2022-07-29T10:08:08Z

pkg/proc/bininfo.go

-	if entryPoint != 0 {
-		image.StaticBase = entryPoint - opth.ImageBase
-	} else {
-		if opth.DllCharacteristics&_IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE != 0 {


Why did you remove this check?

windows/arm64 binaries are always PIE, even if built with -buildmode=exe, so we need to support entryPoint==0. I don't know why this check was added, but as per the TODO comment I supposed it was just that it still wasn't tested with a real program, which I'm doing now.

The TODO command was obsolete, we did end up checking that this worked on windows. AFAIK PIE binaries always have non-zero entryPoint, the check is to make sure that we don't proceed with a PIE binary for which we weren't able to retrieve the entryPoint.

There are some tests calling LoadBinaryInfo with 0 as hardcoded entry point, namely TestDwarfVersion and TestRegabiFlagSentinel. These tests fail on windows/arm64 because the binary is PIE and the entry point is 0.

Anyway, I can revert this change and fix the tests by using a fake entry point, as they don't really need the entry point.

Yes, it's better to change the entry point used by the tests.

derekparker · 2022-07-29T16:30:20Z

That's an odd one, our Windows builders seem to be running fine. Is there anything different about your test / development systems that could cause such an error? I don't have a Windows machine handy to test on right now.

Found the culprit. I'm running the tests inside vscode, and for historical reasons they normalize the drive letter to lower-case. This shouldn't be a problem, because Windows file paths are case-insensitive, but proc.FindFileLocation is case sensitive, so it can't find the file even if it is there.

Ack, thanks for looking into that.

derekparker

First pass, just a couple things.

pkg/proc/arm64_arch.go

derekparker · 2022-08-16T16:47:13Z

pkg/proc/arm64_arch.go

-			default:
-				rule.Offset += crosscall2SPOffsetNonWindows
-			}
+			rule.Offset += crosscall2SPOffsetNonWindows


Do we need such specific constants now? E.g. can we get rid of Windows vs NonWindows specifiers?

We still need these constants, but I've updated their name to be more specific.

qmuntal · 2022-08-19T17:04:09Z

@aarzilli @derekparker I'm mostly done with this PR and all tests in pkg/proc are passing. Could you do one final review?

About the Windows ARM64 builder, anyone can register for the Azure Arm-based VMs public preview at https://forms.office.com/Pages/ResponsePage.aspx?id=v4j5cvGGr0GRqy180BHbRyMSy8VejZVEo6yZykiPSHpURFRIQVY1QTcyWTlJNURUS1pNTktOUTUxVi4u.
See the announcement here: https://azure.microsoft.com/en-us/blog/now-in-preview-azure-virtual-machines-with-ampere-altra-armbased-processors/

It is not free of cost, I can't do anything here, but I can help setting up the testing environment once the VM is up and running.

aarzilli

It is not free of cost, I can't do anything here

Steal an arm64 laptop and hide it in a closet.

aarzilli · 2022-08-19T18:52:37Z

pkg/proc/variables.go

@@ -438,8 +438,15 @@ func getGVariable(thread Thread) (*Variable, error) {

 	gaddr, hasgaddr := regs.GAddr()
 	if !hasgaddr {
-		var err error
-		gaddr, err = readUintRaw(thread.ProcessMemory(), regs.TLS()+thread.BinInfo().GStructOffset(), int64(thread.BinInfo().Arch.PtrSize()))
+		offset := thread.BinInfo().GStructOffset()


move this logic inside (*BinaryInfo).GStructOffset. Also instead of having a derefGStructOffset just check Arch.Name and BinaryInfo.GOOS.

move this logic inside (*BinaryInfo).GStructOffset.

If I understood you correctly, to do this I would have to update GStructOffset to accept a MemoryReader and return an error. Am I right?

Yes, that would be fine.

aarzilli

LGTM

derekparker

LGTM

nathanhammond · 2022-09-22T07:57:42Z

Thank you all for your work in getting this change landed! This will make a tremendous difference for our team and we're very appreciative. 🎉

aarzilli reviewed Jul 14, 2022

View reviewed changes

pkg/proc/native/syscall_windows_arm64.go Outdated Show resolved Hide resolved

pkg/proc/native/threads_windows.go Outdated Show resolved Hide resolved

qmuntal force-pushed the master branch from 4698dad to e199e15 Compare July 20, 2022 10:47

derekparker self-requested a review July 20, 2022 22:35

aarzilli reviewed Jul 21, 2022

View reviewed changes

pkg/proc/native/proc_windows.go Outdated Show resolved Hide resolved

qmuntal mentioned this pull request Jul 27, 2022

Refactor windows framework #3079

Merged

derekparker added the needs-rebase label Jul 27, 2022

qmuntal force-pushed the master branch 2 times, most recently from d235764 to 8405e78 Compare July 28, 2022 10:23

aarzilli reviewed Jul 29, 2022

View reviewed changes

qmuntal force-pushed the master branch from e1f622e to 8425389 Compare July 29, 2022 13:34

qmuntal mentioned this pull request Jul 29, 2022

Fix StepInstruction on 1 byte instruction with a software breakpoint #3084

Merged

derekparker reviewed Aug 16, 2022

View reviewed changes

qmuntal added 5 commits August 17, 2022 15:01

Add support for windows/arm64

d584dcd

split sentinel files and add winarm64 experiment

5f2a5aa

update loadBinaryInfoPE to support PIE binaries

7b12082

skip TestDump on windows/arm64

7914cc6

run windows/arm64 compilation on windows/amd64

4c55f39

qmuntal force-pushed the master branch from 8425389 to 4c55f39 Compare August 17, 2022 13:02

qmuntal added 4 commits August 17, 2022 15:25

add entry point check for pie binaries

2ed4da5

delete unusded code

477d2c1

document windows/arm64 breakpoint

1285e7e

implement changing windows/arm64 fp registers

9491586

qmuntal added 3 commits August 18, 2022 13:00

update crosscall offset names

86282a0

fix G load when using CGO

7cac1aa

fix testvariablescgo

d1b9c0f

aarzilli reviewed Aug 19, 2022

View reviewed changes

qmuntal added 2 commits August 22, 2022 17:48

remove DerefGStructOffset

453b401

derefrence gstructoffset in GStructOffset() if necessary

a529d29

aarzilli approved these changes Aug 23, 2022

View reviewed changes

derekparker approved these changes Sep 21, 2022

View reviewed changes

derekparker merged commit 4455d6a into go-delve:master Sep 21, 2022

qmuntal mentioned this pull request Nov 16, 2022

On-prem windows/arm64 builder #3197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for windows/arm64 #3063

Add support for windows/arm64 #3063

qmuntal commented Jul 13, 2022

aarzilli left a comment

qmuntal commented Jul 20, 2022

aarzilli commented Jul 21, 2022

qmuntal commented Jul 21, 2022

aarzilli commented Jul 21, 2022

qmuntal commented Jul 28, 2022

derekparker commented Jul 28, 2022

qmuntal commented Jul 29, 2022

aarzilli Jul 29, 2022

qmuntal Jul 29, 2022

aarzilli Jul 29, 2022

qmuntal Jul 29, 2022

aarzilli Jul 29, 2022

qmuntal Jul 29, 2022

aarzilli Jul 29, 2022

qmuntal Aug 17, 2022 •

edited

Loading

aarzilli Aug 19, 2022

derekparker commented Jul 29, 2022

derekparker left a comment

derekparker Aug 16, 2022

qmuntal Aug 18, 2022

qmuntal commented Aug 19, 2022

aarzilli left a comment

aarzilli Aug 19, 2022

qmuntal Aug 22, 2022

aarzilli Aug 23, 2022

qmuntal Aug 23, 2022

aarzilli left a comment

derekparker left a comment

nathanhammond commented Sep 22, 2022

Add support for windows/arm64 #3063

Add support for windows/arm64 #3063

Conversation

qmuntal commented Jul 13, 2022

aarzilli left a comment

Choose a reason for hiding this comment

qmuntal commented Jul 20, 2022

aarzilli commented Jul 21, 2022

qmuntal commented Jul 21, 2022

aarzilli commented Jul 21, 2022

qmuntal commented Jul 28, 2022

derekparker commented Jul 28, 2022

qmuntal commented Jul 29, 2022

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

qmuntal Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekparker commented Jul 29, 2022

derekparker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmuntal commented Aug 19, 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

aarzilli left a comment

Choose a reason for hiding this comment

derekparker left a comment

Choose a reason for hiding this comment

nathanhammond commented Sep 22, 2022

qmuntal Aug 17, 2022 •

edited

Loading