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

jnoxon · 2024-03-01T20:24:57Z

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

The text was updated successfully, but these errors were encountered:

randall77 · 2024-03-01T21:11:45Z

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 · 2024-03-01T21:16:37Z

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.

randall77 · 2024-03-01T21:30:44Z

Probably reintroduced at CL 521496

randall77 · 2024-03-03T03:55:31Z

@gopherbot please open a backport issue for 1.22.

gopherbot · 2024-03-03T03:55:53Z

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

gopherbot · 2024-03-03T03:55:55Z

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 · 2024-03-04T12:13:04Z

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:

go/src/cmd/compile/internal/ssa/_gen/ARM64.rules

Lines 1228 to 1232 in 6f5d774

    
           (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:

go/src/cmd/compile/internal/ssa/_gen/ARM64.rules

Lines 1579 to 1582 in 6f5d774

    
           // 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 · 2024-03-04T16:26:14Z

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.

gopherbot · 2024-03-12T20:01:26Z

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

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>

jnoxon closed this as completed Mar 1, 2024

jnoxon reopened this Mar 1, 2024

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 added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 1, 2024

seankhliao added NeedsFix The path to resolution is known, but the work has not been done. arch-arm64 labels Mar 1, 2024

gopherbot mentioned this issue Mar 3, 2024

cmd/compile: out-of-bounds panic with uint32 conversion and modulus operation in Go 1.22.0 on arm64 [1.22 backport] #66076

Closed

mknyszek added this to the Backlog milestone Mar 6, 2024

mknyszek assigned randall77 Mar 6, 2024

gopherbot closed this as completed in a46ecdc Mar 12, 2024

dmitshur modified the milestones: Backlog, Go1.23 Mar 20, 2024

seankhliao mentioned this issue Mar 20, 2024

go compiler: Incorrect results on GOOS=linux GOARCH=arm64 running on Google t2a-standard-8 (Ampere Altra) #66431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jnoxon commented Mar 1, 2024

randall77 commented Mar 1, 2024

jnoxon commented Mar 1, 2024 •

edited

randall77 commented Mar 1, 2024

randall77 commented Mar 3, 2024

gopherbot commented Mar 3, 2024

gopherbot commented Mar 3, 2024

BlackWaters commented Mar 4, 2024

randall77 commented Mar 4, 2024

gopherbot commented Mar 12, 2024

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

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

Comments

jnoxon commented Mar 1, 2024

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

randall77 commented Mar 1, 2024

jnoxon commented Mar 1, 2024 • edited

randall77 commented Mar 1, 2024

randall77 commented Mar 3, 2024

gopherbot commented Mar 3, 2024

gopherbot commented Mar 3, 2024

BlackWaters commented Mar 4, 2024

randall77 commented Mar 4, 2024

gopherbot commented Mar 12, 2024

Output of `go env` in your module/workspace:

jnoxon commented Mar 1, 2024 •

edited