Navigation Menu

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
Closed

cmd/compile: bad small array comparisons on amd64 since go1.9 #23719

mjl- opened this issue Feb 6, 2018 · 15 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@mjl-
Copy link

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

mjl- 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 ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2018
@ALTree
Copy link
Member

ALTree commented Feb 6, 2018

cc @randall77 @cherrymui

@ALTree
Copy link
Member

ALTree commented Feb 6, 2018

Bisected to 3bdc2f3

@mjl-
Copy link
Author

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

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

mvdan commented Feb 6, 2018

/cc @TocarIP

@benpaxton-hf
Copy link

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

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

bradfitz commented Feb 6, 2018

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

@josharian
Copy link
Contributor

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

@TocarIP
Copy link
Contributor

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

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

@randall77
Copy link
Contributor

Reopening for possible backport to 1.9.5.

@randall77 randall77 reopened this Feb 6, 2018
@randall77 randall77 modified the milestones: Go1.10, Go1.9.5 Feb 6, 2018
@randall77 randall77 removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Feb 6, 2018
@andybons
Copy link
Member

andybons commented Mar 27, 2018

CL 103115 OK for Go 1.9.5

@gopherbot
Copy link

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
…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>
@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.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

10 participants