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: bad small array comparisons on amd64 since go1.9 #23719

Closed
mjl- opened this issue Feb 6, 2018 · 15 comments

Comments

Projects
None yet
10 participants
@mjl-
Copy link

commented Feb 6, 2018

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

$ go1.10rc1 version
go version go1.10rc1 darwin/amd64
$ go1.9.3 version
go version go1.9.3 darwin/amd64
$ go1.9 version
go version go1.9 darwin/amd64
$ go1.8.6 version
go version go1.8.6 darwin/amd64

Does this issue reproduce with the latest release?

yes, and tip

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mjl/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/c6/wjpsl76s11l9cn3rv7lzjvq00000gn/T/go-build257631275=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

https://play.golang.org/p/JSCb-q6cHiE

compare an [2]int32 array.
go for amd64 incorrectly says they are equal, even if the elements at indices 1 don't match.
longer/shorter arrays do compare properly.
[2]int64 does compare correctly.
this only seems broken on amd64, since go1.9.
compiling with GOARCH=386 gives correct behaviour, same for arm.

more elaborate program with more cases:

$ cat test.go
package main

import (
	"fmt"
)

func main() {
	// the first one seems wrong, should not be equal
	func() {
		v1 := [2]int32{-102,-102}
		v2 := [2]int32{-102,1126}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()

	func() {
		v1 := [2]int64{-102,-102}
		v2 := [2]int64{-102,1126}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()

	func() {
		v1 := [2]int32{-102, -102}
		v2 := [2]int32{1126, -102}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()

	func() {
		v1 := [3]int32{-102,-102, 1}
		v2 := [3]int32{-102,1126, 2}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()

	func() {
		v1 := [3]int32{-102,-102, 1}
		v2 := [3]int32{-102,-102, 2}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()

	func() {
		v1 := [1]int32{-102}
		v2 := [1]int32{-102}
		fmt.Printf("%#v == %#v is %v\n", v1, v2, v1 == v2)
	}()
}
$ go1.8.6 run test.go
[2]int32{-102, -102} == [2]int32{-102, 1126} is false

$ go1.9 run test.go 
[2]int32{-102, -102} == [2]int32{-102, 1126} is true

$ go1.9.3 run test.go
[2]int32{-102, -102} == [2]int32{-102, 1126} is true

$ go1.10rc1 run test.go
[2]int32{-102, -102} == [2]int32{-102, 1126} is true

$ /Users/mjl/src/go/bin/go version
go version devel +fd7331a821 Tue Feb 6 05:00:01 2018 +0000 darwin/amd64
$ /Users/mjl/src/go/bin/go run test.go 
[2]int32{-102, -102} == [2]int32{-102, 1126} is true


$ GOARCH=386 go1.10rc1 build
$ ./intarraybug 
[2]int32{-102, -102} == [2]int32{-102, 1126} is false

pi@raspberrypi$ go version
go version go1.9.2 linux/arm
pi@raspberrypi$ go run test.go 
[2]int32{-102, -102} == [2]int32{-102, 1126} is false

What did you expect to see?

[2]int32{-102, -102} == [2]int32{-102, 1126} is false

What did you see instead?

[2]int32{-102, -102} == [2]int32{-102, 1126} is true

@mjl-

This comment has been minimized.

Copy link
Author

commented Feb 6, 2018

more cases that go wrong (including int16 and int8 on 386), by GinjaNinja32 on #go-nuts.

https://play.golang.org/p/dE-EIJgGm2V

@ALTree ALTree changed the title wrong int32[2] comparison on amd64 since go1.9 (including 1.10rc1) cmd/compile: wrong int32[2] comparison on amd64 since go1.9 (including 1.10rc1) Feb 6, 2018

@ALTree

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

@ALTree

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

Bisected to 3bdc2f3

@mjl-

This comment has been minimized.

Copy link
Author

commented Feb 6, 2018

generated cases that go bad, again by GinjaNinja32:
https://ptpb.pw/UMjM

seems first array element must be negative to trigger.

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

Not a regression since 1.9, and it's very late in the cycle, so I'm milestoning to 1.11 for now.

@mvdan mvdan added this to the Go1.11 milestone Feb 6, 2018

@mvdan mvdan changed the title cmd/compile: wrong int32[2] comparison on amd64 since go1.9 (including 1.10rc1) cmd/compile: bad small array comparisons on amd64 since go1.9 Feb 6, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

/cc @TocarIP

@benpaxton-hf

This comment has been minimized.

Copy link

commented Feb 6, 2018

Does conv() here convert with sign extension? If so, I think that's the issue - if the array is, say, [2]int8{-3, 2}, the combined int16 will be int16(-3) | (int16(2) << 8), or 0xFFFD, which is not the same as the value of the array read as an int16, which would be (int16(-3) & 0xFF) | (int16(2) << 8) or 0x02FD.

This would explain why it only happens after a negative element within the same combined integer - if all elements are positive, then the sign extension gives 0s, which is correct.

edit: based on this, and a bit of poking around that package, conv() is OCONV which for signed integers is sign-extending.

@ALTree

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

This looks bad enough that it should probably be a go1.10/NeedsDecision issue, since we may decide to just rollback the optimization CL that I linked above before the 1.10 release, to avoid having broken array comparisons for 6 more months..

@bradfitz bradfitz modified the milestones: Go1.11, Go1.10 Feb 6, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

@ALTree, okay, moved to Go1.10 for a decision. @rsc, @ianlancetaylor?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

Agreed, this needs to at least be looked at for 1.10.

@TocarIP

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

@benpaxton-hf You are absolutely right, this is caused by sign extension for signed types.
Looks like converting to same-sized unsigned int first, fixes this.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 6, 2018

Change https://golang.org/cl/92335 mentions this issue: cmd/compile: use unsigned loads for multi-element comparisons

@gopherbot gopherbot closed this in 23e8e19 Feb 6, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

Reopening for possible backport to 1.9.5.

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

CL 103115 OK for Go 1.9.5

@gopherbot

This comment has been minimized.

Copy link

commented Mar 28, 2018

Change https://golang.org/cl/103115 mentions this issue: [release-branch.go1.9] cmd/compile: use unsigned loads for multi-element comparisons

gopherbot pushed a commit that referenced this issue Mar 29, 2018

[release-branch.go1.9] cmd/compile: use unsigned loads for multi-elem…
…ent comparisons

When loading multiple elements of an array into a single register,
make sure we treat them as unsigned.  When treated as signed, the
upper bits might all be set, causing the shift-or combo to clobber
the values higher in the register.

Fixes #23719.

Change-Id: Ic87da03e9bd0fe2c60bb214b99f846e4e9446052
Reviewed-on: https://go-review.googlesource.com/92335
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-on: https://go-review.googlesource.com/103115
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

@andybons andybons closed this Mar 29, 2018

@golang golang locked and limited conversation to collaborators Mar 29, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.