-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
more cases that go wrong (including int16 and int8 on 386), by GinjaNinja32 on #go-nuts. |
Bisected to 3bdc2f3 |
generated cases that go bad, again by GinjaNinja32: seems first array element must be negative to trigger. |
Not a regression since 1.9, and it's very late in the cycle, so I'm milestoning to 1.11 for now. |
/cc @TocarIP |
Does 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, |
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.. |
@ALTree, okay, moved to Go1.10 for a decision. @rsc, @ianlancetaylor? |
Agreed, this needs to at least be looked at for 1.10. |
@benpaxton-hf You are absolutely right, this is caused by sign extension for signed types. |
Change https://golang.org/cl/92335 mentions this issue: |
Reopening for possible backport to 1.9.5. |
CL 103115 OK for Go 1.9.5 |
Change https://golang.org/cl/103115 mentions this issue: |
…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>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes, and tip
What operating system and processor architecture are you using (
go env
)?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:
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
The text was updated successfully, but these errors were encountered: