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: corrupted value #20530

Closed
cznic opened this issue May 30, 2017 · 16 comments

Comments

Projects
None yet
9 participants
@cznic
Copy link
Contributor

commented May 30, 2017

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.

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

@mundaym

This comment has been minimized.

Copy link
Member

commented May 30, 2017

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.)

@dr2chase dr2chase self-assigned this May 30, 2017

@gopherbot

This comment has been minimized.

Copy link

commented May 30, 2017

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

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

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.

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

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

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

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

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.

@gopherbot gopherbot closed this in 9613a63 May 30, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

Reopening for 1.8.4.

@josharian josharian reopened this May 30, 2017

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

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

This comment has been minimized.

Copy link

commented May 31, 2017

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

gopherbot pushed a commit that referenced this issue May 31, 2017

cmd/compile: fix subword store/load elision for 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>
@dr2chase

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

What's the 1.8.4 process after reopening?

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2017

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

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2017

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

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

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

This comment has been minimized.

Copy link

commented Oct 14, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Oct 14, 2017

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

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.8] cmd/compile: fix subword store/load elision fo…
…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>

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.8] cmd/compile: fix subword store/load elision fo…
…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

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

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

@rsc rsc closed this Oct 26, 2017

@golang golang locked and limited conversation to collaborators Oct 26, 2018

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.