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: changing a hot concrete method to interface method triggers a PGO ICE #67016

Closed
prattmic opened this issue Apr 24, 2024 · 6 comments
Closed

cmd/compile: changing a hot concrete method to interface method triggers a PGO ICE #67016

prattmic opened this issue Apr 24, 2024 · 6 comments
Assignees
@prattmic
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

@prattmic
Copy link
Member

Go version

go version go1.22.2 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/usr/local/google/home/mpratt/.cache/go-build'
GOENV='/usr/local/google/home/mpratt/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/usr/local/google/home/mpratt/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/usr/local/google/home/mpratt/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/google/home/mpratt/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.linux-amd64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='go1.22.2'
GOTOOLDIR='/usr/local/google/home/mpratt/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.2.linux-amd64/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/usr/local/google/home/mpratt/Downloads/b336740241_pgo_ice/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2161017047=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Run this program to generate /tmp/cpu.pprof:

package main

import (
        "os"
        "runtime/pprof"
)

type Type struct{}

var sink int

//go:noinline
func (Type) Method() {
        for range 10000000 {
                sink++
        }
}

func main() {
        f, _ := os.Create("/tmp/cpu.pprof")
        defer f.Close()
        pprof.StartCPUProfile(f)
        defer pprof.StopCPUProfile()

        var t Type
        t.Method()
}

Now change Type to an interface type. For example:

package main

import (
        "os"
        "runtime/pprof"
)

type Type interface{
        Method()
}

type Type2 struct{}

var sink int

//go:noinline
func (Type2) Method() {
        for i := 0; i < 10000000; i++ {
                sink++
        }
}

func main() {
        f, _ := os.Create("/tmp/cpu.pprof")
        defer f.Close()
        pprof.StartCPUProfile(f)
        defer pprof.StopCPUProfile()

        var t Type2
        t.Method()
}

Then build with PGO:

$ go build -pgo=/tmp/cpu.pprof

What did you see happen?

$ go build -pgo /tmp/cpu.pprof
# example.com/ice                                                     
./main.go:23:6: internal compiler error: panic: interface conversion: types.Object is nil, not *ir.Name
                                                                      
Please file a bug report including a short program that triggers the error.
https://go.dev/issue/new

What did you expect to see?

No ICE
@prattmic prattmic added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 24, 2024
@prattmic prattmic added this to the Go1.23 milestone Apr 24, 2024
@prattmic prattmic self-assigned this Apr 24, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 24, 2024
@prattmic
Copy link
Member Author

@gopherbot Please backport to 1.22. These compiler failures can happen across otherwise benign refactors, where the profile was collected prior to the refactor.

@prattmic
Copy link
Member Author

cc @cherrymui

@gopherbot
Copy link

Backport issue(s) opened: #67017 (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 gopherbot mentioned this issue Apr 24, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/581437 mentions this issue: cmd/compile: add debug log prior to export data lookup

@gopherbot
Copy link

Change https://go.dev/cl/581436 mentions this issue: cmd/compile: bail PGO method lookup on interface types

gopherbot pushed a commit that referenced this issue Apr 24, 2024
@prattmic
cmd/compile: add debug log prior to export data lookup
d037bba 
If there is a crash in LookupFunc (which has occurred a few times now),
this ensures that we log the offending symbol before crashing.

For #67016.

Change-Id: I0119597de2be3d1b97c41a9361273d1feb90ec11
Reviewed-on: https://go-review.googlesource.com/c/go/+/581437
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/581438 mentions this issue: [release-branch.go1.22] cmd/compile: bail PGO method lookup on interface types

gopherbot pushed a commit that referenced this issue Apr 26, 2024
@prattmic @joedian
[release-branch.go1.22] cmd/compile: bail PGO method lookup on interf…
a7ff78d 
…ace types

Interface types don't have concrete method implementations, so it does
not make sense to attempt a lookup.

An interface method would not normally appear in a PGO profile as it has
no symbol in the final binary. However it can appear if the method was
concrete when the profile was collected and it has since been refactored
to an interface method in the code being compiled.

The guards here (OTYPE, !Alias, !IsInterface) now match
noder.linker.relocObj, which does a similar iteration of all methods.

For #67016.
Fixes #67017.

Change-Id: I858c58929c890ac0b2019fbd7c99f683ab63f8bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/581436
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit 508e761)
Reviewed-on: https://go-review.googlesource.com/c/go/+/581438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

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
2 participants
@prattmic @gopherbot