cmd/compile: max/min builtin broken when used with string(byte) conversions

[mighmi]

Go version

go1.21.0 linux/amd64

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

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/eggplant/.cache/go-build'
GOENV='/home/eggplant/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'

What did you do?

Here is a proof https://go.dev/play/p/Dr26MV2mvGW from https://www.reddit.com/r/golang/comments/18be9xl/max_and_assign_issue/

Credit to /u/ar1819 and /u/YamadaAnna

What did you expect to see?

max() and slices.Max() should have the same results. 7 is the bigger number.

What did you see instead?

max() output numbers lower than 7 as maxes.

[mateusz834]

Simpler reproducer:

func main() {
	num := "765"
	maxSymbol := ""
	for i := 0; i < len(num); i++ {
		newMax := max(string(num[i]), maxSymbol)
		fmt.Printf("max(%q, %q)=%q\n", string(num[i]), maxSymbol, newMax)
		maxSymbol = newMax
	}
}

$ go run main.go
max("7", "")="7"
max("6", "6")="6"
max("5", "5")="5"

Making a tmp variable fixes this issue.

func main() {
	num := "765"
	maxSymbol := ""
	for i := 0; i < len(num); i++ {
		tmp := string(num[i])
		newMax := max(tmp, maxSymbol)
		fmt.Printf("max(%q, %q)=%q\n", tmp, maxSymbol, newMax)
		maxSymbol = newMax
	}
}

$ go run main.go
max("7", "")="7"
max("6", "7")="7"
max("5", "7")="7"

EDIT: The same thing happens with min.

[mateusz834]

CC @golang/compiler

[gazerro]

Note that the commit title is not accurate. string(num[i]) is not a string(rune) conversion but rather a string(byte) conversion.

[mdempsky]

This appears to be an escape analysis issue.

@gopherbot Please backport to Go 1.21. This is a silent miscompilation / memory corruption issue.

[gopherbot]

Backport issue(s) opened: #64567 (for 1.21).

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

[mauri870]

I find it weird that in this reproducer:

func main() {
	num := "765"
	maxSymbol := ""
	for i := 0; i < len(num); i++ {
		newMax := max(string(num[i]), maxSymbol)
		fmt.Printf("max(%q, %q)=%q\n", string(num[i]), maxSymbol, newMax)
		maxSymbol = newMax
	}
}

If you stop a debugger in the newMax assignment line where i == 1, maxSymbol has a value of 7, if you step to the next line maxSymbol suddenly has a value of 6.

[gopherbot]

Change https://go.dev/cl/547715 mentions this issue: cmd/compile: fix escape analysis of string min/max

[mdempsky]

Thanks all for the report. FWIW, since there's some interest in the issue, here's an explanation of what's going on.

Starting from this reproducer:

package main

import "fmt"

func main() {
	m := "0"
	for _, c := range "321" {
		m = max(string(c), m)
		fmt.Println(m)
	}
}

The compiler transforms this into basically:

func main() {
        m := "0"
	for _, c := range "321" {
                var tmp = intstring(new([4]byte), int64(c))
		m = strmax(tmp, m)
		fmt.Println(m)
	}
}

where intstring and strmax are provided by the runtime.

However, the compiler was not correctly taking into consideration that strmax returns one of its arguments. That is, it didn't realize m could point to the memory allocated by new([4]byte). As a result, it mistakenly thought it was safe to reuse that memory allocation across loop iterations, so it further transformed the code into:

func main() {
        m := "0"
        var buf [4]byte
	for _, c := range "321" {
                var tmp = intstring(&buf, int64(c)) // BAD: reuses buf, even though m may refer to it
		m = strmax(tmp, m)
		fmt.Println(m)
	}
}

[mauri870]

Thank you for taking the time to provide such a detailed explanation, much appreciated!

[gopherbot]

Change https://go.dev/cl/547757 mentions this issue: [release-branch.go1.21] cmd/compile: fix escape analysis of string min/max