cmd/compile: encoding/binary.PutUint16 sometimes doesn't write

[sveeig]

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

$ go version go1.20.2 linux/amd64

Does this issue reproduce with the latest release?

yes, also reproduced with go 1.18 and 1.19

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/me/.cache/go-build"
GOENV="/home/me/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/me/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/me/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/go"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/me/my/project/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build460000454=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In certain situations binary.BigEndian.PutUint16 does not modify the targeted byte slice correctly.
See https://go.dev/play/p/OjvbLQZvYcn.

Details

	e := &struct{ flag bool }{flag: true}
	bufferOne := make([]byte, 2)
	bufferTwo := make([]byte, 2)
	var x uint16

	// This if statement is somehow important, must access heap, does not work with local bool
	if e.flag {
		x = 1
	}

	binary.BigEndian.PutUint16(bufferOne, x)

	// do operation that touches heap? print statement instead of Sleep also works, make slice and assign to a var also works
	time.Sleep(1 * time.Microsecond)

	binary.BigEndian.PutUint16(bufferTwo, x)

	fmt.Println("buffers should have same value:", x)
	fmt.Printf("bufferOne: %+v\n", bufferOne)
	fmt.Printf("bufferTwo: %+v", bufferTwo)

Note that the code runs correctly when a dlv debugger is attached to the program. The behaviour is the same if PutUint32 is used instead of PutUint16. Or if the shifted bytes are assigned directly like this:

	bufferOne[0] = byte(x >> 8)
	bufferOne[1] = byte(x)

	time.Sleep(1 * time.Microsecond)

	bufferTwo[0] = byte(x >> 8)
	bufferTwo[1] = byte(x)

What did you expect to see?

PutUint16 updates two buffers with the same value under given circumstances.

What did you see instead?

bufferTwo did not get updated or updated with wrong value.

[greatroar]

Shorter reproducer:

func main() {
	flag := new(bool)
	*flag = true

	var buf1, buf2 []byte
	buf2 = []byte{3} // This is somehow important. What is printed is still [0 0].

	var x uint16
	if *flag {
		x = 1
	}

	buf1 = []byte{byte(x >> 8), byte(x)}
	buf2 = []byte{byte(x >> 8), byte(x)}

	fmt.Println(buf1)
	fmt.Println(buf2)
}

[seankhliao]

cc @golang/compiler

[renthraysk]

Doesn't reproduce with setting GOAMD64=v3. Guess because MOVBEW instruction is available, so avoiding the problem.

[greatroar]

Probably. Doesn't reproduce on 386, arm or arm64, but it does on amd64 w/ Go 1.12 (!).

[randall77]

Looks like the 2-byte byte swap instruction we use here is not typed correctly. It gets a bool type instead of a uint16 type. So if it is spilled/restored, it loses its high 8 bits.

[gopherbot]

Change https://go.dev/cl/481395 mentions this issue: cmd/compile: use correct type for byteswaps on multi-byte stores

[randall77]

@gopherbot please open backport issues for 1.20 and 1.19.

[gopherbot]

Backport issue(s) opened: #59373 (for 1.19), #59374 (for 1.20).

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