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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: miscompilation (missing BSWAPL) in go1.8 #19201

Closed
flyingmutant opened this issue Feb 20, 2017 · 7 comments
Closed

cmd/compile: miscompilation (missing BSWAPL) in go1.8 #19201

flyingmutant opened this issue Feb 20, 2017 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.8.1

Comments

@flyingmutant
Copy link
Contributor

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

go version go1.8 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/user/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build043674809=/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?

https://play.golang.org/p/O9a63D2aU4:

package main

import (
        "encoding/binary"
        "fmt"
)

var (
        ch1 = make(chan int)
        ch2 = make(chan int)

        bin  = []byte("a\000\000\001")
        want = binary.BigEndian.Uint32(bin)

        c consumer = noopConsumer{}
)

type msg struct {
        code uint32
}

type consumer interface {
        consume(msg)
}

type noopConsumer struct{}

func (noopConsumer) consume(msg) {}

func init() {
        close(ch1)
}

func main() {
        var m msg
        m.code = binary.BigEndian.Uint32(bin)

        select {
        case <-ch1:
                c.consume(m)
                if m.code != want {
                        // can not use m.code here, or it will work
                        fmt.Printf("wrong msg %#v, want code 0x%x\n", m, want)
                } 
        case <-ch2:
        }
}

What did you expect to see?

Nothing.

What did you see instead?

wrong msg main.msg{code:0x1000061}, want code 0x61000001
@ALTree
Copy link
Member

ALTree commented Feb 20, 2017

Go1.8 regression, it's not in go1.7.5.

Git bisect says: introduced in 10f7574.

@ALTree ALTree changed the title Miscompilation (missing BSWAPL) in go1.8 cmd/compile: miscompilation (missing BSWAPL) in go1.8 Feb 20, 2017
@ALTree
Copy link
Member

ALTree commented Feb 20, 2017

cc @TocarIP @randall77

@ianlancetaylor ianlancetaylor added this to the Go1.8.1 milestone Feb 20, 2017
@TocarIP
Copy link
Contributor

TocarIP commented Feb 21, 2017

Stepping through instructions in gdb shows that bswap + store are executed twice. bswap(bswap(x)) == x, so this case fails. First execution of bswap is on normal path and the second is after runtime.selectgo sets pc to the basic block containing bswap. I suspect fuse pass, which puts shits/ors into the same block as newselect/selectrecv

@gopherbot
Copy link

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

@randall77 randall77 mentioned this issue Mar 8, 2017
Open
@aclements
Copy link
Member

aclements commented Mar 21, 2017 •
edited

Need to decide whether to move CL 37376 to the release branch or cherry pick CL 37661.

@aclements
Copy link
Member

Created https://go-review.googlesource.com/c/38587 for the change to the release branch. @TocarIP, would you mind paring down the one for master (https://go-review.googlesource.com/c/37376) to just the test?

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 27, 2017
@TocarIP @aclements
[release-branch.go1.8] cmd/compile/internal/ssa: don't schedule value…
c955eb1 
…s after select

Scheduling values after calls to selectrecv,
will cause them to be executed multiple times, due to runtime.selectgo
jumping to the next instruction in the selectrecv basic block.
Prevent this by scheduling calls to selectrecv as late as possible

Fixes #19201

Change-Id: I6415792e2c465dc6d9bd6583ba1e54b107bcf5cc
Reviewed-on: https://go-review.googlesource.com/38587
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@TocarIP @lparth
test/fixedbugs: add a test for 19201
a78d414 
This was cherry-picked to 1.8 as CL 38587, but on master issue was fixed
by CL 37661. Add still relevant part (test) and close issue, since test passes.

Fixes golang#19201

Change-Id: I6415792e2c465dc6d9bd6583ba1e54b107bcf5cc
Reviewed-on: https://go-review.googlesource.com/37376
Run-TryBot: Ilya Tocar <ilya.tocar@intel.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Mar 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8.1
Development

No branches or pull requests
6 participants
@flyingmutant @aclements @ianlancetaylor @ALTree @gopherbot @TocarIP