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

Jump to bottom

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

Closed
rompetroll opened this issue Apr 1, 2023 · 8 comments
Closed

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

rompetroll opened this issue Apr 1, 2023 · 8 comments
Assignees
@randall77
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@rompetroll
Copy link

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. 

	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.
@seankhliao seankhliao changed the title affected/package: encoding/binary cmd/compile: encoding/binary.PutUint16 sometimes doesn't write Apr 1, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 1, 2023
@greatroar
Copy link

greatroar commented Apr 1, 2023 •
edited

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
Copy link
Member

cc @golang/compiler

@renthraysk
Copy link

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

@greatroar
Copy link

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

@randall77
Copy link
Contributor

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
Copy link

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

@randall77
Copy link
Contributor

@gopherbot please open backport issues for 1.20 and 1.19.

This was referenced Apr 1, 2023
Closed
Closed
@gopherbot
Copy link

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.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 3, 2023
@bcmills bcmills added this to the Go1.21 milestone Apr 3, 2023
@golang golang locked and limited conversation to collaborators Apr 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@randall77 randall77

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
7 participants
@rompetroll @bcmills @randall77 @gopherbot @seankhliao @renthraysk @greatroar