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: Go 1.22 build fails with 1.21 PGO profile on internal/saferio change #65615

Closed
konradreiche opened this issue Feb 8, 2024 · 6 comments
Closed

cmd/compile: Go 1.22 build fails with 1.21 PGO profile on internal/saferio change #65615

konradreiche opened this issue Feb 8, 2024 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@konradreiche
Copy link

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

GOARCH='amd64'
GOOS='linux'
GOVERSION='go1.22.0'

What did you do?

We are continuously collecting PGO profiles and apply them to new builds, a process that worked seamlessly with Go 1.21. I upgraded to Go 1.22 and attempted to compile our build using the Go 1.21 PGO profile.

go build -pgo=auto .

What did you see happen?

The go build command failed, producing the following error:

go build -pgo=auto .
# encoding/gob
/usr/lib/go/src/bufio/bufio.go:47:6: internal compiler error: saferio.SliceCap has 0+1 params, but instantiated with 0+0 args

This error suggests a change made to the SliceCap method in the internal/saferio package between Go 1.21 and Go 1.22:

https://github.com/golang/go/blob/release-branch.go1.21/src/internal/saferio/io.go#L116
https://github.com/golang/go/blob/release-branch.go1.22/src/internal/saferio/io.go#L128

The build compiles successfully with PGO turned off:

go build -pgo=off .

What did you expect to see?

Expected the go build command to compile the build successfully, as it did with Go 1.21.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 8, 2024
@konradreiche
Copy link
Author

I assume this isn't expected because according to the PGO documentation a change to the source code could result in performance degradation but not necessarily compilation failure.

@prattmic
Copy link
Member

prattmic commented Feb 8, 2024

Thanks for filing this! We discovered this same crash with a different symbol independently this morning and I was coming to file an issue only to find you've done so for me. :D

This is indeed a bug in the way we lookup functions for PGO. Internally, the compiler internally is trying to look up the function as non-generic (because the profile contains the non-generic symbol name), but the function is actually generic and the compiler chokes. We should fix this.

@prattmic prattmic added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 8, 2024
@prattmic prattmic added this to the Go1.23 milestone Feb 8, 2024
@prattmic
Copy link
Member

prattmic commented Feb 8, 2024 •
edited by dmitshur

@gopherbot Please backport to 1.21 and 1.22. This is a build failure with PGO if a hot function is changed from non-generic to generic. Only workarounds are to rename the symbol or disable PGO.

This was referenced Feb 8, 2024
Closed
Closed
@gopherbot
Copy link

gopherbot commented Feb 8, 2024 •
edited by dmitshur

Backport issue(s) opened: #65617 (for 1.21), #65618 (for 1.22).

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

@gopherbot
Copy link

Change https://go.dev/cl/562737 mentions this issue: cmd/compile: fail noder.LookupFunc gracefully if function generic

@gopherbot
Copy link

Change https://go.dev/cl/563016 mentions this issue: [release-branch.go1.22] cmd/compile: fail noder.LookupFunc gracefully if function generic

gopherbot pushed a commit that referenced this issue Feb 16, 2024
@prattmic @thanm
[release-branch.go1.22] cmd/compile: fail noder.LookupFunc gracefully…
6cbe522 
… if function generic

PGO uses noder.LookupFunc to look for devirtualization targets in
export data.  LookupFunc does not support type-parameterized
functions, and will currently fail the build when attempting to lookup
a type-parameterized function because objIdx is passed the wrong
number of type arguments.

This doesn't usually come up, as a PGO profile will report a generic
function with a symbol name like Func[.go.shape.foo]. In export data,
this is just Func, so when we do LookupFunc("Func[.go.shape.foo]")
lookup simply fails because the name doesn't exist.

However, if Func is not generic when the profile is collected, but the
source has since changed to make Func generic, then LookupFunc("Func")
will find the object successfully, only to fail the build because we
failed to provide type arguments.

Handle this with a objIdxMayFail, which allows graceful failure if the
object requires type arguments.

Bumping the language version to 1.21 in pgo_devirtualize_test.go is
required for type inference of the uses of mult.MultFn in
cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go.

For #65615.
Fixes #65618.

Change-Id: I84d9344840b851182f5321b8f7a29a591221b29f
Reviewed-on: https://go-review.googlesource.com/c/go/+/562737
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 532c6f1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/563016
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@prattmic @ezz-no
cmd/compile: fail noder.LookupFunc gracefully if function generic
00a1b89 
PGO uses noder.LookupFunc to look for devirtualization targets in
export data.  LookupFunc does not support type-parameterized
functions, and will currently fail the build when attempting to lookup
a type-parameterized function because objIdx is passed the wrong
number of type arguments.

This doesn't usually come up, as a PGO profile will report a generic
function with a symbol name like Func[.go.shape.foo]. In export data,
this is just Func, so when we do LookupFunc("Func[.go.shape.foo]")
lookup simply fails because the name doesn't exist.

However, if Func is not generic when the profile is collected, but the
source has since changed to make Func generic, then LookupFunc("Func")
will find the object successfully, only to fail the build because we
failed to provide type arguments.

Handle this with a objIdxMayFail, which allows graceful failure if the
object requires type arguments.

Bumping the language version to 1.21 in pgo_devirtualize_test.go is
required for type inference of the uses of mult.MultFn in
cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go.

Fixes golang#65615.

Change-Id: I84d9344840b851182f5321b8f7a29a591221b29f
Reviewed-on: https://go-review.googlesource.com/c/go/+/562737
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Mar 5, 2024
@prattmic @bradfitz
[release-branch.go1.22] cmd/compile: fail noder.LookupFunc gracefully…
05bdd83 
… if function generic

PGO uses noder.LookupFunc to look for devirtualization targets in
export data.  LookupFunc does not support type-parameterized
functions, and will currently fail the build when attempting to lookup
a type-parameterized function because objIdx is passed the wrong
number of type arguments.

This doesn't usually come up, as a PGO profile will report a generic
function with a symbol name like Func[.go.shape.foo]. In export data,
this is just Func, so when we do LookupFunc("Func[.go.shape.foo]")
lookup simply fails because the name doesn't exist.

However, if Func is not generic when the profile is collected, but the
source has since changed to make Func generic, then LookupFunc("Func")
will find the object successfully, only to fail the build because we
failed to provide type arguments.

Handle this with a objIdxMayFail, which allows graceful failure if the
object requires type arguments.

Bumping the language version to 1.21 in pgo_devirtualize_test.go is
required for type inference of the uses of mult.MultFn in
cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go.

For golang#65615.
Fixes golang#65618.

Change-Id: I84d9344840b851182f5321b8f7a29a591221b29f
Reviewed-on: https://go-review.googlesource.com/c/go/+/562737
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 532c6f1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/563016
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 6, 2024
@prattmic @romaindoumenc
[release-branch.go1.22] cmd/compile: fail noder.LookupFunc gracefully…
a781b9c 
… if function generic

PGO uses noder.LookupFunc to look for devirtualization targets in
export data.  LookupFunc does not support type-parameterized
functions, and will currently fail the build when attempting to lookup
a type-parameterized function because objIdx is passed the wrong
number of type arguments.

This doesn't usually come up, as a PGO profile will report a generic
function with a symbol name like Func[.go.shape.foo]. In export data,
this is just Func, so when we do LookupFunc("Func[.go.shape.foo]")
lookup simply fails because the name doesn't exist.

However, if Func is not generic when the profile is collected, but the
source has since changed to make Func generic, then LookupFunc("Func")
will find the object successfully, only to fail the build because we
failed to provide type arguments.

Handle this with a objIdxMayFail, which allows graceful failure if the
object requires type arguments.

Bumping the language version to 1.21 in pgo_devirtualize_test.go is
required for type inference of the uses of mult.MultFn in
cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go.

For golang#65615.
Fixes golang#65618.

Change-Id: I84d9344840b851182f5321b8f7a29a591221b29f
Reviewed-on: https://go-review.googlesource.com/c/go/+/562737
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 532c6f1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/563016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@konradreiche @prattmic @gopherbot