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

cmd/compile: out-of-bounds panic with uint32 conversion and modulus operation in Go 1.22.0 on arm64 #66066

Closed
jnoxon opened this issue Mar 1, 2024 · 9 comments
Assignees
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.
Milestone

Comments

@jnoxon
Copy link

jnoxon commented Mar 1, 2024

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/jeff/Library/Caches/go-build'
GOENV='/Users/jeff/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/jeff/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/jeff/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/jeff/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.0.darwin-arm64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/jeff/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.0.darwin-arm64/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/jeff/go/src/github.com/spyderbat/crash-example/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/h8/9kxv3svd2bx_6hg8pxcg_skh0000gn/T/go-build1296351590=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

On linux/arm64 and darwin/arm64, with the latest compiler, this program crashes:

package main

import "fmt"

/*
This code demonstrates an out-of-bounds panic that occurs on go1.22.0 on linux/arm64 and darwin/arm64.

It does not panic on go1.21.6 on arm64, or on go1.22.0 on amd64.
It also does not panic when built with -gcflags="-N -l" on go1.22.0 on arm64.
*/

// if this function is inlined, the code will not panic
//
//go:noinline
func u32max() uint32 {
	// if bit 32 is clear, the code will not panic
	x := int64(1<<32 - 1)
	return uint32(x)
}

func main() {
	arr := []int{1, 2, 3}

	fmt.Printf("u32max: %d\n", u32max())
	fmt.Printf("len(arr): %d\n", len(arr))
	fmt.Printf("u32max mod len(arr): %d\n", u32max()%uint32(len(arr)))

	_ = arr[u32max()%uint32(len(arr))]
}

What did you see happen?

go run main.go

u32max: 4294967295
len(arr): 3
u32max mod len(arr): 0
panic: runtime error: index out of range [-4294967296]

goroutine 1 [running]:
main.main()
	/Users/jeff/go/src/github.com/jnoxon/crash-example/main.go:28 +0x150
exit status 2

What did you expect to see?

It should run the same as it does without optimizations:

go run -gcflags="-N -l" main.go

u32max: 4294967295
len(arr): 3
u32max mod len(arr): 0
@randall77
Copy link
Contributor

Hm, something very like this was fixed in 1.21.1. In fact, this program panics in 1.21.0 and doesn't in 1.21.1.
See #62131.
@RuinanSun

@jnoxon
Copy link
Author

jnoxon commented Mar 1, 2024

Hm, something very like this was fixed in 1.21.1. In fact, this program panics in 1.21.0 and doesn't in 1.21.1. See #62131. @RuinanSun

Yeah, definitely the same issue. Thanks! I'll close this. Long day, old version. I'll leave this open for triage.

@jnoxon jnoxon closed this as completed Mar 1, 2024
@jnoxon jnoxon reopened this Mar 1, 2024
@randall77
Copy link
Contributor

Probably reintroduced at CL 521496

@seankhliao seankhliao changed the title arm64: out-of-bounds panic with uint32 conversion and modulus operation in Go 1.22.0 cmd/compile: out-of-bounds panic with uint32 conversion and modulus operation in Go 1.22.0 on arm64 Mar 1, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 1, 2024
@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. arch-arm64 labels Mar 1, 2024
@randall77
Copy link
Contributor

@gopherbot please open a backport issue for 1.22.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/568616 mentions this issue: cmd/compile: fix sign/zero-extension removal

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #66076 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@BlackWaters
Copy link

Hi, this is Ruinan

Thanks for the quick fix! @randall77

After some tests, I find more issues:

There are many rules like (Op32bit ...) => (MOVWUreg ...), here are some examples:

(MULW x (MOVDconst [c])) && isPowerOfTwo64(c+1) && int32(c) >= 7 => (MOVWUreg (ADDshiftLL <x.Type> (NEG <x.Type> x) x [log64(c+1)]))
(MULW x (MOVDconst [c])) && c%3 == 0 && isPowerOfTwo64(c/3) && is32Bit(c) => (MOVWUreg (SLLconst <x.Type> [log64(c/3)] (ADDshiftLL <x.Type> x x [1])))
(MULW x (MOVDconst [c])) && c%5 == 0 && isPowerOfTwo64(c/5) && is32Bit(c) => (MOVWUreg (SLLconst <x.Type> [log64(c/5)] (ADDshiftLL <x.Type> x x [2])))
(MULW x (MOVDconst [c])) && c%7 == 0 && isPowerOfTwo64(c/7) && is32Bit(c) => (MOVWUreg (SLLconst <x.Type> [log64(c/7)] (ADDshiftLL <x.Type> (NEG <x.Type> x) x [3])))
(MULW x (MOVDconst [c])) && c%9 == 0 && isPowerOfTwo64(c/9) && is32Bit(c) => (MOVWUreg (SLLconst <x.Type> [log64(c/9)] (ADDshiftLL <x.Type> x x [3])))

The most tricky part is that the type of generated MOVWUreg will not change, which remains the same as the Op32bit. So if the type width of Op32bit is less than 4, then the MOVWUreg will be omitted by the following rules:

// Don't bother extending if we're not using the higher bits.
(MOV(B|BU)reg x) && v.Type.Size() <= 1 => x
(MOV(H|HU)reg x) && v.Type.Size() <= 2 => x
(MOV(W|WU)reg x) && v.Type.Size() <= 4 => x

A small code case:

package main

//go:noinline
func test(a uint32) uint64 {
	return uint64(uint32(a * 3))
}

func main() {
	println(uint64(test(4294967294))) // should be same
	println(uint32(test(4294967294)))
	return
}

/* results:
18446744073709551610
4294967290
*/

This will also trash the high 32 bits.

So we have to be very careful when we're generating MOVWUreg and such Opcodes. Maybe need some type restrictions.

And now I no longer work at Arm, so this account @RuinanSun will be deprecated. I can only contribute to Go in my spare time. If this is not in a hurry, I will do a CL after your fix.

Thanks!
Ruinan

@randall77
Copy link
Contributor

Interesting.

I don't think the rules you quote are actually broken.
The MOVWUreg generated as part of the MULW rewrite is often going to be useless, and the MULW will typically have 32-bit type. A MOVWUreg with a 32-bit type is, as commented on the extension removal rules, useless. However, if the MULW has 64-bit type, indicating the user of that value needs all 64 bits of the result, then the MOVWUreg is needed.

I think the actual problem then is this rule:

(MOVWUreg x:(MOVWUreg _)) => (MOVDreg x)

If the outer one has 64-bit type and the inner one has 32-bit type, then we're removing the extension we actually need.
If I disable this rule your example no longer fails.

I will try to incorporate this error+fix into my CL.

@mknyszek mknyszek added this to the Backlog milestone Mar 6, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/571135 mentions this issue: cmd/compile: don't assume args are always zero-extended

gopherbot pushed a commit that referenced this issue Mar 20, 2024
On amd64, we always zero-extend when loading arguments from the stack.
On arm64, we extend based on the type. This causes problems with
zeroUpper*Bits, which reports the top bits are zero when they aren't.

Fix it to use the type to decide if the top bits are really zero.

For tests, only f32 currently fails on arm64. Added other tests
just for future-proofing.

Update #66066

Change-Id: I2f13fb47198e139ef13c9a34eb1edc932eea3ee3
Reviewed-on: https://go-review.googlesource.com/c/go/+/571135
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Mar 20, 2024
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.
Projects
Development

No branches or pull requests

7 participants