golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.84k stars 17.65k forks source link

cmd/compile: panic attempting to spill int128 #19515

Closed Omustardo closed 7 years ago

Omustardo commented 7 years ago

Edit: This first post is mostly irrelevant. See second post and further for more info.

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

go version go1.8 windows/amd64

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

GOHOSTARCH=amd64 GOHOSTOS=windows

What did you do?

Tried to go get a specific package: go get github.com/shibukawa/gui4go

What did you expect to see?

I expected the package to be downloaded and stored in the standard $GOPATH

What did you see instead?

A popup from OpenSSH asking me to confirm that I wanted to connect to github. http://i.imgur.com/xs3N6zM.png I declined out of instinct and it resulted in a panic:

$ go get github.com/shibukawa/gui4go
# github.com/shibukawa/gui4go
panic: bad store type

goroutine 1 [running]:
cmd/compile/internal/amd64.storeByType(0xd597a0, 0xd738e0, 0xa6a920)
        c:/go/src/cmd/compile/internal/amd64/ssa.go:76 +0xcf
cmd/compile/internal/amd64.ssaGenValue(0xc0441e3f80, 0xc04349cf20)
        c:/go/src/cmd/compile/internal/amd64/ssa.go:699 +0x5aeb
cmd/compile/internal/gc.genssa(0xc0441c7b00, 0xc0420d4238, 0xc0442253b0, 0xc044225420)
        c:/go/src/cmd/compile/internal/gc/ssa.go:4422 +0x265
cmd/compile/internal/gc.compile(0xc042e4d950)
        c:/go/src/cmd/compile/internal/gc/pgen.go:443 +0x70e
cmd/compile/internal/gc.funccompile(0xc042e4d950)
        c:/go/src/cmd/compile/internal/gc/dcl.go:1292 +0xe3
cmd/compile/internal/gc.Main()
        c:/go/src/cmd/compile/internal/gc/main.go:464 +0x1f0f
main.main()
        c:/go/src/cmd/compile/main.go:50 +0x105

I then cleared the files it had downloaded to $GOPATH and started again.

This time I accepted connecting to github and it downloaded the files again.

$ go get -v github.com/shibukawa/gui4go
github.com/shibukawa/gui4go (download)
github.com/shibukawa/glfw (download)
github.com/shibukawa/glfw-2 (download)
github.com/shibukawa/nanovgo (download)
# cd C:\workspace\Go\src\github.com\shibukawa\nanovgo; git submodule update --init --recursive
Submodule 'gh-pages' (git@github.com:shibukawa/nanovgo.git) registered for path 'gh-pages'
Cloning into 'gh-pages'...
Warning: Permanently added 'github.com,192.30.253.112' (RSA) to the list of known hosts.
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
fatal: clone of 'git@github.com:shibukawa/nanovgo.git' into submodule path 'gh-pages' failed
package github.com/shibukawa/nanovgo: exit status 128

There seems to be an issue with permissions in the repo, which is not something on Go's end. I'll follow up with the author to see how it was set up.

On the Go side of things, this is probably an unusual situation and a minor issue, but the error message for go get being "panic: bad store type" isn't very descriptive as an end user. Perhaps something could be logged about permissions or the inability to connect to the repository? I'm also not sure why extra permissions needed to be given in the first place. I've had no trouble with any other go github repository I've tried to go get from.

Omustardo commented 7 years ago

An update - the panic isn't specific to go get. I downloaded the code manually and changed a couple imports to get the demo code to be valid (references to nanogui were changed to gui4go). When trying to build I get the same panic:

$ go build github.com/shibukawa/gui4go/sample1/sample.go
# github.com/shibukawa/gui4go
panic: bad store type

goroutine 1 [running]:
cmd/compile/internal/amd64.storeByType(0xd597a0, 0xd738e0, 0xa6a920)
        c:/go/src/cmd/compile/internal/amd64/ssa.go:76 +0xcf
cmd/compile/internal/amd64.ssaGenValue(0xc044226d00, 0xc043474f20)
        c:/go/src/cmd/compile/internal/amd64/ssa.go:699 +0x5aeb
cmd/compile/internal/gc.genssa(0xc0441c3d40, 0xc0420d4238, 0xc0442315e0, 0xc044231650)
        c:/go/src/cmd/compile/internal/gc/ssa.go:4422 +0x265
cmd/compile/internal/gc.compile(0xc042de9320)
        c:/go/src/cmd/compile/internal/gc/pgen.go:443 +0x70e
cmd/compile/internal/gc.funccompile(0xc042de9320)
        c:/go/src/cmd/compile/internal/gc/dcl.go:1292 +0xe3
cmd/compile/internal/gc.Main()
        c:/go/src/cmd/compile/internal/gc/main.go:464 +0x1f0f
main.main()
        c:/go/src/cmd/compile/main.go:50 +0x105

The first post in this thread is likely irrelevant since the issue isn't with go get, but I'll leave it as is rather than confusing things with edits.

josharian commented 7 years ago

Reproduces with tip. Also reproduces with 1.7, so not a regression, so not marking for 1.8.1.

Looks like this happens when the register allocator decides to spill an int128 value, which weren't designed to be spilled. Preventing CSE of int128s appears to fix the problem (near the top of cse.go):

            if v.Type.IsMemory() || v.Type == TypeInt128 {
                continue // memory values and int128s can never cse
            }

Haven't made a test case. Won't look at this again for a day or three, so jotting down what I know to avoid duplicate effort. (If anyone else wants to take this over, feel free.)

cc @randall77

josharian commented 7 years ago

Come to think of it, movoconst should never be spilled, since it is (or ought to be) rematerializable. This might be a regalloc problem. I'm AFK, but cc @cherrymui.

randall77 commented 7 years ago

We do have some MOVOload instructions. My hunch is that those are to blame. We might need to back out copying using MOVOload. Or teach the amd64 backend how to spill them.

josharian commented 7 years ago

In this particular case, it is a MOVOconst, so something is not right in regalloc. (Note that both MOVOload and MOVOstore have memory type, and thus will never be CSE'd.) We should indeed probably also teach amd64 how to spill them, which will also require teaching it how to allocate stack space for them correctly.

Getting a minimized test case for this is a major pain.

josharian commented 7 years ago

Marking as HelpWanted for phase one, namely extracting a moderate-sized, standalone reproducer.

dominikh commented 7 years ago

https://play.golang.org/p/YuA5LJzM5c is the closest I got to a minimal test case.

dominikh commented 7 years ago

A bit more minimal, even: https://play.golang.org/p/7J9qNNXlUY

josharian commented 7 years ago

Thanks, @dominikh, that's fabulous.

gopherbot commented 7 years ago

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