cmd/compile: panic on conversion to anonymous interface when 2 versions of library are built

[jirfag]

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

$ go version
go version go1.11.4 darwin/amd64

Does this issue reproduce with the latest release?

yes it does

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/denis/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/denis/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nl/54f5x38s4m53mkzzj92zsj340000gn/T/go-build581161643=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run

git clone https://github.com/golangci/test4.git
go run main.go

and got

panic: interface conversion: *ssa.Call is not interface { ssa.setNum(int) }: missing method setNum

goroutine 1 [running]:
github.com/golangci/test2/ssa.Panics(0x105cde0, 0xc000064020, 0x2)
        /Users/denis/go/pkg/mod/github.com/golangci/[email protected]/ssa/func.go:6 +0x82
main.main()
        /Users/denis/go/src/github.com/golangci/test4/main.go:14 +0xa0
exit status 2

What did you expect to see?

No panic with anonymous interface cast.

What did you see instead?

Panic.

[mvdan]

Unless I'm missing something, this is a run-time panic, not a compiler panic. Any reason you assigned this to cmd/compile?

Also, were you able to reproduce this with a smaller package or Go program?

[jirfag]

sorry, suggest more appropriate component, please. I set cmd/compile because it looks like a compiler bug and #24547 had such component.

I will try to reproduce it with a smaller program.

[jirfag]

I could reproduce it with much smaller program

[mvdan]

sorry, suggest more appropriate component, please.

Oh, I don't know if the compiler has a bug here - certainly possible, but I was wondering if you had dug up the cause.

If you can reproduce the crash with a smaller self-contained program, please share it. That will make debugging the problem faster.

[jirfag]

I have already changed the description of the issue with the smaller test case:

git clone https://github.com/golangci/test4.git
go run main.go

[mvdan]

Sorry, but that repo is a 404. Can you just share the file on play.golang.org?

[jirfag]

sorry, it was private, I made the repo a public. There are a few packages needed, I can't share it on play.golang.org

[mvdan]

It does seem like something is very wrong. I've been able to minimize your test case to a single module with about half the amount of code, but still involving two packages with very similar types and the same package names.

$ git clone https://github.com/mvdan/go-issue-29612
$ cd go-issue-29612
$ go build
$ ./foo
Works succeeded
panic: interface conversion: *ssa.T is not interface { ssa.foo() }: missing method foo

goroutine 1 [running]:
test.tld/foo/p2/ssa.Panics(0x45ecc0, 0x4dce30)
        /home/mvdan/foo/p2/ssa/ssa.go:21 +0x52
main.main()
        /home/mvdan/foo/main.go:16 +0x88
$ # remove ssa1 from main.go; see the comment there
$ go build
$ ./foo
Works succeeded
Panics succeeded

This weird behavior happens on both 1.11.4 and go version devel +73fb3c38a6 Mon Jan 7 14:13:33 2019 +0000 linux/amd64. My best guess without any digging is that either the compiler or the runtime get confused by the very similar types in the very similar pakages, and there's a conflict or misplacement somewhere.

The fact that removing the first ssa package from main.go makes the second ssa package work fine tells me that there's no bug in either of the ssa packages.

/cc @randall77 @ianlancetaylor @dr2chase for some input

[mvdan]

Broken in 1.11 too, and we're late in the cycle, so I'm leaving this for 1.13 for now. Perhaps it's reconsidered to be critical enough (or simple enough to fix) to include in 1.12.

[ianlancetaylor]

Interesting. This has been broken since at least Go 1.1. It works with gccgo.

[cuonglm]

@mvdan it's a runtime bug.

The interface type interface { ssa.foo() } both appears in test.tld/foo/p1 and test.tld/foo/p2. Why doing (*itab).init(), with type interface { ssa.foo() } of test.tld/foo/p2, the code get method foo from test.tld/foo/p1:

name := inter.typ.nameOff(i.name)
iname := name.name()
ipkg := name.pkgPath()
if ipkg == "" {
	ipkg = inter.pkgpath.name()
}

Here name.pkgPath() returns test.tld/foo/p1. Because the interface methods are shared in *itab, the method foo() we see in test.tld/foo/p2 has package path test.tld/foo/p1.

To confirm that behavior, just changing foo() to Foo() or make p2 import before p1:

~/go/src/test.tld/foo(master ✗) cat main.go 
package main

import (
	ssa2 "test.tld/foo/p2/ssa"

	ssa1 "test.tld/foo/p1/ssa"
)

func main() {
	// Remove the ssa1 import and these two lines to magically fix the program.
	v1 := &ssa1.T{}
	_ = v1

	v2 := &ssa2.T{}
	ssa2.Works(v2)
	println("Works succeeded")
	ssa2.Panics(v2)
	println("Panics succeeded")
}
~/go/src/test.tld/foo(master ✗) go-tip run main.go
Works succeeded
Panics succeeded

I'm trying to fix this bug but don't know how, I'm not too familiar with the runtime source code. One solution I'm thinking about, if the type package path (test.tld/foo/p2/ssa) and the interface package path (test.tld/foo/p1/ssa) have the same suffix (ssa), then the type implements the interface

go/src/runtime/iface.go

Line 216 in 44cf595

if tname.isExported() || pkgPath == ipkg {

Any idea or suggestion @ianlancetaylor ?

[gopherbot]

Change https://golang.org/cl/157397 mentions this issue: runtime: fix same anonymous interface conversion panic

[ianlancetaylor]

I haven't looked but I'm guessing that the problem is that the two distinct interfaces have the same hash. I think the compiler or the runtime needs to include the full package path in the hash of any interface that contains an unexported method.

[cuonglm]

@ianlancetaylor I checked and the hashes are different.

The fact is that both test.tld/foo/p1/ssa.interface { foo() } and test.tld/foo/p2/ssa.interface { foo() } appears as interface { ssa.foo() } in symtab, so they're treated as the same in itab.

I checked runtime and sounds like it doesn't turn test.tld/foo/p1/ssa.interface { foo() } to interface { ssa.foo() }, so it should be the compiler. Do you know where?

[ianlancetaylor]

All the type descriptor information is generated in cmd/compile/internal/gc/reflect.go.

[cuonglm]

@ianlancetaylor I thought we think it too complicated. Just adding a condition that interface is literal or not will fix that problem.

I updated the CL, please take a look.

[ianlancetaylor]

I don't think the updated CL is correct.

It sounds like the problem is that there are two interfaces that look the same, but come from different packages, and have an unexported method. Because of the unexported method, the two interfaces should be treated as different, but the itab hash table is treating them as the same interface.

If I have described the problem correctly, then that is what we have to fix: one way or another, the two different interfaces must not hash to the same entry in the itab hash table.

Perhaps the reason that they are same entry in the itab hash table is that they are the same entry in the executable's symbol table. That is, perhaps the linker is merging the two types into the same symbol. If that is the case, then that is what we must fix. We must change the symbol names.

[cuonglm]

@ianlancetaylor: Maybe I missed something, but they are not the same entry in this case, only the methods are the same.
They have diffirent hash inter.typ.hash. As I understand the code, two methods always treated as the same, if it’s unexported, itab use their package path to differentate them.

go/src/runtime/iface.go

Line 216 in 5848b6c

if tname.isExported() || pkgPath == ipkg {

. So one/a.fooer.foo and two/a.fooer.foo will have the same symbol.

Also it’s only matter for literal interface, named interface does not affect by this change. But literal interface can not be used outside the packge it was declared. That’s why I think add isLiteral to if condition above is enough.

[ianlancetaylor]

So one/a.fooer.foo and two/a.fooer.foo will have the same symbol.

Can you clarify what you mean by that? What you mean by "the same symbol"?

[cuonglm]

@ianlancetaylor I mean in this line

go/src/runtime/iface.go

Line 209 in 5848b6c

t := &xmhdr[j]

will return the same t for both one/a.fooer.foo and two/a.fooer.foo. Then when the code reaches line 216, it checks the method is exported or in the same package (pkgPath == ipkg).

That's why I said if we add one more check for literal interface, we don't affect named interface, because they're cover by two previous conditions:

if tname.isExported() || pkgPath == ipkg

The condition inter.isLiteral() covers the literal interface case. I added your test case in CL https://go-review.googlesource.com/c/go/+/157397/3/src/cmd/go/go_test.go#6208

Maybe I don't understand the code correctly, but for me, all interface with the same signature is shared, and they differentiate by path if not exported.

[ianlancetaylor]

The type t set at iface.go:209 is type method. It has a package path fetched from the name field (see the comment beginning with "name is an encoded type name with optional extra data" in reflect/type.go). If package one/a defines an interface with method foo, then that package path should be one/a. And if package two/a defines an interface with method foo, then that package path should be two/a. So if line 209 sets the same t for both one/a.fooer and two/a.fooer, then something must be wrong. What is the package path for that method? It should be either one/a or two/a. It can't be both.

[cuonglm]

@ianlancetaylor In reflect.Type, the comment lines after name is an encoded type name with optional extra data said that:

// If the import path follows, then 4 bytes at the end of
// the data form a nameOff. The import path is only set for concrete
// methods that are defined in a different package than their type.

So for interface method, it doesn't set, right? That's why the import path can be empty, in that case, import path of type is used.

go/src/runtime/iface.go

Line 213 in 5848b6c

if pkgPath == "" {

[cuonglm]

@ianlancetaylor I added some debug lines:

println("1: ", ipkg)
if ipkg == "" {
	ipkg = inter.pkgpath.name()
}
println("2: ", ipkg, i, itype, itype.string(), itype.hash)
for ; j < nt; j++ {
	t := &xmhdr[j]
	tname := typ.nameOff(t.name)
	if typ.typeOff(t.mtyp) == itype && tname.name() == iname {
		pkgPath := tname.pkgPath()
		println("3: ", pkgPath)
		if pkgPath == "" {
			pkgPath = typ.nameOff(x.pkgpath).name()
		}
		println("4: ", pkgPath, t, tname.name(), iname, typ.typeOff(t.mtyp).hash)
                ....

And the result:

1:  
2:  one/a 0x4a08c0 0x4963a0 func() 4135763190
3:  
4:  one/a 0x49de20 m m 4135763190
true
1:  
2:  one/a 0x4a08c0 0x4963a0 func() 4135763190
3:  
4:  two/a 0x49e2a0 m m 4135763190
false
1:  
2:  two/a 0x4a09c0 0x4963a0 func() 4135763190
3:  
4:  one/a 0x49de20 m m 4135763190
false
1:  
2:  two/a 0x4a09c0 0x4963a0 func() 4135763190
3:  
4:  two/a 0x49e2a0 m m 4135763190
true

For the program in the issue:

1:  
2:  test.tld/foo/p2/ssa 0x4600c0 0x45a4c0 func() 4135763190
3:  
4:  test.tld/foo/p2/ssa 0x45ed88 foo foo 4135763190
Works succeeded
1:  test.tld/foo/p1/ssa
2:  test.tld/foo/p1/ssa 0x45ee50 0x45a4c0 func() 4135763190
3:  
4:  test.tld/foo/p2/ssa 0x45ed88 foo foo 4135763190
1:  test.tld/foo/p1/ssa
2:  test.tld/foo/p1/ssa 0x45ee50 0x45a4c0 func() 4135763190
3:  
4:  test.tld/foo/p2/ssa 0x45ed88 foo foo 4135763190
1:  
2:   0x45ffc0 0x45aaa0 func() string 516648354
3:  
4:  runtime 0x45f4c8 Error Error 516648354
panic: interface conversion: *ssa.T is not interface { ssa.foo() }: missing method foo

goroutine 1 [running]:
test.tld/foo/p2/ssa.Panics(0x45ed40, 0x4dce10)
	/home/cuonglm/go/src/test.tld/foo/p2/ssa/ssa.go:38 +0x8e
main.main()
	/home/cuonglm/go/src/test.tld/foo/main.go:15 +0x88

So sounds like 2 interfaces treated the same. With literal interface, the name.pkgPath() is set to the first one seen test.tld/foo/p1/ssa, that's why the same literal interface in other package never seen.

Also, inter.pkgpath.name() seems to be set for all named interface, and empty for all literal interface. If that's true, then we can use it for checking interface literal or not instead of parsing interface name like I did in previous CL update.

[ianlancetaylor]

So sounds like 2 interfaces treated the same. With literal interface, the name.pkgPath() is set to the first one seen test.tld/foo/p1/ssa, that's why the same literal interface in other package never seen.

Thanks, that seems like you have found the bug there. The code in package p2/ssa should not be seeing an interface type that has a pkgPath of p1/ssa.

[cuonglm]

@rsc @ianlancetaylor do you have any suggestion to fix this issue?

[cuonglm]

I added a debug line in

go/src/cmd/compile/internal/gc/reflect.go

Line 1259 in a192507

fmt.Printf("XXX: %#v - %#v - %#v\n", t, ot, tpkg)

I got:

go-tip run main.go
# test.tld/foo/p1/sas
XXX: interface { ssa.foo() } - 80 - (*types.Pkg)(nil)
XXX: interface {} - 80 - (*types.Pkg)(nil)
# test.tld/foo/p2/ssa
XXX: fooer - 96 - &types.Pkg{Path:"", Name:"ssa", Prefix:"\"\"", Syms:map[string]*types.Sym{"(*T).foo":(*T).foo, ".fp":.fp, ".i0":.i0, ".s0":.s0, ".s1":.s1, ".sink":.sink, ".this":.this, "Error":Error, "Panics":Panics, "T":T, "T.foo":T.foo, "Works":Works, "_":_, "alignme":alignme, "append":append, "bool":bool, "byte":byte, "cap":cap, "cgo":cgo, "close":close, "complex":complex, "complex128":complex128, "complex64":complex64, "copy":copy, "delete":delete, "enabled":enabled, "error":error, "false":false, "float32":float32, "float64":float64, "foo":foo, "fooer":fooer, "fooer.foo":fooer.foo, "imag":imag, "int":int, "int16":int16, "int32":int32, "int64":int64, "int8":int8, "iota":iota, "len":len, "make":make, "needed":needed, "new":new, "nil":nil, "pad":pad, "panic":panic, "print":print, "println":println, "real":real, "recover":recover, "rune":rune, "string":string, "true":true, "uint":uint, "uint16":uint16, "uint32":uint32, "uint64":uint64, "uint8":uint8, "uintptr":uintptr, "v":v}, Pathsym:(*obj.LSym)(0xc000540c40), Height:0, Imported:false, Direct:false}
YYY: &types.Pkg{Path:"", Name:"ssa", Prefix:"\"\"", Syms:map[string]*types.Sym{"(*T).foo":(*T).foo, ".fp":.fp, ".i0":.i0, ".s0":.s0, ".s1":.s1, ".sink":.sink, ".this":.this, "Error":Error, "Panics":Panics, "T":T, "T.foo":T.foo, "Works":Works, "_":_, "alignme":alignme, "append":append, "bool":bool, "byte":byte, "cap":cap, "cgo":cgo, "close":close, "complex":complex, "complex128":complex128, "complex64":complex64, "copy":copy, "delete":delete, "enabled":enabled, "error":error, "false":false, "float32":float32, "float64":float64, "foo":foo, "fooer":fooer, "fooer.foo":fooer.foo, "imag":imag, "int":int, "int16":int16, "int32":int32, "int64":int64, "int8":int8, "iota":iota, "len":len, "make":make, "needed":needed, "new":new, "nil":nil, "pad":pad, "panic":panic, "print":print, "println":println, "real":real, "recover":recover, "rune":rune, "string":string, "true":true, "uint":uint, "uint16":uint16, "uint32":uint32, "uint64":uint64, "uint8":uint8, "uintptr":uintptr, "v":v}, Pathsym:(*obj.LSym)(0xc000540c40), Height:0, Imported:false, Direct:false}
XXX: interface { ssa.foo() } - 80 - (*types.Pkg)(nil)
XXX: interface {} - 80 - (*types.Pkg)(nil)
Works succeeded
panic: interface conversion: *ssa.T is not interface { ssa.foo() }: missing method foo

goroutine 1 [running]:
test.tld/foo/p2/ssa.Panics(0x45ece0, 0x4dce10)
	/home/cuonglm/go/src/test.tld/foo/p2/ssa/ssa.go:22 +0x8e
main.main()
	/home/cuonglm/go/src/test.tld/foo/main.go:13 +0x7c
exit status 2

What is ot? Why does empty interface and anonymous interface have the same ot?

[ianlancetaylor]

The ot variable holds an offset into the type descriptor that is being built. It's just used to put values at the right place.

[gopherbot]

Change https://golang.org/cl/165859 mentions this issue: cmd/compile: use full package path to print unexported interface methods

[gopherbot]

Change https://golang.org/cl/170157 mentions this issue: cmd/compile: use full package path for unexported interface methods, struct fields

[gopherbot]

Change https://golang.org/cl/195782 mentions this issue: test: add test coverage for type-switch hash collisions