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: internal compiler error: Value live at entry. It shouldn't be. #44355

Closed
tonyghita opened this issue Feb 18, 2021 · 14 comments
Closed

cmd/compile: internal compiler error: Value live at entry. It shouldn't be. #44355

tonyghita opened this issue Feb 18, 2021 · 14 comments
Assignees
Milestone

Comments

@tonyghita
Copy link

@tonyghita tonyghita commented Feb 18, 2021

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

$ go version
go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

Yes. I've git bisect the error to commit f2c0c2b

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tonyghita/Library/Caches/go-build"
GOENV="/Users/tonyghita/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/tonyghita/go/pkg/mod"
GONOPROXY="*"
GONOSUMDB="*"
GOOS="darwin"
GOPATH="/Users/tonyghita/go"
GOPRIVATE="*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/tonyghita/sdk/go1.16"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/tonyghita/sdk/go1.16/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/tonyghita/go/src/github.com/tonyghita/repro/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gv/vdmq3_l541b_0lq_ds37xslnzgdpkw/T/go-build3906706978=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I made a small reproducer repository to demonstrate the issue https://github.com/tonyghita/repro and run go test against it.

What did you expect to see?

Test pass without issue, as in Go 1.15.

What did you see instead?

# github.com/tonyghita/repro_test [github.com/tonyghita/repro.test]
./repro_test.go:10:13: internal compiler error: 'TestF.func1': Value live at entry. It shouldn't be. func TestF.func1, node ~R0, value v19
@ianlancetaylor ianlancetaylor changed the title internal compiler error: Value live at entry. It shouldn't be. cmd/compile: internal compiler error: Value live at entry. It shouldn't be. Feb 18, 2021
@ianlancetaylor ianlancetaylor added this to the Go1.17 milestone Feb 18, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 18, 2021

CC @mdempsky @dr2chase

If this is a real bug it probably needs a backport to the 1.16 branch.

@mdempsky mdempsky self-assigned this Feb 18, 2021
@tonyghita
Copy link
Author

@tonyghita tonyghita commented Feb 18, 2021

I forgot to describe the setup to reproduce the issue.

package repro

type A struct{}

func (*A) F() (_ *B) { return }

type B struct{}

and then in the repro_test package (or some other package)

package repro_test

import (
	"testing"

	"github.com/tonyghita/repro"
)

func TestF(t *testing.T) {
	var a *repro.A

	if a.F() != nil {
		t.Fail()
	}
}
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2021

Thanks for the minimized test case, @tonyghita . I'm able to reproduce the issue locally.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2021

The issue is due to inlining an imported function with a blank result parameter and only one return statement, which also doesn't have any result arguments.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2021

@tonyghita Can I ask what the original function looks like and why it was written that way? It's definitely a compiler issue that it's mishandling this case that I expect to have a fix for shortly, but I'm surprised there's real world code that exhibits this behavior, so I'm curious.

FWIW, if you're in need of a workaround until Go 1.16.1 is available, you can should be able to either change the bare return statement to explicitly return zero values, or just add a second (unreachable) return statement. Either of these would suppress the optimization that's crashing here.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 18, 2021

Aside: From all my years working on the compiler, there is no internal compiler error I hate more. (Of course, that means I also love it in a way, because it is catching bugs.)

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 18, 2021

Change https://golang.org/cl/293293 mentions this issue: cmd/compile: declare inlined result params early for empty returns

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2021

@gopherbot please backport to Go 1.16

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 18, 2021

Backport issue(s) opened: #44358 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@tonyghita
Copy link
Author

@tonyghita tonyghita commented Feb 18, 2021

@mdempsky the original function is not very different from the reproducer code. It's pretty silly/useless and can/will be removed.

To give a bit more context, the codebase is an API where the fields of the API map to method sets via reflect package. For example, given the API schema:

type Query {
  user(id: ID): User
}

type User {
  name: String
  nickname: String
}

we would have equivalent structs type Query struct and type User struct in Go.

type Query struct {
  // hidden fields
}

func (q *Query) User(ctx context.Context, args struct{ ID string }) *User {
  return q.load.UserByID(ctx, args.ID) // pretend this function loads a user by their ID.
}

type User struct {
  // hidden fields
}

func (u *User) Name() *string {
  return u.name
}

// Deprecated: use Name instead.
func (*User) Nickname() (_ *string) { return }

Sometimes, we want to remove the implementation of a field without breaking the API, so the method is reduced to the bare minimum implementation until it is safe to remove the field from the API without breaking clients..

Sometimes we really flub the design of an API and so we remove the implementation for many fields that return many different types, and it is convenient to avoid having to specify the zero value for each type (although maybe too clever).

func (*User) Nickname() (_ *string)    { return }
func (*User) CreatedAt() (_ int)       { return }
func (*User) BestFriend() (_ *User)    { return }
func (*User) LicensePlate() (_ string) { return }

The implementation is removed but the schema/interface can still be satisfied when we reflect type User's method set.

This bug was triggered by one of these "gutted" methods getting called in a forgotten unit test. As a workaround, we might be able to remove the useless test...
although maybe the issue remains since reflect could still call one of these functions.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2021

@tonyghita Thanks for the context. That seems reasonable. Glad to know it's nothing performance sensitive, so just turning off the new, failing optimization in that case should be fine.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Feb 19, 2021

There is another possibly related case that appeared at #44415

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 19, 2021

Is https://go-review.googlesource.com/c/go/+/293293/ okay to submit now? I'm thinking since it's a 1.16 regression that needs to be backported, it makes sense to commit now and get the backport fix CL ready.

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 25, 2021

Please go ahead and submit.

@gopherbot gopherbot closed this in 6c3bcda Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants