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: wrong calculation result for bit operation that's inlined and has all constant shifts in rewrite rules #32680
Comments
Hello @chanxuehong, thank you for this report! In order to help isolate the bug, I have made a simple modification to show where the problem is 146 254 150 46 781647506
75f7453db3643a46136d82e46c82f80c
1ef94ebae8013e871c555d8ec5d89ab6
75f7453db3643a46136d82e46c82f80c on systems that provide inlining and then 146 254 150 46 781647506
75f7453db3643a46136d82e46c82f80c
75f7453db3643a46136d82e46c82f80c
75f7453db3643a46136d82e46c82f80c on systems such the Go playground and when we turn off inlining with The issue seems to be in func readLittleEndian32_2(a, b, c, d byte) uint32 {
return uint32(a) | (uint32(b) << 8) | (uint32(c) << 16) | (uint32(d) << 24)
} where there we are inlining. I believe in readLittleEndian32_1 you added the extra Here is a minimal repro package main
import (
"fmt"
)
var foo = []byte{105, 57, 172, 152, 175, 139, 47, 108}
func main() {
for i := 0; i < len(foo); i += 4 {
fmt.Println(readLittleEndian32_1(foo[i], foo[i+1], foo[i+2], foo[i+3]))
fmt.Println(readLittleEndian32_2(foo[i], foo[i+1], foo[i+2], foo[i+3]))
fmt.Println(readLittleEndian32_3(foo[i], foo[i+1], foo[i+2], foo[i+3]))
fmt.Println("\n\n")
}
}
var x = false
func readLittleEndian32_1(a, b, c, d byte) uint32 {
r := uint32(a) | (uint32(b) << 8) | (uint32(c) << 16) | (uint32(d) << 24)
if !x {
x = true
fmt.Println(a, b, c, d, r)
}
return r
}
func readLittleEndian32_2(a, b, c, d byte) uint32 {
return uint32(a) | (uint32(b) << 8) | (uint32(c) << 16) | (uint32(d) << 24)
}
func readLittleEndian32_3(a, b, c, d byte) uint32 {
return (uint32(a) & 0xFF) | ((uint32(b) << 8) & 0xFF00) | ((uint32(c) << 16) & 0xFF0000) | ((uint32(d) << 24) & 0xFF000000)
} which prints 105 57 172 152 2561423721
2561423721
2561409216
2561423721
1815055279
1815034217
1815055279 and as we can see Kindly paging some compiler/math experts @randall77 @josharian @dr2chase @rasky |
Apologies for the false page @dr2chase @rsc: it was a manual edit from me that falsely produced the bug, and moreover at 1AM so my mind was a little fuzzy @randall77 seems like https://go-review.googlesource.com/38801 aka 53f8a6a#diff-e5f79fbc16c194021eb0476790c8a606, the commit before it doesn't have the problem, the one after has it, please help me confirm. Go1.8 and below are safe, from Go1.9 and beyond have this bug thus a couple of backports shall be needed. |
A variant of the test program causes a complier crash:, in both at least go1.11 and tip:
Also crashes in the playground: https://play.golang.org/p/MzGNDnB7hDF |
FYI, fix is coming, won't be ready for submit till tomorrow AM (needs a test), will set it up for review tonight. |
Change https://golang.org/cl/183059 mentions this issue: |
Change https://golang.org/cl/183239 mentions this issue: |
Change https://golang.org/cl/183240 mentions this issue: |
@gopherbot please open backport issues. It technically fails the regression policy (been broken too long -- probably since 1.9) but it is a silent bad code problem which is very bad, and the fix is very low risk. |
Backport issue(s) opened: #32711 (for 1.11), #32712 (for 1.12). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
Change https://golang.org/cl/183241 mentions this issue: |
Change https://golang.org/cl/183242 mentions this issue: |
Thank you for the fix @dr2chase! So since this bug goes ways back to Go1.9, is there a way for us to backport the fix to those versions of Go too? Kindly paging @golang/proposal-review |
I can answer that one. :) We won’t backport past the last two releases, regardless of how long the bug has existed. |
Gotcha, thank you for the answer @josharian! |
This appears to have fixed a bug I've been chasing for a couple months (as I attempted to learn more about SSA to fix). Would this crasher be a helpful addition? // errorcheck
package p
func hashBytesRaw(b0, b1, b2, b3, b7 byte) uint64 {
return (uint64(b0) | uint64(b1)<<8 | uint64(b2)<<16 | uint64(b3)<<24 )
}
func doStuff(data []byte) uint64 {
return hashBytesRaw(data[0], data[1], data[2], data[3], data[7]) // ERROR ".*runtime error: index out of range.*"
} |
@shanemhansen: Thanks, I'll add that as a test. |
Change https://golang.org/cl/185137 mentions this issue: |
Update #32680 Change-Id: I0318c22c22c5cd6ab6441d1aa2d1a40d20d71242 Reviewed-on: https://go-review.googlesource.com/c/go/+/185137 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Reviewed-by: David Chase <drchase@google.com>
…int in rewrite rules A missing operand to mergePoint caused lower to place values in the wrong blocks. Includes test, belt+suspenders to do both ssa check and verify the output (was is how the bug was originally observed). The fixed bug here is very likely present in Go versions 1.9-1.12 on amd64 and s390x Updates #32680. Fixes #32711. Change-Id: I63e702c4c40602cb795ef71b1691eb704d38ccc7 Reviewed-on: https://go-review.googlesource.com/c/go/+/183059 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> (cherry picked from commit 769fda2) Reviewed-on: https://go-review.googlesource.com/c/go/+/183242
…int in rewrite rules A missing operand to mergePoint caused lower to place values in the wrong blocks. Includes test, belt+suspenders to do both ssa check and verify the output (was is how the bug was originally observed). The fixed bug here is very likely present in Go versions 1.9-1.12 on amd64 and s390x Updates #32680. Fixes #32712. Change-Id: I63e702c4c40602cb795ef71b1691eb704d38ccc7 Reviewed-on: https://go-review.googlesource.com/c/go/+/183059 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> (cherry picked from commit 769fda2) Reviewed-on: https://go-review.googlesource.com/c/go/+/183241
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
What did you expect to see?
What did you see instead?
The text was updated successfully, but these errors were encountered: