cmd/compile: corrupted value #20530

cznic · 2017-05-30T15:51:03Z

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

go version go1.8.3 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jnml"
GORACE=""
GOROOT="/home/jnml/go"
GOTOOLDIR="/home/jnml/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build797922225=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

$ cat bug.go 
package main

var a uint8

func main() {
	b := int8(func() int32 { return -1 }())
	a = uint8(b)
	//println(a)
	if int32(a) != 255 {
		println("got", a, "expected 255")
	}
}
$ go run bug.go
got 255 expected 255
$

Playground

What did you expect to see?

Nothing

What did you see instead?

got 255 expected 255

Additional info

Uncommenting line 8 (//println(a)) "fixes" the problem.

The text was updated successfully, but these errors were encountered:

mundaym · 2017-05-30T18:48:27Z

I think the problem is these rules:

// replace load from same location as preceding store with copy
(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
(MOVWload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
(MOVLload [off] {sym} ptr (MOVLstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
(MOVQload [off] {sym} ptr (MOVQstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x

The MOVBQZX is merged into the MOVBload, which is then replaced with a copy.

(s390x had the same problem which was fixed in 094992e (CL 37154). That CL hasn't been backported to 1.8.x.)

gopherbot · 2017-05-30T20:15:45Z

CL https://golang.org/cl/44355 mentions this issue.

randall77 · 2017-05-30T20:16:58Z

This bug exists in 1.7 and 1.8, but not 1.6.
Technically not a 1.8 regression because 1.7 also has the bug. But nevertheless I'm going to mark for a possible 1.8.4.

dr2chase · 2017-05-30T20:19:21Z

And also x86:

// replace load from same location as preceding store with copy
(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
(MOVWload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
(MOVLload [off] {sym} ptr (MOVLstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x

and ARM:

(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type) -> x
(MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type) -> x
(MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type) -> x
(MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type) -> x

So I'd predict some likely failures in the trybots with my shiny new test. :-)

dr2chase · 2017-05-30T20:47:04Z

PPC64 lacks the pattern and the bug also does not reproduce there.
It looks like it ought to fail on ARM but I might need to tweak the test just a bit.

josharian · 2017-05-30T22:09:23Z

Reopening for 1.8.4.

cherrymui · 2017-05-31T12:26:12Z

A slight variation of the test case can trigger the failure on ARM. The CL fixed it.

package main

var a uint8

func main() {
        b := int8(func() uint32 { return 0xffffffff }())
        a = uint8(b)
        if int32(a) != 255 {
                // Failing case prints 'got 255 expected 255'
                println("got", a, "expected 255")
        }
}

It also fails on MIPS. CL coming.

gopherbot · 2017-05-31T12:36:20Z

CL https://golang.org/cl/44430 mentions this issue.

Apply the fix in CL 44355 to MIPS. ARM64 has these rules but commented out for performance reason. Fix the commented rules, in case they are enabled in the future. Enhance the test so it triggers the failure on ARM and MIPS without the fix. Updates #20530. Change-Id: I82d77448e3939a545fe519d0a29a164f8fa5417c Reviewed-on: https://go-review.googlesource.com/44430 Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>

dr2chase · 2017-05-31T16:05:38Z

What's the 1.8.4 process after reopening?

randall77 · 2017-05-31T16:19:27Z

Just leave it for now. When we get to doing a 1.8.4 release, all the CLs attached to 1.8.4 marked bugs will be backported to the release branch.

cznic · 2017-09-17T11:52:29Z

I believe this is fixed in 1.9. Someone please cross-check and close.

cherrymui · 2017-09-19T21:14:23Z

This is kept open for Go 1.8.4, if there is one.

rsc · 2017-10-13T20:09:39Z

CL 37154 OK for Go 1.8.5 (after CL 41395).
~~CL 44355 OK for Go 1.8.5.~~
CL 70841 OK for Go 1.8.5. (Fixed problems in CL 44355.)
CL 44430 OK for Go 1.8.5.

gopherbot · 2017-10-14T17:58:17Z

Change https://golang.org/cl/70840 mentions this issue: cmd/compile: fix subword store/load elision for MIPS

gopherbot · 2017-10-14T18:15:04Z

Change https://golang.org/cl/70841 mentions this issue: [release-branch.go1.8] cmd/compile: fix subword store/load elision for amd64, x86, arm

…r amd64, x86, arm Replacing byteload-of-bytestore-of-x with x is incorrect when x contains a larger-than-byte value (and so on for 16 and 32-bit load/store pairs). Replace "x" with the appropriate zero/sign extension of x, which if unnecessary will be repaired by other rules. Made logic for arm match x86 and amd64; yields minor extra optimization, plus I am (much) more confident it's correct, despite inability to reproduce bug on arm. Ppc64 lacks this optimization, hence lacks this problem. See related https://golang.org/cl/37154/ Fixes #20530. [Merge conflicts in generated rewrite files resolved by regenerating from scratch, using the programs in ssa/gen.] Change-Id: I6af9cac2ad43bee99cafdcb04725ce7e55a43323 Reviewed-on: https://go-review.googlesource.com/44355 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-on: https://go-review.googlesource.com/70841 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

…r MIPS Apply the fix in CL 44355 to MIPS. ARM64 has these rules but commented out for performance reason. Fix the commented rules, in case they are enabled in the future. Enhance the test so it triggers the failure on ARM and MIPS without the fix. Updates #20530. Change-Id: I82d77448e3939a545fe519d0a29a164f8fa5417c Reviewed-on: https://go-review.googlesource.com/44430 Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> Reviewed-on: https://go-review.googlesource.com/70840 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

rsc · 2017-10-26T21:07:48Z

go1.8.5 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:07:48 UTC

ianlancetaylor added this to the Go1.9 milestone May 30, 2017

dr2chase self-assigned this May 30, 2017

randall77 modified the milestones: Go1.8.4, Go1.9 May 30, 2017

gopherbot closed this as completed in 9613a63 May 30, 2017

josharian reopened this May 30, 2017

rsc modified the milestones: Go1.8.4, Go1.8.5 Oct 4, 2017

rsc added release-blocker CherryPickApproved Used during the release process for point releases and removed release-blocker labels Oct 13, 2017

rsc mentioned this issue Oct 14, 2017

all: Go 1.8.5 release status #22264

Closed

rsc closed this as completed Oct 26, 2017

golang locked and limited conversation to collaborators Oct 26, 2018

gopherbot added the FrozenDueToAge label Oct 26, 2018

rsc unassigned dr2chase Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: corrupted value #20530

cmd/compile: corrupted value #20530

cznic commented May 30, 2017

mundaym commented May 30, 2017

gopherbot commented May 30, 2017

randall77 commented May 30, 2017

dr2chase commented May 30, 2017

dr2chase commented May 30, 2017

josharian commented May 30, 2017

cherrymui commented May 31, 2017

gopherbot commented May 31, 2017

dr2chase commented May 31, 2017

randall77 commented May 31, 2017

cznic commented Sep 17, 2017

cherrymui commented Sep 19, 2017

rsc commented Oct 13, 2017 •

edited

gopherbot commented Oct 14, 2017

gopherbot commented Oct 14, 2017

rsc commented Oct 26, 2017

cmd/compile: corrupted value #20530

cmd/compile: corrupted value #20530

Comments

cznic commented May 30, 2017

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

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

What did you do?

What did you expect to see?

What did you see instead?

Additional info

mundaym commented May 30, 2017

gopherbot commented May 30, 2017

randall77 commented May 30, 2017

dr2chase commented May 30, 2017

dr2chase commented May 30, 2017

josharian commented May 30, 2017

cherrymui commented May 31, 2017

gopherbot commented May 31, 2017

dr2chase commented May 31, 2017

randall77 commented May 31, 2017

cznic commented Sep 17, 2017

cherrymui commented Sep 19, 2017

rsc commented Oct 13, 2017 • edited

gopherbot commented Oct 14, 2017

gopherbot commented Oct 14, 2017

rsc commented Oct 26, 2017

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

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

rsc commented Oct 13, 2017 •

edited