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

runtime: runtime.Breakpoint() generates illegal instruction on windows/arm64 #53837

Closed
qmuntal opened this issue Jul 13, 2022 · 20 comments
Closed
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Jul 13, 2022

What version of Go are you using (go version)?

go version go1.19rc1 windows/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
set GO111MODULE=
set GOARCH=arm64
set GOBIN=
set GOCACHE=C:\Users\qmuntaldiaz\AppData\Local\go-build
set GOENV=C:\Users\qmuntaldiaz\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=arm64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\qmuntaldiaz\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\qmuntaldiaz\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_arm64
set GOVCS=
set GOVERSION=go1.19rc1
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\qmuntaldiaz\code\netabort\go.mod
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-mthreads -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\QMUNTA~1\AppData\Local\Temp\go-build3550564676=/tmp/go-build -gno-record-gcc-switches

What did you do?

package main

import "runtime"

func main() {
	runtime.Breakpoint()
        println("Hello World!")
}

go run .

What did you expect to see?

Edited: exit status 0xc0000409
Hello World!

What did you see instead?

Program crashes due to an ilegal instruction:

fatal error: fault
[signal 0xc000001d code=0x0 addr=0x0 pc=0x7ff70c383180]

goroutine 1 [running]:
runtime.throw({0x7ff70c399235?, 0x0?})
        C:/Program Files/Go/src/runtime/panic.go:1047 +0x40 fp=0x4000049f00 sp=0x4000049ed0 pc=0x7ff70c35f3b0
runtime.sigpanic()
        C:/Program Files/Go/src/runtime/signal_windows.go:273 +0x1a0 fp=0x4000049f50 sp=0x4000049f00 pc=0x7ff70c371830
runtime.breakpoint()
        C:/Program Files/Go/src/runtime/asm_arm64.s:111 fp=0x4000049f60 sp=0x4000049f60 pc=0x7ff70c383180
runtime.Breakpoint(...)
        C:/Program Files/Go/src/runtime/proc.go:4359
main.main()
        C:/Users/qmuntaldiaz/code/netabort/main.go:6 +0x20 fp=0x4000049f70 sp=0x4000049f60 pc=0x7ff70c38b860
runtime.main()
        C:/Program Files/Go/src/runtime/proc.go:250 +0x234 fp=0x4000049fd0 sp=0x4000049f70 pc=0x7ff70c361884
runtime.goexit()
        C:/Program Files/Go/src/runtime/asm_arm64.s:1155 +0x4 fp=0x4000049fd0 sp=0x4000049fd0 pc=0x7ff70c385834

goroutine 2 [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
        C:/Program Files/Go/src/runtime/proc.go:363 +0xe4 fp=0x4000045fa0 sp=0x4000045f80 pc=0x7ff70c361c34
runtime.goparkunlock(...)
        C:/Program Files/Go/src/runtime/proc.go:369
runtime.forcegchelper()
        C:/Program Files/Go/src/runtime/proc.go:302 +0xb4 fp=0x4000045fd0 sp=0x4000045fa0 pc=0x7ff70c361ac4
runtime.goexit()
        C:/Program Files/Go/src/runtime/asm_arm64.s:1155 +0x4 fp=0x4000045fd0 sp=0x4000045fd0 pc=0x7ff70c385834
created by runtime.init.6
        C:/Program Files/Go/src/runtime/proc.go:290 +0x24

goroutine 3 [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
        C:/Program Files/Go/src/runtime/proc.go:363 +0xe4 fp=0x4000047f70 sp=0x4000047f50 pc=0x7ff70c361c34
runtime.goparkunlock(...)
        C:/Program Files/Go/src/runtime/proc.go:369
runtime.bgsweep(0x0?)
        C:/Program Files/Go/src/runtime/mgcsweep.go:278 +0xa4 fp=0x4000047fb0 sp=0x4000047f70 pc=0x7ff70c34de64
runtime.gcenable.func1()
        C:/Program Files/Go/src/runtime/mgc.go:178 +0x28 fp=0x4000047fd0 sp=0x4000047fb0 pc=0x7ff70c342598
runtime.goexit()
        C:/Program Files/Go/src/runtime/asm_arm64.s:1155 +0x4 fp=0x4000047fd0 sp=0x4000047fd0 pc=0x7ff70c385834
created by runtime.gcenable
        C:/Program Files/Go/src/runtime/mgc.go:178 +0x70

goroutine 4 [GC scavenge wait]:
runtime.gopark(0x40000180e0?, 0x7ff70c3b0358?, 0x1?, 0x0?, 0x0?)
        C:/Program Files/Go/src/runtime/proc.go:363 +0xe4 fp=0x4000055f50 sp=0x4000055f30 pc=0x7ff70c361c34
runtime.goparkunlock(...)
        C:/Program Files/Go/src/runtime/proc.go:369
runtime.(*scavengerState).park(0x7ff70c3f3200)
        C:/Program Files/Go/src/runtime/mgcscavenge.go:389 +0x5c fp=0x4000055f80 sp=0x4000055f50 pc=0x7ff70c34be5c
runtime.bgscavenge(0x0?)
        C:/Program Files/Go/src/runtime/mgcscavenge.go:617 +0x44 fp=0x4000055fb0 sp=0x4000055f80 pc=0x7ff70c34c3c4
runtime.gcenable.func2()
        C:/Program Files/Go/src/runtime/mgc.go:179 +0x28 fp=0x4000055fd0 sp=0x4000055fb0 pc=0x7ff70c342538
runtime.goexit()
        C:/Program Files/Go/src/runtime/asm_arm64.s:1155 +0x4 fp=0x4000055fd0 sp=0x4000055fd0 pc=0x7ff70c385834
created by runtime.gcenable
        C:/Program Files/Go/src/runtime/mgc.go:179 +0xb4
exit status 2
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 13, 2022

Windows ARM64 expects a breakpoint to be compiled to the instruction BRK #0xF000 (see docs), but we are compiling it to just BRK, which does work for Linux and others.

go/src/runtime/asm_arm64.s

Lines 110 to 112 in bf2ef26

TEXT runtime·breakpoint(SB),NOSPLIT|NOFRAME,$0-0
BRK
RET

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 13, 2022
@mknyszek mknyszek added this to the Backlog milestone Jul 13, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Jul 13, 2022

This seems like it would've been a problem since the origin of the port. This isn't new in 1.19 is it?

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 13, 2022

This seems like it would've been a problem since the origin of the port. This isn't new in 1.19 is it?

That's right.

Some more context: I found this while porting go-delve to windows/arm64, and it uses runtime.Breakpoint() in a couple of unit tests.

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 13, 2022

I naively tried to fix it updating the runtime.Breakpoint() implementation writing the instruction directly in its byte representation, but I'm new to Go assembly and it may be doing it wrong, as it is not working:

TEXT runtime·breakpoint(SB),NOSPLIT|NOFRAME,$0-0
#ifdef GOOS_windows
	WORD $0x00003ed4 // BRK #0xF000
#else
	BRK
#endif
	RET

@aarzilli
Copy link
Contributor

aarzilli commented Jul 13, 2022

What does the argument do?

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 13, 2022

What does the argument do?

I don't know. Found it thanks to this llvm issue: https://www.mail-archive.com/lldb-dev@lists.llvm.org/msg09058.html and confirmed llocally that it works.

@cherrymui
Copy link
Member

cherrymui commented Jul 14, 2022

What did you expect to see?
Hello World!

I don't think we should expect a successful run. Usually runtime.Breakpoint generates some sort of trap (usually SIGTRAP on UNIX-y systems).

Maybe Windows doesn't distinguish a trap vs. an illegal instruction exception? Is there a SIGTRAP-like thing on Windows?

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 14, 2022

I don't think we should expect a successful run. Usually runtime.Breakpoint generates some sort of trap (usually SIGTRAP on UNIX-y systems).

Maybe Windows doesn't distinguish a trap vs. an illegal instruction exception? Is there a SIGTRAP-like thing on Windows?

My bad, it does not end successfully on windows/amd64 nor print Hello World, but exists with STATUS_FAIL_FAST_EXCEPTION if nobody catch the initial EXCEPTION_DEBUG_EVENT exception.

The problem on windows/arm64 is that BRK makes it crash with EXCEPTION_ILLEGAL_INSTRUCTION, while a debugger is expecting EXCEPTION_DEBUG_EVENT. Using BRK #f000 instead would solve the issue.

@beoran
Copy link

beoran commented Jul 14, 2022

https://developer.arm.com/documentation/ddi0602/2022-06/Base-Instructions/BRK--Breakpoint-instruction-?lang=en

The BRK instructions needs an immediate 16 bits argument.

It seems like the operating system could inspect the immediate set by BRK and then send a different signal to the process depending on the value. BRK without the immediate would then become a shorthand for BRK 0.

@cherrymui
Copy link
Member

cherrymui commented Jul 14, 2022

Thanks. Sounds like changing the function body to BRK $0xf000 should make it work? Could you try that? Does it also work on UNIX-y systems?

Is there a document about what constant Windows expects? The ARM instruction manual doesn't seem to mention what the constant does exactly.

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 14, 2022

I can confirm that changing runtime.Breakpoint intrinsic to the following code make Windows happy:

TEXT runtime·breakpoint(SB),NOSPLIT|NOFRAME,$0-0
	BRK $0xf000
	RET

I don't have a Linux ARM64 machine, so can't test it.

Is there a document about what constant Windows expects? The ARM instruction manual doesn't seem to mention what the constant does exactly.

The only place I've seen Windows docs mention the BRK argument for ARM64 is here: https://docs.microsoft.com/en-us/cpp/intrinsics/debugbreak?view=msvc-170#remarks

On ARM64, the __debugbreak intrinsic is compiled into the instruction brk #0xF000.

@cherrymui
Copy link
Member

cherrymui commented Jul 14, 2022

I tried BRK $0xf000 on Linux/ARM64 and macOS/ARM64, and both generate SIGTRAP, the same as before. So it should be fine. Feel free to send a CL.

@aarzilli does it sound okay to you?

@cherrymui
Copy link
Member

cherrymui commented Jul 14, 2022

If we do this change, https://cs.opensource.google/go/go/+/master:src/runtime/export_debug_arm64_test.go;l=24 probably needs update. https://cs.opensource.google/go/go/+/master:src/runtime/signal_darwin_arm64.go;l=85 should probably be made to understand all BRK instructions.

@aarzilli
Copy link
Contributor

aarzilli commented Jul 14, 2022

Changing BRK to BRK $0xf000 will break old versions of delve with new versions of go. This happens frequently, so it's fine to do, but something to consider. The debugCall code also has a bunch of BRK instructions that would have to be changed similarly.

Maybe it's the assembler that should be changed to emit BRK $0xf000 when it sees BRK (either on windows only or everywhere, depending what is chosen).

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 14, 2022

I tried BRK $0xf000 on Linux/ARM64 and macOS/ARM64, and both generate SIGTRAP, the same as before. So it should be fine. Feel free to send a CL.

Wouldn't it be better to add the argument just on windows/arm64? Who knows what can we break on older unix/darwin machines with this change.

@gopherbot
Copy link

gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418734 mentions this issue: runtime: fix runtime.Breakpoint() on windows/arm64

@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 21, 2022

@cherrymui change submitted. I've finally only set the argument on windows/arm64 to avoid breaking changes, what do you think?

@ianlancetaylor Could we make this into go1.19? It is low risk and will facilitate Delve support for windows/arm64.

@cherrymui
Copy link
Member

cherrymui commented Jul 21, 2022

I've finally only set the argument on windows/arm64 to avoid breaking changes, what do you think?

SGTM. Thanks.

I would think we don't change the assembler to emit a different instruction for BRK, which is (technically) a breaking change. And one can always use BRK $nnn if necessary.

facilitate Delve support for windows/arm64.

Does Delve require a specific type of exception? Just curious.

@aarzilli
Copy link
Contributor

aarzilli commented Jul 22, 2022

Does Delve require a specific type of exception? Just curious.

Delve does different things with different types of exceptions, most of them just get sent back to the target process. It would be possible to check what the illegal instruction is and if it is a BRK (with any argument) treat it as if it was a breakpoint (I'm assuming here that illegal instruction exceptions do get sent to the debugger first, I haven't checked). IMO however it is better to not fight the os on this and just use the instruction microsoft wants us to use.

@cherrymui
Copy link
Member

cherrymui commented Jul 22, 2022

Sounds good. Thanks!

@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Jul 25, 2022
@dmitshur dmitshur removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 25, 2022
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 25, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
Fixes golang#53837

Change-Id: I4219fe35aac1a88aae2905998fbb1d7db87bbfb2
Reviewed-on: https://go-review.googlesource.com/c/go/+/418734
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
Status: Done
Development

No branches or pull requests

7 participants