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

Jump to bottom

cmd/compile/internal/gc: bad store type when compiling an openshift origin package #42568

Closed
derekparker opened this issue Nov 12, 2020 · 6 comments
Closed

cmd/compile/internal/gc: bad store type when compiling an openshift origin package #42568

derekparker opened this issue Nov 12, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.16

Comments

@derekparker
Copy link
Contributor

derekparker commented Nov 12, 2020 •
edited

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

$ go version
go version devel +15f01d6ae9 Thu Oct 29 03:23:51 2020 +0000 linux/amd64

This also reproduces on tip.

Does this issue reproduce with the latest release?

No, only on tip.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/deparker/.cache/go-build"
GOENV="/home/deparker/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/deparker/Code/gopath"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/deparker/Code/go/src/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 -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build790675458=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. git clone github.com/openshift/origin
  2. cd pkg/templateservicebroker/servicebroker
  3. GO111MODULE=auto go build

What did you expect to see?

Successful compilation.

What did you see instead?

# github.com/openshift/origin/pkg/templateservicebroker/servicebroker
panic: bad store type

goroutine 1 [running]:
cmd/compile/internal/amd64.storeByType(0xc00005dec0, 0xc0006f82d0)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:78 +0x129
cmd/compile/internal/amd64.loadByType(0xc00005dec0, 0xc001a80813)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:53 +0x38
cmd/compile/internal/amd64.ssaGenValue(0xc001cfaa80, 0xc001a88340)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:950 +0x6525
cmd/compile/internal/gc.genssa(0xc001c8c6e0, 0xc001cfaa00)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/ssa.go:6319 +0x6eb
cmd/compile/internal/gc.compileSSA(0xc0001522c0, 0x0)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:329 +0x3a5
cmd/compile/internal/gc.compile(0xc0001522c0)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:277 +0x39e
cmd/compile/internal/gc.funccompile(0xc0001522c0)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:220 +0xc5
cmd/compile/internal/gc.Main(0xcb9768)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/main.go:752 +0x34af
main.main()
        /home/deparker/Code/go/src/cmd/compile/main.go:52 +0xb1

Here is some prelim debugging output I've collected about what is causing this panic to trigger:

panic: bad store type

goroutine 15 [running]:
cmd/compile/internal/amd64.storeByType(0xc000051ec0, 0xc0005a62d0)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:78 +0x129
cmd/compile/internal/amd64.loadByType(0xc000051ec0, 0xc001b00813)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:53 +0x38
cmd/compile/internal/amd64.ssaGenValue(0xc0018c2d00, 0xc001b01b90)
        /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:950 +0x6525
cmd/compile/internal/gc.genssa(0xc001dfc000, 0xc0018c2c00)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/ssa.go:6319 +0x6eb
cmd/compile/internal/gc.compileSSA(0xc0000c82c0, 0x0)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:329 +0x3a5
cmd/compile/internal/gc.compileFunctions.func2(0xc001f464e0, 0xc001e27b30, 0x0)
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:384 +0x4d
created by cmd/compile/internal/gc.compileFunctions
        /home/deparker/Code/go/src/cmd/compile/internal/gc/pgen.go:382 +0x129
(dlv) c
> [unrecovered-panic] runtime.fatalpanic() /home/deparker/Code/go/src/runtime/panic.go:1185 (hits goroutine(15):1 total:1) (PC: 0x4375a0)
Warning: debugging optimized function
        runtime.curg._panic.arg: interface {}(string) "bad store type"
Current event: 21604
  1180: // fatalpanic implements an unrecoverable panic. It is like fatalthrow, except
  1181: // that if msgs != nil, fatalpanic also prints panic messages and decrements
  1182: // runningPanicDefers once main is blocked from exiting.
  1183: //
  1184: //go:nosplit
=>1185: func fatalpanic(msgs *_panic) {
  1186:         pc := getcallerpc()
  1187:         sp := getcallersp()
  1188:         gp := getg()
  1189:         var docrash bool
  1190:         // Switch to the system stack to avoid any stack growth, which
(dlv) b /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:78
Breakpoint 1 set at 0xbadc2d for cmd/compile/internal/amd64.storeByType() /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:78
(dlv) rev continue
> cmd/compile/internal/amd64.storeByType() /home/deparker/Code/go/src/cmd/compile/internal/amd64/ssa.go:78 (hits goroutine(15):1 total:1) (PC: 0xbadc2d)
Warning: debugging optimized function
Current event: 21604
    73:                         return x86.AMOVL
    74:                 case 8:
    75:                         return x86.AMOVQ
    76:                 }
    77:         }
=>  78:         panic("bad store type")
    79: }
    80:
    81: // moveByType returns the reg->reg move instruction of the given type.
    82: func moveByType(t *types.Type) obj.As {
    83:         if t.IsFloat() {
(dlv) args
t = ("*cmd/compile/internal/types.Type")(0xc000051ec0)
~r1 = (unreadable empty OP stack)
(dlv) p t
*cmd/compile/internal/types.Type {
        Extra: interface {}(*cmd/compile/internal/types.Interface) *{
                Fields: (*"cmd/compile/internal/types.Fields")(0xc00004c670),
                pkg: *cmd/compile/internal/types.Pkg nil,},
        Width: 16,
        methods: cmd/compile/internal/types.Fields {
                s: *[]*cmd/compile/internal/types.Field nil,},
        allMethods: cmd/compile/internal/types.Fields {
                s: *[]*cmd/compile/internal/types.Field nil,},
        Nod: *cmd/compile/internal/types.Node {_: 0},
        Orig: *cmd/compile/internal/types.Type {
                Extra: interface {}(*cmd/compile/internal/types.Interface) ...,
                Width: 16,
                methods: (*"cmd/compile/internal/types.Fields")(0xc000051ed8),
                allMethods: (*"cmd/compile/internal/types.Fields")(0xc000051ee0),
                Nod: *(*"cmd/compile/internal/types.Node")(0xc0005de500),
                Orig: *(*"cmd/compile/internal/types.Type")(0xc000051ec0),
                Cache: (*"struct { cmd/compile/internal/types.ptr *cmd/compile/internal/types.Type; cmd/compile/internal/types.slice *cmd/compile/internal/types.Type }")(0xc000051ef8),
                Sym: *cmd/compile/internal/types.Sym nil,
                Vargen: 0,
                Etype: cmd/compile/internal/gc.TINTER (24),
                Align: 8,
                flags: 0,},
        Cache: struct { cmd/compile/internal/types.ptr *cmd/compile/internal/types.Type; cmd/compile/internal/types.slice *cmd/compile/internal/types.Type } {
                ptr: *(*"cmd/compile/internal/types.Type")(0xc001aa96e0),
                slice: *(*"cmd/compile/internal/types.Type")(0xc001aa97a0),},
        Sym: *cmd/compile/internal/types.Sym nil,
        Vargen: 0,
        Etype: cmd/compile/internal/gc.TINTER (24),
        Align: 8,
        flags: 0,}

I did a little bit of digging and the problem seems to be that the compiler is not properly decomposing this store into two smaller stores for some reason. I've bisected the tree from 1.15.4 where this worked and I've found the commit that introduced this bug to be 15f01d6.

I'm happy to continue working on this and submit a fix, I've spent some time tracking this down and figured I'd at least create an issue and see if there are any obvious things to look at where things may be going wrong from folks who are more familiar with the actual SSA code than I am at the present moment.
@derekparker
Copy link
Contributor Author

cc @cherrymui @dr2chase @randall77

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Nov 13, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.16 milestone Nov 13, 2020
@cuonglm
Copy link
Member

cuonglm commented Nov 13, 2020

Here's smaller reproducible program:

package p

type S struct{}

func (S) M() {}

type I interface {
	M()
}

func f(i I) {
	o := i.(interface{})
	if _, ok := i.(*S); ok {
		o = nil
	}
	println(o)
}

Somehow, expand_calls does not handle OpSelectN correctly.

@derekparker
Copy link
Contributor Author

Here's some more debug output using the small reproducer above:

expandsCalls((*S).M)
v2p[v10 = SelectN <mem> [0] v9] = 0

expandsCalls(f)
allOrdered[0] = b3, v10 = SelectN <mem> [0] v9, uses=1
rewriteSelect(v10 = SelectN <mem> [0] v9, v10 = SelectN <mem> [0] v9, 0)
storeArg v7 = Addr <*uint8> {type.interface {}} v3, *uint8, 0
        storeArgOrLoad(v41 = SP <uintptr>;  v7 = Addr <*uint8> {type.interface {}} v3;  v1;  *uint8; 0)
                storeArg returns v28 = Store <mem> {*uint8} v30 v7 v1
storeArg v33 = IMake <interface {}> v27 v9, interface {}, 0
        storeArgOrLoad(v41 = SP <uintptr>;  v33 = IMake <interface {}> v27 v9;  v32;  interface {}; 0)
        storeArgOrLoad(v41 = SP <uintptr>;  v27 = Phi <uintptr> v16 v29 (o.type[uintptr]);  v32;  uintptr; 0)

expandsCalls(main)
                storeArg returns v24 = Store <mem> {uintptr} v40 v27 v32
        storeArgOrLoad(v41 = SP <uintptr>;  v9 = Phi <*uint8> v17 v25 (o.data[*uint8]);  v24;  *uint8; 8)
                storeArg returns v11 = Store <mem> {*uint8} v13 v9 v24
v2p[v5 = Arg <I> {i} (i[I])] = 0
v2p[v5 = Arg <I> {i} (i[I])] = 1
v2p[v8 = ITab <*uint8> v5] = 0
v2p[v5 = Arg <I> {i} (i[I])] = 2
v2p[v17 = IData <interface {}> v5 (o.data[*uint8])] = 0
v2p[v32 = SelectN <mem> [0] v31] = 0
v2p[v35 = SelectN <mem> [0] v34] = 0
v2p[v37 = SelectN <mem> [0] v36] = 0
v2p[v39 = SelectN <mem> [0] v38] = 0
allOrdered[0] = b7, v32 = SelectN <mem> [0] v31, uses=1
rewriteSelect(v32 = SelectN <mem> [0] v31, v32 = SelectN <mem> [0] v31, 0)
allOrdered[1] = b7, v35 = SelectN <mem> [0] v34, uses=1
rewriteSelect(v35 = SelectN <mem> [0] v34, v35 = SelectN <mem> [0] v34, 0)
allOrdered[2] = b7, v37 = SelectN <mem> [0] v36, uses=1
rewriteSelect(v37 = SelectN <mem> [0] v36, v37 = SelectN <mem> [0] v36, 0)
allOrdered[3] = b7, v39 = SelectN <mem> [0] v38, uses=1

expandsCalls(S.M)
storeArg v13 = Addr <*uint8> {type.interface {}} v3, *uint8, 0
        storeArgOrLoad(v2 = SP <uintptr>;  v13 = Addr <*uint8> {type.interface {}} v3;  v5;  *uint8; 0)
                storeArg returns v15 = Store <mem> {*uint8} v7 v13 v5
rewriteSelect(v39 = SelectN <mem> [0] v38, v39 = SelectN <mem> [0] v38, 0)
storeArg v39 = IMake <interface {}> v28 v33, interface {}, 0
allOrdered[4] = b2, v17 = IData <interface {}> v5 (o.data[*uint8]), uses=2
        storeArgOrLoad(v2 = SP <uintptr>;  v39 = IMake <interface {}> v28 v33;  v38;  interface {}; 0)
        storeArgOrLoad(v2 = SP <uintptr>;  v28 = Phi <uintptr> v22 v35 (o.type[uintptr]);  v38;  uintptr; 0)
                storeArg returns v4 = Store <mem> {uintptr} v23 v28 v38
        storeArgOrLoad(v2 = SP <uintptr>;  v33 = Phi <*uint8> v8 v31 (o.data[*uint8]);  v4;  *uint8; 8)
                storeArg returns v14 = Store <mem> {*uint8} v6 v33 v4
v2p[v38 = SelectN <mem> [0] v37] = 0
v2p[v41 = SelectN <mem> [0] v40] = 0
v2p[v43 = SelectN <mem> [0] v42] = 0
v2p[v45 = SelectN <mem> [0] v44] = 0
allOrdered[0] = b9, v38 = SelectN <mem> [0] v37, uses=1
rewriteSelect(v38 = SelectN <mem> [0] v37, v38 = SelectN <mem> [0] v37, 0)
allOrdered[1] = b9, v41 = SelectN <mem> [0] v40, uses=1
rewriteSelect(v41 = SelectN <mem> [0] v40, v41 = SelectN <mem> [0] v40, 0)
allOrdered[2] = b9, v43 = SelectN <mem> [0] v42, uses=1
rewriteSelect(v43 = SelectN <mem> [0] v42, v43 = SelectN <mem> [0] v42, 0)
allOrdered[3] = b9, v45 = SelectN <mem> [0] v44, uses=1
rewriteSelect(v45 = SelectN <mem> [0] v44, v45 = SelectN <mem> [0] v44, 0)

expandsCalls(I.M)
storeArg v14 = Load <*uint8> v15 v1, uintptr, 0
        storeArgOrLoad(v2 = SP <uintptr>;  v14 = Load <*uint8> v15 v1;  v1;  uintptr; 0)
                storeArg returns v16 = Store <mem> {uintptr} v6 v14 v1
v2p[v13 = SelectN <mem> [0] v12] = 0
allOrdered[0] = b1, v13 = SelectN <mem> [0] v12, uses=1
rewriteSelect(v13 = SelectN <mem> [0] v12, v13 = SelectN <mem> [0] v12, 0)
rewriteSelect(v17 = IData <interface {}> v5 (o.data[*uint8]), v17 = IData <interface {}> v5 (o.data[*uint8]), 0)
rewriteSelect(v17 = IData <interface {}> v5 (o.data[*uint8]), v5 = Arg <I> {i} (i[I]), 8)
        new v19 = Arg <interface {}> {i} [8]
allOrdered[5] = b1, v8 = ITab <*uint8> v5, uses=3
rewriteSelect(v8 = ITab <*uint8> v5, v8 = ITab <*uint8> v5, 0)
rewriteSelect(v8 = ITab <*uint8> v5, v5 = Arg <I> {i} (i[I]), 0)
allOrdered[6] = b1, v5 = Arg <I> {i} (i[I]), uses=0

@dr2chase dr2chase self-assigned this Nov 13, 2020
@gopherbot
Copy link

Change https://golang.org/cl/270057 mentions this issue: cmd/compile: fix load of interface{}-typed OpIData in expand_calls

@gopherbot
Copy link

Change https://golang.org/cl/276952 mentions this issue: cmd/compile: set correct type for OpIData

@josharian
Copy link
Contributor

(Further comment here that I wanted to leave on the CL, but gerrit is almost completely non-functional on Firefox on mobile.)

Specifically I think you should run compilecmp -fn=changed. Also, this reminds me of https://go-review.googlesource.com/c/go/+/229984. Please see Keith’s comment there.

Thanks for digging to find a root cause here.

gopherbot pushed a commit that referenced this issue Dec 14, 2020
@cuonglm
cmd/compile: set correct type for OpIData
0a02371 
Since CL 270057, there're many attempts to fix the expand_calls pass
with interface{}-typed. But all of them did not fix the root cause. The
main issue is during SSA conversion in gc/ssa.go, for empty interface
case, we make its type as n.Type, instead of BytePtr.

To fix these, we can just use BytePtr for now, since when itab fields
are treated as scalar.

No significal changes on compiler speed, size.

cmd/compile/internal/ssa
expandCalls.func6 9488 -> 9232  (-2.70%)

file                       before   after    Δ       %
cmd/compile/internal/ssa.s 3992893  3992637  -256    -0.006%
total                      20500447 20500191 -256    -0.001%

Fixes #43112
Updates #42784
Updates #42727
Updates #42568

Change-Id: I0b15d9434e0be5448453e61f98ef9c2d6cd93792
Reviewed-on: https://go-review.googlesource.com/c/go/+/276952
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
Go1.16
Development

No branches or pull requests
6 participants
@josharian @derekparker @dr2chase @ianlancetaylor @cuonglm @gopherbot