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: panic attempting to spill int128 #19515

Closed
Omustardo opened this issue Mar 12, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@Omustardo
Copy link

commented Mar 12, 2017

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

This comment has been minimized.

Copy link
Author

commented Mar 12, 2017

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 josharian changed the title go get panic on specific repository cmd/compile: panic attempting to spill int128 Mar 12, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2017

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 josharian added this to the Go1.9 milestone Mar 12, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

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

@dominikh

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

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

@dominikh

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

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

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2017

Thanks, @dominikh, that's fabulous.

@josharian josharian removed the help wanted label Mar 14, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Mar 14, 2017

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

@gopherbot gopherbot closed this in 8a44c8e Mar 14, 2017

@htrob htrob referenced this issue Mar 17, 2017

Open

Building issues #3

@golang golang locked and limited conversation to collaborators Mar 14, 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.