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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@tonyghita
Copy link

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 18, 2021
@ianlancetaylor ianlancetaylor added this to the Go1.17 milestone Feb 18, 2021
@ianlancetaylor
Copy link
Contributor

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

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

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

@mdempsky
Copy link
Member

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

@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

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

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

@mdempsky
Copy link
Member

@gopherbot please backport to Go 1.16

@gopherbot
Copy link

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

There is another possibly related case that appeared at #44415

@mdempsky
Copy link
Member

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

Please go ahead and submit.

@golang golang locked and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants